From 86e2397d0a19f669358a9fb0f8927b5eca19291c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 20:05:46 -0700 Subject: [PATCH 1/4] msglist [nfc]: Assert the model is non-null a bit more This method is only called from callbacks which can only get invoked after the widget has built; and the build method already requires `model` to have been initialized. This is therefore NFC. --- lib/widgets/message_list.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 98beda3205..dffeec5143 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -507,7 +507,7 @@ class _MessageListState extends State 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(); } } From a6e0efde2daa69f9ce965be9ff3f3809c3dff5ef Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 20:09:02 -0700 Subject: [PATCH 2/4] msglist [nfc]: Simplify a bit by centralizing `model!` null-assertion --- lib/widgets/message_list.dart | 36 ++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index dffeec5143..82be2ffe18 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -448,8 +448,11 @@ class MessageList extends StatefulWidget { } class _MessageListState extends State with PerAccountStoreAwareStateMixin { - MessageListView? model; + MessageListView get model => _model!; + MessageListView? _model; + final MessageListScrollController scrollController = MessageListScrollController(); + final ValueNotifier _scrollToBottomVisible = ValueNotifier(false); @override @@ -460,32 +463,32 @@ class _MessageListState extends State 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(); 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. @@ -507,7 +510,7 @@ class _MessageListState extends State 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(); } } @@ -528,8 +531,7 @@ class _MessageListState extends State 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( @@ -571,9 +573,9 @@ class _MessageListState extends State with PerAccountStoreAwareStat // 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; @@ -599,7 +601,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat // and will not trigger this callback. findChildIndexCallback: (Key key) { final messageId = (key as ValueKey).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; @@ -608,7 +610,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat childCount: topItems, (context, childIndex) { final itemIndex = totalItems - 1 - (childIndex + bottomItems); - final data = model!.items[itemIndex]; + final data = model.items[itemIndex]; final item = _buildItem(zulipLocalizations, data); return item; })); @@ -637,7 +639,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat // and will not trigger this callback. findChildIndexCallback: (Key key) { final messageId = (key as ValueKey).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; @@ -654,7 +656,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat if (childIndex == bottomItems) return TypingStatusWidget(narrow: widget.narrow); final itemIndex = topItems + childIndex; - final data = model!.items[itemIndex]; + final data = model.items[itemIndex]; return _buildItem(zulipLocalizations, data); })); From 4b69adfae505b5b67a36da12ba9d0a5315b2c229 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 15:02:20 -0700 Subject: [PATCH 3/4] msglist [nfc]: Make start/loading indicators their own widgets --- lib/widgets/message_list.dart | 42 +++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 82be2ffe18..01b973f9ca 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -569,7 +569,6 @@ class _MessageListState extends State 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. @@ -611,7 +610,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat (context, childIndex) { final itemIndex = totalItems - 1 - (childIndex + bottomItems); final data = model.items[itemIndex]; - final item = _buildItem(zulipLocalizations, data); + final item = _buildItem(data); return item; })); @@ -657,7 +656,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat final itemIndex = topItems + childIndex; final data = model.items[itemIndex]; - return _buildItem(zulipLocalizations, data); + return _buildItem(data); })); if (!ComposeBox.hasComposeBox(widget.narrow)) { @@ -689,18 +688,12 @@ class _MessageListState extends State with PerAccountStoreAwareStat ]); } - Widget _buildItem(ZulipLocalizations zulipLocalizations, MessageListItem data) { + 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 + return const _MessageListHistoryStart(); case MessageListLoadingItem(): - return const Center( - child: Padding( - padding: EdgeInsets.symmetric(vertical: 16.0), - child: CircularProgressIndicator())); // TODO perhaps a different indicator + return const _MessageListLoadingMore(); case MessageListRecipientHeaderItem(): final header = RecipientHeader(message: data.message, narrow: widget.narrow); return StickyHeaderItem(allowOverflow: true, @@ -721,6 +714,31 @@ class _MessageListState extends State 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}); From c5e695738130adb0924306db84ec0b320fe573bf Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 8 May 2025 15:18:33 -0700 Subject: [PATCH 4/4] msglist [nfc]: Handle start markers within widget code, not model The view-model already exposes flags that express all the same information. By leaving these markers out of the list of items, we can save ourselves a bunch of stateful updates. --- lib/model/message_list.dart | 49 ------------------------------- lib/widgets/message_list.dart | 21 +++++++++---- test/model/message_list_test.dart | 11 +------ 3 files changed, 17 insertions(+), 64 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 275aa66363..2f8777d3ed 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -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]. @@ -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 @@ -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(); } } @@ -508,7 +463,6 @@ class MessageListView with ChangeNotifier, _MessageSequence { } _fetched = true; _haveOldest = result.foundOldest; - _updateEndMarkers(); notifyListeners(); } @@ -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; @@ -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(); } } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 01b973f9ca..3e8850f5cc 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -606,8 +606,10 @@ class _MessageListState extends State with PerAccountStoreAwareStat 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(data); @@ -688,12 +690,21 @@ class _MessageListState extends State with PerAccountStoreAwareStat ]); } + Widget _buildStartCap() { + // These assertions are invariants of [MessageListView]. + assert(!(model.fetchingOlder && model.fetchOlderCoolingDown)); + final effectiveFetchingOlder = + model.fetchingOlder || model.fetchOlderCoolingDown; + assert(!(model.haveOldest && effectiveFetchingOlder)); + return switch ((effectiveFetchingOlder, model.haveOldest)) { + (true, _) => const _MessageListLoadingMore(), + (_, true) => const _MessageListHistoryStart(), + (_, _) => const SizedBox.shrink(), + }; + } + Widget _buildItem(MessageListItem data) { switch (data) { - case MessageListHistoryStartItem(): - return const _MessageListHistoryStart(); - case MessageListLoadingItem(): - return const _MessageListLoadingMore(); case MessageListRecipientHeaderItem(): final header = RecipientHeader(message: data.message, narrow: widget.narrow); return StickyHeaderItem(allowOverflow: true, diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 539dbeaba7..bb85556ae9 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -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()>[ - (it) => it.isA(), (it) => it.isA(), (it) => it.isA().showSender.isTrue(), (it) => it.isA().showSender.isFalse(), @@ -1964,12 +1963,6 @@ void checkInvariants(MessageListView model) { } int i = 0; - if (model.haveOldest) { - check(model.items[i++]).isA(); - } - if (model.fetchingOlder || model.fetchOlderCoolingDown) { - check(model.items[i++]).isA(); - } for (int j = 0; j < model.messages.length; j++) { bool forcedShowSender = false; if (j == 0 @@ -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);