msglist: Fetch more messages despite previous muted batch

Fixes: zulip#1256

Author: sm-sayedi

lib/model/message_list.dart

@@ -145,6 +145,32 @@ mixin _MessageSequence {
   /// The corresponding item index is [middleItem].
   int middleMessage = 0;
 
+  /// The ID of the oldest known message so far in this narrow.
+  ///
+  /// This is used as the anchor for fetching the next batch of older messages
+  /// and will be `null` if no messages of this narrow have been fetched yet.
+  /// 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. Also, the related message may not stay
+  /// in the narrow for reasons such as message moves or deletions,
+  /// but that's fine for what this is used for.
+  int? get oldMessageId => _oldMessageId;
+  int? _oldMessageId;
+
+  /// The ID of the newest known message so far in this narrow.
+  ///
+  /// This is used as the anchor for fetching the next batch of newer messages
+  /// and will be `null` if no messages of this narrow have been fetched yet.
+  /// 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. Also, the related message may not stay
+  /// in the narrow for reasons such as message moves or deletions,
+  /// but that's fine for what this is used for.
+  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 +435,8 @@ mixin _MessageSequence {
     generation += 1;
     messages.clear();
     middleMessage = 0;
+    _oldMessageId = null;
+    _newMessageId = null;
     outboxMessages.clear();
     _haveOldest = false;
     _haveNewest = false;
@@ -815,9 +843,13 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   }
 
   /// Fetch messages, starting from scratch.
+  ///
+  /// This makes sure there is at least one non-muted message fetched; if any.
+  /// It may do so my repeatedly calling [fetchOlder] and [fetchNewer].
   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.
@@ -841,6 +873,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     );
     if (this.generation > generation) return;
 
+    _oldMessageId = result.messages.firstOrNull?.id;
+    _newMessageId = result.messages.lastOrNull?.id;
+
     _adjustNarrowForTopicPermalink(result.messages.firstOrNull);
 
     store.reconcileMessages(result.messages);
@@ -868,6 +903,12 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       _syncOutboxMessagesFromStore();
     }
 
+    while (messages.isEmpty && !(haveOldest && haveNewest)) {
+      await fetchOlder(partOfInitialFetch: true);
+      if (messages.isNotEmpty) break;
+      await fetchNewer(partOfInitialFetch: true);
+    }
+
     _setStatus(FetchingStatus.idle, was: FetchingStatus.fetchInitial);
   }
 
@@ -911,16 +952,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   /// then this method does nothing and immediately returns.
   /// 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 {
+  Future<void> fetchOlder({bool partOfInitialFetch = false}) async {
     if (haveOldest) return;
     if (busyFetchingMore) return;
-    assert(fetched);
-    assert(messages.isNotEmpty);
+    assert(partOfInitialFetch || fetched);
+    assert(oldMessageId != null);
     await _fetchMore(
-      anchor: NumericAnchor(messages[0].id),
+      anchor: NumericAnchor(oldMessageId!),
       numBefore: kMessageListFetchBatchSize,
       numAfter: 0,
+      partOfInitialFetch: partOfInitialFetch,
       processResult: (result) {
+        _oldMessageId = result.messages.firstOrNull?.id ?? oldMessageId;
         store.reconcileMessages(result.messages);
         store.recentSenders.handleMessages(result.messages); // TODO(#824)
 
@@ -941,16 +984,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   /// then this method does nothing and immediately returns.
   /// 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 {
+  Future<void> fetchNewer({bool partOfInitialFetch = false}) async {
     if (haveNewest) return;
     if (busyFetchingMore) return;
-    assert(fetched);
-    assert(messages.isNotEmpty);
+    assert(partOfInitialFetch || fetched);
+    assert(newMessageId != null);
     await _fetchMore(
-      anchor: NumericAnchor(messages.last.id),
+      anchor: NumericAnchor(newMessageId!),
       numBefore: 0,
       numAfter: kMessageListFetchBatchSize,
+      partOfInitialFetch: partOfInitialFetch,
       processResult: (result) {
+        _newMessageId = result.messages.lastOrNull?.id ?? newMessageId;
         store.reconcileMessages(result.messages);
         store.recentSenders.handleMessages(result.messages); // TODO(#824)
 
@@ -971,12 +1016,15 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     required Anchor anchor,
     required int numBefore,
     required int numAfter,
+    required bool partOfInitialFetch,
     required void Function(GetMessagesResult) processResult,
   }) async {
     assert(narrow is! TopicNarrow
       // We only intend to send "with" in [fetchInitial]; see there.
       || (narrow as TopicNarrow).with_ == null);
-    _setStatus(FetchingStatus.fetchingMore, was: FetchingStatus.idle);
+    if (!partOfInitialFetch) {
+      _setStatus(FetchingStatus.fetchingMore, was: FetchingStatus.idle);
+    }
     final generation = this.generation;
     bool hasFetchError = false;
     try {
@@ -998,7 +1046,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
 
       processResult(result);
     } finally {
-      if (this.generation == generation) {
+      if (this.generation == generation && !partOfInitialFetch) {
         if (hasFetchError) {
           _setStatus(FetchingStatus.backoff, was: FetchingStatus.fetchingMore);
           unawaited((_fetchBackoffMachine ??= BackoffMachine())

lib/widgets/message_list.dart

@@ -874,8 +874,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
@@ -902,14 +900,25 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       // This method was called because that just changed.
     });
 
-    if (!_prevFetched && model.fetched && model.messages.isEmpty) {
+    if (scrollController.hasClients && model.fetched) {
+      // This is needed here in order to fetch more messages if the previous
+      // batch is all muted, or its visible (non-muted) messages combined with
+      // the previous messages are less than a screenful. In such cases, there
+      // will be no change in the scroll metrics to fire a notification that
+      // will fetch more messages.
+      // Calling this method in here (inside `_modelChanged`) causes an
+      // apparently inevitable double-fetch glitch which is described inside
+      // the method itself.
+      _fetchMoreIfNeeded(scrollController.position);
+    }
+
+    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,
@@ -1032,26 +1041,20 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       _scrollToBottomVisible.value = true;
     }
 
+    _fetchMoreIfNeeded(scrollMetrics);
+  }
+
+  void _fetchMoreIfNeeded(ScrollMetrics scrollMetrics) {
     if (scrollMetrics.extentBefore < kFetchMessagesBufferPixels) {
-      // This ends up firing a second time shortly after we fetch a batch while
-      // there is a fling going on.
+      // This ends up firing a second time shortly after we fetch a batch.
       // The result is that each time we decide to fetch a batch, we end up
       // fetching two batches in quick succession. This is basically harmless
       // but makes things a bit more complicated to reason about.
       // The cause is that this gets called again with minScrollExtent
       // still not yet updated to account for the newly-added messages.
-      // This relates to how [SchedulerBinding] executes different tasks when
-      // producing a new frame, like first executing transient callbacks
-      // (typically ticking animations) followed by persistent callbacks
-      // (typically the build/layout/paint pipeline), and so on.
-      // So when there is a new message batch received, the related widgets are
-      // marked dirty for the next frame. With the ongoing fling, the underlying
-      // animation registers transient callback(s) for [ScrollPosition.setPixels]
-      // to be executed in the transient callbacks phase at the start of
-      // the frame. It will then notify its listeners, eventually calling
-      // `_scrollChanged` and in turn current method with old minScrollExtent,
-      // causing the second batch fetch. Then in the persistent callbacks phase,
-      // minScrollExtent will be updated, effective in the next frame.
+      // This relates to `_modelChanged` (and in turn the current method) being
+      // called right away as a listener of the model, before the next frame
+      // being drawn with the newly-added messages, updating minScrollExtent.
       model.fetchOlder();
     }
     if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {

test/model/message_list_test.dart

@@ -727,6 +727,132 @@ void main() {
     });
   });
 
+  group('oldMessageId, newMessageId', () {
+    group('fetchInitial', () {
+      test('visible messages', () async {
+        await prepare();
+        check(model)..oldMessageId.isNull()..newMessageId.isNull();
+
+        connection.prepare(json: newestResult(
+          foundOldest: true,
+          messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)),
+        ).toJson());
+        await model.fetchInitial();
+
+        checkNotifiedOnce();
+        check(model)
+          ..messages.length.equals(100)
+          ..oldMessageId.equals(100)..newMessageId.equals(199);
+      });
+
+      test('invisible messages', () async {
+        final mutedUser = eg.user();
+        await prepare(users: [mutedUser], mutedUserIds: [mutedUser.userId]);
+        check(model)..oldMessageId.isNull()..newMessageId.isNull();
+
+        connection.prepare(json: newestResult(
+          foundOldest: true,
+          messages: List.generate(100,
+            (i) => eg.dmMessage(id: 100 + i, from: eg.selfUser, to: [mutedUser])),
+        ).toJson());
+        await model.fetchInitial();
+
+        checkNotifiedOnce();
+        check(model)
+          ..messages.isEmpty()
+          ..oldMessageId.equals(100)..newMessageId.equals(199);
+      });
+
+      test('no messages found', () async {
+        await prepare();
+        check(model)..oldMessageId.isNull()..newMessageId.isNull();
+
+        connection.prepare(json: newestResult(
+          foundOldest: true,
+          messages: [],
+        ).toJson());
+        await model.fetchInitial();
+
+        checkNotifiedOnce();
+        check(model)
+          ..messages.isEmpty()
+          ..oldMessageId.isNull()..newMessageId.isNull();
+      });
+    });
+
+    group('fetching more', () {
+      test('visible messages', () async {
+        await prepare(anchor: AnchorCode.firstUnread);
+        check(model)..oldMessageId.isNull()..newMessageId.isNull();
+
+        await prepareMessages(
+          foundOldest: false, foundNewest: false,
+          anchorMessageId: 250,
+          messages: List.generate(100, (i) => eg.streamMessage(id: 200 + i)));
+        check(model)
+          ..messages.length.equals(100)
+          ..oldMessageId.equals(200)..newMessageId.equals(299);
+
+        connection.prepare(json: olderResult(
+          anchor: 200, foundOldest: true,
+          messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)),
+        ).toJson());
+        await model.fetchOlder();
+        checkNotified(count: 2);
+        check(model)
+          ..messages.length.equals(200)
+          ..oldMessageId.equals(100)..newMessageId.equals(299);
+
+        connection.prepare(json: newerResult(
+          anchor: 299, foundNewest: true,
+          messages: List.generate(100, (i) => eg.streamMessage(id: 300 + i)),
+        ).toJson());
+        await model.fetchNewer();
+        checkNotified(count: 2);
+        check(model)
+          ..messages.length.equals(300)
+          ..oldMessageId.equals(100)..newMessageId.equals(399);
+      });
+
+      test('invisible messages', () async {
+        final mutedUser = eg.user();
+        await prepare(anchor: AnchorCode.firstUnread,
+          users: [mutedUser], mutedUserIds: [mutedUser.userId]);
+        check(model)..oldMessageId.isNull()..newMessageId.isNull();
+
+        await prepareMessages(
+          foundOldest: false, foundNewest: false,
+          anchorMessageId: 250,
+          messages: List.generate(100, (i) => eg.streamMessage(id: 200 + i)));
+        check(model)
+          ..messages.length.equals(100)
+          ..oldMessageId.equals(200)..newMessageId.equals(299);
+
+        connection.prepare(json: olderResult(
+          anchor: 200, foundOldest: true,
+          messages: List.generate(100,
+            (i) => eg.dmMessage(id: 100 + i, from: eg.selfUser, to: [mutedUser])),
+        ).toJson());
+        await model.fetchOlder();
+        checkNotified(count: 2);
+        check(model)
+          ..messages.length.equals(100)
+          ..oldMessageId.equals(100)..newMessageId.equals(299);
+
+        connection.prepare(json: newerResult(
+          anchor: 299, foundNewest: true,
+          messages: List.generate(100,
+            (i) => eg.dmMessage(id: 300 + i, from: eg.selfUser, to: [mutedUser])),
+        ).toJson());
+        await model.fetchNewer();
+        checkNotified(count: 2);
+        check(model)
+          ..messages.length.equals(100)
+          ..oldMessageId.equals(100)..newMessageId.equals(399);
+      });
+    });
+  });
+
   // TODO(#1569): test jumpToEnd
 
   group('MessageEvent', () {
@@ -3226,6 +3352,8 @@ void checkInvariants(MessageListView model) {
     check(model)
       ..messages.isEmpty()
       ..outboxMessages.isEmpty()
+      ..oldMessageId.isNull()
+      ..newMessageId.isNull()
       ..haveOldest.isFalse()
       ..haveNewest.isFalse()
       ..busyFetchingMore.isFalse();
@@ -3414,6 +3542,8 @@ extension MessageListViewChecks on Subject<MessageListView> {
   Subject<List<MessageListItem>> get items => has((x) => x.items, 'items');
   Subject<int> get middleItem => has((x) => x.middleItem, 'middleItem');
   Subject<bool> get fetched => has((x) => x.fetched, 'fetched');
+  Subject<int?> get oldMessageId => has((x) => x.oldMessageId, 'oldMessageId');
+  Subject<int?> get newMessageId => has((x) => x.newMessageId, 'newMessageId');
   Subject<bool> get haveOldest => has((x) => x.haveOldest, 'haveOldest');
   Subject<bool> get haveNewest => has((x) => x.haveNewest, 'haveNewest');
   Subject<bool> get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore');