Skip to content

msglist [nfc]: Handle start markers within widget code, not model #1512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 14, 2025
+64 −91
Merged

msglist [nfc]: Handle start markers within widget code, not model #1512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86e2397
msglist [nfc]: Assert the model is non-null a bit more
gnprice May 2, 2025
a6e0efd
msglist [nfc]: Simplify a bit by centralizing `model!` null-assertion
gnprice May 2, 2025
4b69adf
msglist [nfc]: Make start/loading indicators their own widgets
gnprice May 8, 2025
c5e6957
msglist [nfc]: Handle start markers within widget code, not model
gnprice May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 0 additions & 49 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,21 +63,6 @@ class MessageListMessageItem extends MessageListMessageBaseItem {
});
}

/// Indicates the app is loading more messages at the top.
// TODO(#80): or loading at the bottom, by adding a [MessageListDirection.newer]
class MessageListLoadingItem extends MessageListItem {
final MessageListDirection direction;

const MessageListLoadingItem(this.direction);
}

enum MessageListDirection { older }

/// Indicates we've reached the oldest message in the narrow.
class MessageListHistoryStartItem extends MessageListItem {
const MessageListHistoryStartItem();
}

/// The sequence of messages in a message list, and how to display them.
///
/// This comprises much of the guts of [MessageListView].
Expand Down Expand Up @@ -161,11 +146,6 @@ mixin _MessageSequence {

static int _compareItemToMessageId(MessageListItem item, int messageId) {
switch (item) {
case MessageListHistoryStartItem(): return -1;
case MessageListLoadingItem():
switch (item.direction) {
case MessageListDirection.older: return -1;
}
case MessageListRecipientHeaderItem(:var message):
case MessageListDateSeparatorItem(:var message):
if (message.id == null) return 1; // TODO(#1441): test
Expand Down Expand Up @@ -328,37 +308,12 @@ mixin _MessageSequence {
showSender: !canShareSender, isLastInBlock: true));
}

/// Update [items] to include markers at start and end as appropriate.
void _updateEndMarkers() {
assert(fetched);
assert(!(fetchingOlder && fetchOlderCoolingDown));
final effectiveFetchingOlder = fetchingOlder || fetchOlderCoolingDown;
assert(!(effectiveFetchingOlder && haveOldest));
final startMarker = switch ((effectiveFetchingOlder, haveOldest)) {
(true, _) => const MessageListLoadingItem(MessageListDirection.older),
(_, true) => const MessageListHistoryStartItem(),
(_, _) => null,
};
final hasStartMarker = switch (items.firstOrNull) {
MessageListLoadingItem() => true,
MessageListHistoryStartItem() => true,
_ => false,
};
switch ((startMarker != null, hasStartMarker)) {
case (true, true): items[0] = startMarker!;
case (true, _ ): items.addFirst(startMarker!);
case (_, true): items.removeFirst();
case (_, _ ): break;
}
}

/// Recompute [items] from scratch, based on [messages], [contents], and flags.
void _reprocessAll() {
items.clear();
for (var i = 0; i < messages.length; i++) {
_processMessage(i);
}
_updateEndMarkers();
}
}

Expand Down Expand Up @@ -508,7 +463,6 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
_fetched = true;
_haveOldest = result.foundOldest;
_updateEndMarkers();
notifyListeners();
}

Expand Down Expand Up @@ -555,7 +509,6 @@ class MessageListView with ChangeNotifier, _MessageSequence {
|| (narrow as TopicNarrow).with_ == null);
assert(messages.isNotEmpty);
_fetchingOlder = true;
_updateEndMarkers();
notifyListeners();
final generation = this.generation;
bool hasFetchError = false;
Expand Down Expand Up @@ -601,13 +554,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
.wait().then((_) {
if (this.generation != generation) return;
_fetchOlderCoolingDown = false;
_updateEndMarkers();
notifyListeners();
}));
} else {
_fetchOlderCooldownBackoffMachine = null;
}
_updateEndMarkers();
notifyListeners();
}
}
Expand Down
95 changes: 63 additions & 32 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,11 @@ class MessageList extends StatefulWidget {
}

class _MessageListState extends State<MessageList> with PerAccountStoreAwareStateMixin<MessageList> {
MessageListView? model;
MessageListView get model => _model!;
MessageListView? _model;

final MessageListScrollController scrollController = MessageListScrollController();

final ValueNotifier<bool> _scrollToBottomVisible = ValueNotifier<bool>(false);

@override
Expand All @@ -460,32 +463,32 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat

@override
void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages
model?.dispose();
_model?.dispose();
_initModel(PerAccountStoreWidget.of(context));
}

@override
void dispose() {
model?.dispose();
_model?.dispose();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if dispose can ever get called before _initModel.

I guess the answer is no. When dispose is called, the State should be in its "ready" state.

  void dispose() {
    assert(_debugLifecycleState == _StateLifecycle.ready);
    assert(() {
      _debugLifecycleState = _StateLifecycle.defunct;
      return true;
    }());
    assert(debugMaybeDispatchDisposed(this));
  }

and from the dart doc of _StateLifecycle, "ready" is later than "created" and "initialized" in State objects' lifecycle:

/// Tracks the lifecycle of [State] objects when asserts are enabled.
enum _StateLifecycle {
  /// The [State] object has been created. [State.initState] is called at this
  /// time.
  created,

  /// The [State.initState] method has been called but the [State] object is
  /// not yet ready to build. [State.didChangeDependencies] is called at this time.
  initialized,

  /// The [State] object is ready to build and [State.dispose] has not yet been
  /// called.
  ready,

  /// The [State.dispose] method has been called and the [State] object is
  /// no longer able to build.
  defunct,
}

so State.didChangeDependencies (and then onNewStore) should have been called already.

On a side-note, we have been cautious about this at other places. While the framework might maintain this guarantee that onNewStore is called before dispose is called, it's probably easier to rest assure that ?.dispose() is always going to be safe. So I'm actually not sure if we want a null-assert here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the investigation.

Given those results, I think there would be some value in systematically having our dispose methods assume that didChangeDependencies (and so onNewStore where applicable) has been called. By visibly relying on that guarantee from the framework, it'd help teach the reader that that guarantee is there, and give confidence in relying on it in whatever new code they're writing.

That'd be an independent change from this one, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, _EmojiPickerState seems like a place where a similar refactor may apply. The teaching aspect of this is refreshing think about.

scrollController.dispose();
_scrollToBottomVisible.dispose();
super.dispose();
}

void _initModel(PerAccountStore store) {
model = MessageListView.init(store: store, narrow: widget.narrow);
model!.addListener(_modelChanged);
model!.fetchInitial();
_model = MessageListView.init(store: store, narrow: widget.narrow);
model.addListener(_modelChanged);
model.fetchInitial();
}

void _modelChanged() {
if (model!.narrow != widget.narrow) {
if (model.narrow != widget.narrow) {
// Either:
// - A message move event occurred, where propagate mode is
// [PropagateMode.changeAll] or [PropagateMode.changeLater]. Or:
// - We fetched a "with" / topic-permalink narrow, and the response
// redirected us to the new location of the operand message ID.
widget.onNarrowChanged(model!.narrow);
widget.onNarrowChanged(model.narrow);
}
setState(() {
// The actual state lives in the [MessageListView] model.
Expand All @@ -507,7 +510,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// but makes things a bit more complicated to reason about.
// The cause seems to be that this gets called again with maxScrollExtent
// still not yet updated to account for the newly-added messages.
model?.fetchOlder();
model.fetchOlder();
}
}

Expand All @@ -528,8 +531,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat

@override
Widget build(BuildContext context) {
assert(model != null);
if (!model!.fetched) return const Center(child: CircularProgressIndicator());
if (!model.fetched) return const Center(child: CircularProgressIndicator());

// Pad the left and right insets, for small devices in landscape.
return SafeArea(
Expand Down Expand Up @@ -567,13 +569,12 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat

Widget _buildListView(BuildContext context) {
const centerSliverKey = ValueKey('center sliver');
final zulipLocalizations = ZulipLocalizations.of(context);

// The list has two slivers: a top sliver growing upward,
// and a bottom sliver growing downward.
// Each sliver has some of the items from `model!.items`.
// Each sliver has some of the items from `model.items`.
const maxBottomItems = 1;
final totalItems = model!.items.length;
final totalItems = model.items.length;
final bottomItems = totalItems <= maxBottomItems ? totalItems : maxBottomItems;
final topItems = totalItems - bottomItems;

Expand All @@ -599,17 +600,19 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final messageId = (key as ValueKey<int>).value;
final itemIndex = model!.findItemWithMessageId(messageId);
final itemIndex = model.findItemWithMessageId(messageId);
if (itemIndex == -1) return null;
final childIndex = totalItems - 1 - (itemIndex + bottomItems);
if (childIndex < 0) return null;
return childIndex;
},
childCount: topItems,
childCount: topItems + 1,
(context, childIndex) {
if (childIndex == topItems) return _buildStartCap();

final itemIndex = totalItems - 1 - (childIndex + bottomItems);
final data = model!.items[itemIndex];
final item = _buildItem(zulipLocalizations, data);
final data = model.items[itemIndex];
final item = _buildItem(data);
return item;
}));

Expand Down Expand Up @@ -637,7 +640,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final messageId = (key as ValueKey<int>).value;
final itemIndex = model!.findItemWithMessageId(messageId);
final itemIndex = model.findItemWithMessageId(messageId);
if (itemIndex == -1) return null;
final childIndex = itemIndex - topItems;
if (childIndex < 0) return null;
Expand All @@ -654,8 +657,8 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
if (childIndex == bottomItems) return TypingStatusWidget(narrow: widget.narrow);

final itemIndex = topItems + childIndex;
final data = model!.items[itemIndex];
return _buildItem(zulipLocalizations, data);
final data = model.items[itemIndex];
return _buildItem(data);
}));

if (!ComposeBox.hasComposeBox(widget.narrow)) {
Expand Down Expand Up @@ -687,18 +690,21 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
]);
}

Widget _buildItem(ZulipLocalizations zulipLocalizations, MessageListItem data) {
Widget _buildStartCap() {
// These assertions are invariants of [MessageListView].
assert(!(model.fetchingOlder && model.fetchOlderCoolingDown));
final effectiveFetchingOlder =
model.fetchingOlder || model.fetchOlderCoolingDown;
assert(!(model.haveOldest && effectiveFetchingOlder));
Comment on lines +693 to +698
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert(fetched) being absent seems like a notable change here.

I suppose _updateEndMarkers has it because it nothing from items matter when fetched is false, because the widget code will just show the loading indicator and skip building the list view altogether.

I feel that it still applies here, right? _buildStartCap shouldn't be called at all if nothing has been fetched yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the bulk of the code on _MessageListState runs only when model.fetched is true. When model.fetched is false, the actual list doesn't get shown, so none of the "build" code runs beyond the first line of the build method.

In MessageListView (or _MessageSequence) where _updateEndMarkers lives/lived, the control flow between different methods is a lot more complicated (going up and down in the file), and so that's less clear.

I think this method's logic also makes sense even without the assumption that model.fetched is true. If we're fetching more, we say we're doing so; if we know we have the oldest messages, we say so; else say nothing. In _updateEndMarkers, there's the additional complication that the method is mutating items, so in order to think through the logic one wants to narrow down the possible states that items could be in.

return switch ((effectiveFetchingOlder, model.haveOldest)) {
(true, _) => const _MessageListLoadingMore(),
(_, true) => const _MessageListHistoryStart(),
(_, _) => const SizedBox.shrink(),
};
}

Widget _buildItem(MessageListItem data) {
switch (data) {
case MessageListHistoryStartItem():
return Center(
child: Padding(
padding: const EdgeInsets.symmetric(vertical: 16.0),
child: Text(zulipLocalizations.noEarlierMessages))); // TODO use an icon
case MessageListLoadingItem():
return const Center(
child: Padding(
padding: EdgeInsets.symmetric(vertical: 16.0),
child: CircularProgressIndicator())); // TODO perhaps a different indicator
case MessageListRecipientHeaderItem():
final header = RecipientHeader(message: data.message, narrow: widget.narrow);
return StickyHeaderItem(allowOverflow: true,
Expand All @@ -719,6 +725,31 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
}
}

class _MessageListHistoryStart extends StatelessWidget {
const _MessageListHistoryStart();

@override
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
return Center(
child: Padding(
padding: const EdgeInsets.symmetric(vertical: 16.0),
child: Text(zulipLocalizations.noEarlierMessages))); // TODO use an icon
}
}

class _MessageListLoadingMore extends StatelessWidget {
const _MessageListLoadingMore();

@override
Widget build(BuildContext context) {
return const Center(
child: Padding(
padding: EdgeInsets.symmetric(vertical: 16.0),
child: CircularProgressIndicator())); // TODO perhaps a different indicator
}
}

class ScrollToBottomButton extends StatelessWidget {
const ScrollToBottomButton({super.key, required this.scrollController, required this.visible});

Expand Down
11 changes: 1 addition & 10 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1791,7 +1791,6 @@ void main() {
// We check showSender has the right values in [checkInvariants],
// but to make this test explicit:
check(model.items).deepEquals(<void Function(Subject<Object?>)>[
(it) => it.isA<MessageListHistoryStartItem>(),
(it) => it.isA<MessageListRecipientHeaderItem>(),
(it) => it.isA<MessageListMessageItem>().showSender.isTrue(),
(it) => it.isA<MessageListMessageItem>().showSender.isFalse(),
Expand Down Expand Up @@ -1964,12 +1963,6 @@ void checkInvariants(MessageListView model) {
}

int i = 0;
if (model.haveOldest) {
check(model.items[i++]).isA<MessageListHistoryStartItem>();
}
if (model.fetchingOlder || model.fetchOlderCoolingDown) {
check(model.items[i++]).isA<MessageListLoadingItem>();
}
for (int j = 0; j < model.messages.length; j++) {
bool forcedShowSender = false;
if (j == 0
Expand All @@ -1991,9 +1984,7 @@ void checkInvariants(MessageListView model) {
i == model.items.length || switch (model.items[i]) {
MessageListMessageItem()
|| MessageListDateSeparatorItem() => false,
MessageListRecipientHeaderItem()
|| MessageListHistoryStartItem()
|| MessageListLoadingItem() => true,
MessageListRecipientHeaderItem() => true,
});
}
check(model.items).length.equals(i);
Expand Down