msglist: Fetch newer messages despite previous muted batch

Author: sm-sayedi

lib/model/message_list.dart

@@ -145,6 +145,26 @@ mixin _MessageSequence {
   /// The corresponding item index is [middleItem].
   int middleMessage = 0;
 
+  /// The ID of the oldest known message so far in this narrow.
+  ///
+  /// This will be `null` if no messages of this narrow are fetched yet.
+  /// Having a non-null value for this doesn't always mean [haveOldest] is `true`.
+  ///
+  /// The related message may not appear in [messages] because it
+  /// is muted in one way or another.
+  int? get oldMessageId => _oldMessageId;
+  int? _oldMessageId;
+
+  /// The ID of the newest known message so far in this narrow.
+  ///
+  /// This will be `null` if no messages of this narrow are fetched yet.
+  /// Having a non-null value for this doesn't always mean [haveNewest] is `true`.
+  ///
+  /// The related message may not appear in [messages] because it
+  /// is muted in one way or another.
+  int? get newMessageId => _newMessageId;
+  int? _newMessageId;
+
   /// Whether [messages] and [items] represent the results of a fetch.
   ///
   /// This allows the UI to distinguish "still working on fetching messages"
@@ -409,6 +429,8 @@ mixin _MessageSequence {
     generation += 1;
     messages.clear();
     middleMessage = 0;
+    _oldMessageId = null;
+    _newMessageId = null;
     outboxMessages.clear();
     _haveOldest = false;
     _haveNewest = false;
@@ -775,6 +797,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   Future<void> fetchInitial() async {
     assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore);
     assert(messages.isEmpty && contents.isEmpty);
+    assert(oldMessageId == null && newMessageId == null);
 
     if (narrow case KeywordSearchNarrow(keyword: '')) {
       // The server would reject an empty keyword search; skip the request.
@@ -796,6 +819,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       numAfter: kMessageListFetchBatchSize,
       allowEmptyTopicName: true,
     );
+    _oldMessageId = result.messages.firstOrNull?.id;
+    _newMessageId = result.messages.lastOrNull?.id;
     if (this.generation > generation) return;
 
     _adjustNarrowForTopicPermalink(result.messages.firstOrNull);
@@ -869,25 +894,32 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   /// That makes this method suitable to call frequently, e.g. every frame,
   /// whenever it looks likely to be useful to have more messages.
   Future<void> fetchOlder() async {
-    if (haveOldest) return;
-    if (busyFetchingMore) return;
-    assert(fetched);
-    assert(messages.isNotEmpty);
-    await _fetchMore(
-      anchor: NumericAnchor(messages[0].id),
-      numBefore: kMessageListFetchBatchSize,
-      numAfter: 0,
-      processResult: (result) {
-        store.reconcileMessages(result.messages);
-        store.recentSenders.handleMessages(result.messages); // TODO(#824)
-
-        final fetchedMessages = _allMessagesVisible
-          ? result.messages // Avoid unnecessarily copying the list.
-          : result.messages.where(_messageVisible);
-
-        _insertAllMessages(0, fetchedMessages);
-        _haveOldest = result.foundOldest;
-      });
+    final generation = this.generation;
+    int visibleMessageCount = 0;
+    do {
+      if (haveOldest) return;
+      if (busyFetchingMore) return;
+      assert(fetched);
+      assert(oldMessageId != null);
+      await _fetchMore(
+        anchor: NumericAnchor(oldMessageId!),
+        numBefore: kMessageListFetchBatchSize,
+        numAfter: 0,
+        processResult: (result) {
+          _oldMessageId = result.messages.firstOrNull?.id ?? oldMessageId;
+          store.reconcileMessages(result.messages);
+          store.recentSenders.handleMessages(result.messages); // TODO(#824)
+
+          final fetchedMessages = _allMessagesVisible
+            ? result.messages // Avoid unnecessarily copying the list.
+            : result.messages.where(_messageVisible);
+
+          _insertAllMessages(0, fetchedMessages);
+          _haveOldest = result.foundOldest;
+          visibleMessageCount += fetchedMessages.length;
+        });
+    } while (visibleMessageCount < kMessageListFetchBatchSize / 2
+             && this.generation == generation);
   }
 
   /// Fetch the next batch of newer messages, if applicable.
@@ -899,29 +931,36 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   /// That makes this method suitable to call frequently, e.g. every frame,
   /// whenever it looks likely to be useful to have more messages.
   Future<void> fetchNewer() async {
-    if (haveNewest) return;
-    if (busyFetchingMore) return;
-    assert(fetched);
-    assert(messages.isNotEmpty);
-    await _fetchMore(
-      anchor: NumericAnchor(messages.last.id),
-      numBefore: 0,
-      numAfter: kMessageListFetchBatchSize,
-      processResult: (result) {
-        store.reconcileMessages(result.messages);
-        store.recentSenders.handleMessages(result.messages); // TODO(#824)
-
-        for (final message in result.messages) {
-          if (_messageVisible(message)) {
-            _addMessage(message);
+    final generation = this.generation;
+    int visibleMessageCount = 0;
+    do {
+      if (haveNewest) return;
+      if (busyFetchingMore) return;
+      assert(fetched);
+      assert(newMessageId != null);
+      await _fetchMore(
+        anchor: NumericAnchor(newMessageId!),
+        numBefore: 0,
+        numAfter: kMessageListFetchBatchSize,
+        processResult: (result) {
+          _newMessageId = result.messages.lastOrNull?.id ?? newMessageId;
+          store.reconcileMessages(result.messages);
+          store.recentSenders.handleMessages(result.messages); // TODO(#824)
+
+          for (final message in result.messages) {
+            if (_messageVisible(message)) {
+              _addMessage(message);
+              visibleMessageCount++;
+            }
           }
-        }
-        _haveNewest = result.foundNewest;
+          _haveNewest = result.foundNewest;
 
-        if (haveNewest) {
-          _syncOutboxMessagesFromStore();
-        }
-      });
+          if (haveNewest) {
+            _syncOutboxMessagesFromStore();
+          }
+        });
+    } while (visibleMessageCount < kMessageListFetchBatchSize / 2
+             && this.generation == generation);
   }
 
   Future<void> _fetchMore({

lib/widgets/message_list.dart

@@ -838,8 +838,6 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
     model.fetchInitial();
   }
 
-  bool _prevFetched = false;
-
   void _modelChanged() {
     // When you're scrolling quickly, our mark-as-read requests include the
     // messages *between* _messagesRecentlyInViewport and the messages currently
@@ -866,14 +864,13 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       // This method was called because that just changed.
     });
 
-    if (!_prevFetched && model.fetched && model.messages.isEmpty) {
+    if (model.messages.isEmpty && model.haveOldest && model.haveNewest) {
       // If the fetch came up empty, there's nothing to read,
       // so opening the keyboard won't be bothersome and could be helpful.
       // It's definitely helpful if we got here from the new-DM page.
       MessageListPage.ancestorOf(context)
         .composeBoxState?.controller.requestFocusIfUnfocused();
     }
-    _prevFetched = model.fetched;
   }
 
   /// Find the range of message IDs on screen, as a (first, last) tuple,
@@ -1222,7 +1219,8 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       return Column(crossAxisAlignment: CrossAxisAlignment.stretch, children: [
         TypingStatusWidget(narrow: widget.narrow),
         // TODO perhaps offer mark-as-read even when not done fetching?
-        MarkAsReadWidget(narrow: widget.narrow),
+        if (model.messages.isNotEmpty)
+          MarkAsReadWidget(narrow: widget.narrow),
         // To reinforce that the end of the feed has been reached:
         //   https://chat.zulip.org/#narrow/channel/48-mobile/topic/space.20at.20end.20of.20thread/near/2203391
         const SizedBox(height: 12),

test/model/message_list_test.dart

@@ -640,10 +640,10 @@ void main() {
     });
 
     test('nop during backoff', () => awaitFakeAsync((async) async {
-      final olderMessages = List.generate(5, (i) => eg.streamMessage());
-      final initialMessages = List.generate(5, (i) => eg.streamMessage());
-      final newerMessages = List.generate(5, (i) => eg.streamMessage());
-      await prepare(anchor: NumericAnchor(initialMessages[2].id));
+      final olderMessages = List.generate(50, (i) => eg.streamMessage());
+      final initialMessages = List.generate(50, (i) => eg.streamMessage());
+      final newerMessages = List.generate(50, (i) => eg.streamMessage());
+      await prepare(anchor: NumericAnchor(initialMessages[25].id));
       await prepareMessages(foundOldest: false, foundNewest: false,
         messages: initialMessages);
       check(connection.takeRequests()).single;
@@ -687,10 +687,10 @@ void main() {
     // TODO(#824): move this test
     test('fetchOlder recent senders track all the messages', () async {
       await prepare();
-      final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i));
+      final initialMessages = List.generate(50, (i) => eg.streamMessage(id: 100 + i));
       await prepareMessages(foundOldest: false, messages: initialMessages);
 
-      final oldMessages = List.generate(10, (i) => eg.streamMessage(id: 89 + i))
+      final oldMessages = List.generate(50, (i) => eg.streamMessage(id: 49 + i))
         // Not subscribed to the stream with id 10.
         ..add(eg.streamMessage(id: 99, stream: eg.stream(streamId: 10)));
       connection.prepare(json: olderResult(
@@ -699,28 +699,28 @@ void main() {
       ).toJson());
       await model.fetchOlder();
 
-      check(model).messages.length.equals(20);
+      check(model).messages.length.equals(100);
       recent_senders_test.checkMatchesMessages(store.recentSenders,
         [...initialMessages, ...oldMessages]);
     });
 
     // TODO(#824): move this test
     test('TODO fetchNewer recent senders track all the messages', () async {
       await prepare(anchor: NumericAnchor(100));
-      final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i));
+      final initialMessages = List.generate(50, (i) => eg.streamMessage(id: 100 + i));
       await prepareMessages(foundOldest: true, foundNewest: false,
         messages: initialMessages);
 
-      final newMessages = List.generate(10, (i) => eg.streamMessage(id: 110 + i))
+      final newMessages = List.generate(50, (i) => eg.streamMessage(id: 150 + i))
         // Not subscribed to the stream with id 10.
-        ..add(eg.streamMessage(id: 120, stream: eg.stream(streamId: 10)));
+        ..add(eg.streamMessage(id: 200, stream: eg.stream(streamId: 10)));
       connection.prepare(json: newerResult(
         anchor: 100, foundNewest: false,
         messages: newMessages,
       ).toJson());
       await model.fetchNewer();
 
-      check(model).messages.length.equals(20);
+      check(model).messages.length.equals(100);
       recent_senders_test.checkMatchesMessages(store.recentSenders,
         [...initialMessages, ...newMessages]);
     });
@@ -2822,11 +2822,19 @@ void main() {
     Message dmMessage(int id) =>
       eg.dmMessage(id: id, from: eg.selfUser, to: [], timestamp: timestamp);
 
+    List<Message> streamMessages({required int fromId, required int toId}) {
+      assert(fromId > 0 && fromId <= toId);
+      return [
+        for (int id = fromId; id <= toId; id++)
+          streamMessage(id)
+      ];
+    }
+
     // First, test fetchInitial, where some headers are needed and others not.
     await prepare(narrow: CombinedFeedNarrow());
     connection.prepare(json: newestResult(
       foundOldest: false,
-      messages: [streamMessage(10), streamMessage(11), dmMessage(12)],
+      messages: [streamMessage(200), streamMessage(201), dmMessage(202)],
     ).toJson());
     await model.fetchInitial();
     checkNotifiedOnce();
@@ -2835,7 +2843,7 @@ void main() {
     connection.prepare(json: olderResult(
       anchor: model.messages[0].id,
       foundOldest: false,
-      messages: [streamMessage(7), streamMessage(8), dmMessage(9)],
+      messages: [...streamMessages(fromId: 150, toId: 198), dmMessage(199)],
     ).toJson());
     await model.fetchOlder();
     checkNotified(count: 2);
@@ -2844,17 +2852,17 @@ void main() {
     connection.prepare(json: olderResult(
       anchor: model.messages[0].id,
       foundOldest: false,
-      messages: [streamMessage(6)],
+      messages: streamMessages(fromId: 100, toId: 149),
     ).toJson());
     await model.fetchOlder();
     checkNotified(count: 2);
 
     // Then test MessageEvent, where a new header is needed…
-    await store.addMessage(streamMessage(13));
+    await store.addMessage(streamMessage(203));
     checkNotifiedOnce();
 
     // … and where it's not.
-    await store.addMessage(streamMessage(14));
+    await store.addMessage(streamMessage(204));
     checkNotifiedOnce();
 
     // Then test UpdateMessageEvent edits, where a header is and remains needed…
@@ -2892,7 +2900,7 @@ void main() {
     connection.prepare(json: olderResult(
       anchor: model.messages[0].id,
       foundOldest: true,
-      messages: [streamMessage(5)],
+      messages: [streamMessage(99)],
     ).toJson());
     await model.fetchOlder();
     checkNotified(count: 2);
@@ -2904,16 +2912,16 @@ void main() {
     final outboxMessageIds = store.outboxMessages.keys.toList();
     // Then test removing the first outbox message…
     await store.handleEvent(eg.messageEvent(
-      dmMessage(15), localMessageId: outboxMessageIds.first));
+      dmMessage(205), localMessageId: outboxMessageIds.first));
     checkNotifiedOnce();
 
     // … and handling a new non-outbox message…
-    await store.handleEvent(eg.messageEvent(streamMessage(16)));
+    await store.handleEvent(eg.messageEvent(streamMessage(206)));
     checkNotifiedOnce();
 
     // … and removing the second outbox message.
     await store.handleEvent(eg.messageEvent(
-      dmMessage(17), localMessageId: outboxMessageIds.last));
+      dmMessage(207), localMessageId: outboxMessageIds.last));
     checkNotifiedOnce();
   }));