msglist: Fetch more messages despite previous muted batch

Fixes: #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;
@@ -818,6 +846,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.
@@ -841,6 +870,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 +900,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 +949,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 +981,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 +1013,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 +1043,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,17 @@ 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) {
+      _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 +1033,21 @@ 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 where minScrollExtent
+      // is be updated.
       model.fetchOlder();
     }
     if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {