wip

Author: rajveermalviya

lib/model/message_list.dart

@@ -14,7 +14,7 @@ import 'narrow.dart';
 import 'store.dart';
 
 /// The number of messages to fetch in each request.
-const kMessageListFetchBatchSize = 100; // TODO tune
+const kMessageListFetchBatchSize = 5; // TODO tune
 
 /// A message, or one of its siblings shown in the message list.
 ///
@@ -58,7 +58,7 @@ class MessageListLoadingItem extends MessageListItem {
   const MessageListLoadingItem(this.direction);
 }
 
-enum MessageListDirection { older }
+enum MessageListDirection { older, newer }
 
 /// Indicates we've reached the oldest message in the narrow.
 class MessageListHistoryStartItem extends MessageListItem {
@@ -85,9 +85,6 @@ mixin _MessageSequence {
   bool _fetched = false;
 
   /// Whether we know we have the oldest messages for this narrow.
-  ///
-  /// (Currently we always have the newest messages for the narrow,
-  /// once [fetched] is true, because we start from the newest.)
   bool get haveOldest => _haveOldest;
   bool _haveOldest = false;
 
@@ -118,6 +115,40 @@ mixin _MessageSequence {
 
   BackoffMachine? _fetchOlderCooldownBackoffMachine;
 
+  /// Whether we know we have the newest messages for this narrow.
+  bool get haveNewest => _haveNewest;
+  bool _haveNewest = false;
+
+  /// Whether we are currently fetching the next batch of newer messages.
+  ///
+  /// When this is true, [fetchNewer] is a no-op.
+  /// That method is called frequently by Flutter's scrolling logic,
+  /// and this field helps us avoid spamming the same request just to get
+  /// the same response each time.
+  ///
+  /// See also [fetchNewerCoolingDown].
+  bool get fetchingNewer => _fetchingNewer;
+  bool _fetchingNewer = false;
+
+  /// Whether [fetchNewer] had a request error recently.
+  ///
+  /// When this is true, [fetchNewer] is a no-op.
+  /// That method is called frequently by Flutter's scrolling logic,
+  /// and this field mitigates spamming the same request and getting
+  /// the same error each time.
+  ///
+  /// "Recently" is decided by a [BackoffMachine] that resets
+  /// when a [fetchNewer] request succeeds.
+  ///
+  /// See also [fetchingNewer].
+  bool get fetchNewerCoolingDown => _fetchNewerCoolingDown;
+  bool _fetchNewerCoolingDown = false;
+
+  BackoffMachine? _fetchNewerCooldownBackoffMachine;
+
+  int? get firstUnreadMessageId => _firstUnreadMessageId;
+  int? _firstUnreadMessageId;
+
   /// The parsed message contents, as a list parallel to [messages].
   ///
   /// The i'th element is the result of parsing the i'th element of [messages].
@@ -151,6 +182,7 @@ mixin _MessageSequence {
       case MessageListHistoryStartItem():        return -1;
       case MessageListLoadingItem():
         switch (item.direction) {
+          case MessageListDirection.newer:       return 1;
           case MessageListDirection.older:       return -1;
         }
       case MessageListRecipientHeaderItem(:var message):
@@ -269,6 +301,11 @@ mixin _MessageSequence {
     _fetchingOlder = false;
     _fetchOlderCoolingDown = false;
     _fetchOlderCooldownBackoffMachine = null;
+    _haveNewest = false;
+    _fetchingNewer = false;
+    _fetchNewerCoolingDown = false;
+    _fetchNewerCooldownBackoffMachine = null;
+    _firstUnreadMessageId = null;
     contents.clear();
     items.clear();
   }
@@ -317,7 +354,8 @@ mixin _MessageSequence {
   /// Update [items] to include markers at start and end as appropriate.
   void _updateEndMarkers() {
     assert(fetched);
-    assert(!(fetchingOlder && fetchOlderCoolingDown));
+    assert(!(fetchingOlder && fetchOlderCoolingDown)
+        || !(fetchingNewer && fetchNewerCoolingDown));
     final effectiveFetchingOlder = fetchingOlder || fetchOlderCoolingDown;
     assert(!(effectiveFetchingOlder && haveOldest));
     final startMarker = switch ((effectiveFetchingOlder, haveOldest)) {
@@ -336,6 +374,21 @@ mixin _MessageSequence {
       case (_,    true): items.removeFirst();
       case (_,    _   ): break;
     }
+
+    final effectiveFetchingNewer = fetchingNewer || fetchNewerCoolingDown;
+    final endMarker = switch (effectiveFetchingNewer) {
+      true => const MessageListLoadingItem(MessageListDirection.newer),
+      false => null,
+    };
+    final hasEndMarker = switch (items.lastOrNull) {
+      MessageListLoadingItem() => true,
+      _                        => false,
+    };
+    switch ((endMarker != null, hasEndMarker)) {
+      case (true,  false): items.add(endMarker!);
+      case (false, true ): items.removeLast();
+      case (_,     _    ): break;
+    }
   }
 
   /// Recompute [items] from scratch, based on [messages], [contents], and flags.
@@ -500,15 +553,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   Future<void> fetchInitial() async {
     // TODO(#80): fetch from anchor firstUnread, instead of newest
     // TODO(#82): fetch from a given message ID as anchor
-    assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown);
+    assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown
+                    && !haveNewest && !fetchingNewer && !fetchNewerCoolingDown);
     assert(messages.isEmpty && contents.isEmpty);
     // TODO schedule all this in another isolate
     final generation = this.generation;
     final result = await getMessages(store.connection,
       narrow: narrow.apiEncode(),
-      anchor: AnchorCode.newest,
+      anchor: AnchorCode.firstUnread,
       numBefore: kMessageListFetchBatchSize,
-      numAfter: 0,
+      // Results will include the anchor message, so fetch one less.
+      numAfter: kMessageListFetchBatchSize - 1,
     );
     if (this.generation > generation) return;
     store.reconcileMessages(result.messages);
@@ -520,6 +575,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
     _fetched = true;
     _haveOldest = result.foundOldest;
+    _haveNewest = result.foundNewest;
+    _firstUnreadMessageId = result.anchor;
     _updateEndMarkers();
     notifyListeners();
   }
@@ -541,7 +598,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       try {
         result = await getMessages(store.connection,
           narrow: narrow.apiEncode(),
-          anchor: NumericAnchor(messages[0].id),
+          anchor: NumericAnchor(messages.first.id),
           includeAnchor: false,
           numBefore: kMessageListFetchBatchSize,
           numAfter: 0,
@@ -553,7 +610,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       if (this.generation > generation) return;
 
       if (result.messages.isNotEmpty
-          && result.messages.last.id == messages[0].id) {
+          && result.messages.last.id == messages.first.id) {
         // TODO(server-6): includeAnchor should make this impossible
         result.messages.removeLast();
       }
@@ -589,6 +646,71 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
   }
 
+  /// Fetch the next batch of newer messages, if applicable.
+  Future<void> fetchNewer() async {
+    if (haveNewest) return;
+    if (fetchingNewer) return;
+    if (fetchNewerCoolingDown) return;
+    assert(fetched);
+    assert(messages.isNotEmpty);
+    _fetchingNewer = true;
+    _updateEndMarkers();
+    notifyListeners();
+    final generation = this.generation;
+    bool hasFetchError = false;
+    try {
+      final GetMessagesResult result;
+      try {
+        result = await getMessages(store.connection,
+          narrow: narrow.apiEncode(),
+          anchor: NumericAnchor(messages.last.id),
+          includeAnchor: false,
+          numBefore: 0,
+          numAfter: kMessageListFetchBatchSize,
+        );
+      } catch (e) {
+        hasFetchError = true;
+        rethrow;
+      }
+      if (this.generation > generation) return;
+
+      if (result.messages.isNotEmpty
+          && result.messages.first.id == messages.last.id) {
+        // TODO(server-6): includeAnchor should make this impossible
+        result.messages.removeAt(0);
+      }
+
+      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(messages.length, fetchedMessages);
+      _haveNewest = result.foundNewest;
+    } finally {
+      if (this.generation == generation) {
+        _fetchingNewer = false;
+        if (hasFetchError) {
+          assert(!fetchNewerCoolingDown);
+          _fetchNewerCoolingDown = true;
+          unawaited((_fetchNewerCooldownBackoffMachine ??= BackoffMachine())
+            .wait().then((_) {
+              if (this.generation != generation) return;
+              _fetchNewerCoolingDown = false;
+              _updateEndMarkers();
+              notifyListeners();
+            }));
+        } else {
+          _fetchNewerCooldownBackoffMachine = null;
+        }
+        _updateEndMarkers();
+        notifyListeners();
+      }
+    }
+  }
+
   void handleUserTopicEvent(UserTopicEvent event) {
     switch (_canAffectVisibility(event)) {
       case VisibilityEffect.none:

lib/widgets/message_list.dart

@@ -509,6 +509,9 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       //   still not yet updated to account for the newly-added messages.
       model?.fetchOlder();
     }
+    if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {
+      model?.fetchNewer();
+    }
   }
 
   void _scrollChanged() {
@@ -566,10 +569,26 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   }
 
   Widget _buildListView(BuildContext context) {
-    final length = model!.items.length;
+    const newestMessageId = 10000000000000000;
     const centerSliverKey = ValueKey('center sliver');
 
-    Widget sliver = SliverStickyHeaderList(
+    // TODO(#311) If we have a bottom nav, it will pad the bottom
+    //   inset, and this shouldn't be necessary
+    final hasComposeBox = ComposeBox.hasComposeBox(widget.narrow);
+
+    // TODO make sticky headers work
+    assert(model!.firstUnreadMessageId! != 0);
+    final List<MessageListItem> olderItems, newerItems;
+    if (model!.firstUnreadMessageId! == newestMessageId) {
+      olderItems = model!.items;
+      newerItems = const [];
+    } else {
+      final firstUnreadItemIndex = model!.findItemWithMessageId(model!.firstUnreadMessageId!);
+      olderItems = model!.items.slice(0, firstUnreadItemIndex);
+      newerItems = model!.items.slice(firstUnreadItemIndex);
+    }
+
+    Widget topSliver = SliverStickyHeaderList(
       headerPlacement: HeaderPlacement.scrollingStart,
       delegate: SliverChildBuilderDelegate(
         // To preserve state across rebuilds for individual [MessageItem]
@@ -591,26 +610,56 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
           final valueKey = key as ValueKey<int>;
           final index = model!.findItemWithMessageId(valueKey.value);
           if (index == -1) return null;
-          return length - 1 - (index - 3);
+          return olderItems.length - 1 - index;
         },
-        childCount: length + 3,
+        childCount: olderItems.length,
         (context, i) {
-          // To reinforce that the end of the feed has been reached:
-          //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
-          if (i == 0) return const SizedBox(height: 36);
+          final data = olderItems[olderItems.length - 1 - i];
+          return _buildItem(data, i);
+        }));
 
-          if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);
+    Widget bottomSliver = SliverStickyHeaderList(
+      key: hasComposeBox ? centerSliverKey : null,
+      headerPlacement: HeaderPlacement.scrollingStart,
+      delegate: SliverChildBuilderDelegate(
+        // To preserve state across rebuilds for individual [MessageItem]
+        // widgets as the size of [MessageListView.items] changes we need
+        // to match old widgets by their key to their new position in
+        // the list.
+        //
+        // The keys are of type [ValueKey] with a value of [Message.id]
+        // and here we use a O(log n) binary search method. This could
+        // be improved but for now it only triggers for materialized
+        // widgets. As a simple test, flinging through Combined feed in
+        // CZO on a Pixel 5, this only runs about 10 times per rebuild
+        // and the timing for each call is <100 microseconds.
+        //
+        // Non-message items (e.g., start and end markers) that do not
+        // have state that needs to be preserved have not been given keys
+        // and will not trigger this callback.
+        findChildIndexCallback: (Key key) {
+          final valueKey = key as ValueKey<int>;
+          final index = model!.findItemWithMessageId(valueKey.value);
+          if (index == -1) return null;
+          return index;
+        },
+        childCount: newerItems.length + 3,
+        (context, i) {
+          if (i == newerItems.length) return TypingStatusWidget(narrow: widget.narrow);
 
-          if (i == 2) return TypingStatusWidget(narrow: widget.narrow);
+          if (i == newerItems.length + 1) return MarkAsReadWidget(narrow: widget.narrow);
 
-          final data = model!.items[length - 1 - (i - 3)];
+          // To reinforce that the end of the feed has been reached:
+          //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
+          if (i == newerItems.length + 2) return const SizedBox(height: 36);
+
+          final data = newerItems[i];
           return _buildItem(data, i);
         }));
 
-    if (!ComposeBox.hasComposeBox(widget.narrow)) {
-      // TODO(#311) If we have a bottom nav, it will pad the bottom inset,
-      //   and this can be removed; also remove mention in MessageList dartdoc
-      sliver = SliverSafeArea(sliver: sliver);
+    if (!hasComposeBox) {
+      topSliver = SliverSafeArea(sliver: topSliver);
+      bottomSliver = SliverSafeArea(key: centerSliverKey, sliver: bottomSliver);
     }
 
     return CustomScrollView(
@@ -626,17 +675,12 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       },
 
       controller: scrollController,
-      semanticChildCount: length + 2,
-      anchor: 1.0,
+      semanticChildCount: model!.items.length + 2,
+      anchor: 0.75,
       center: centerSliverKey,
-
       slivers: [
-        sliver,
-
-        // This is a trivial placeholder that occupies no space.  Its purpose is
-        // to have the key that's passed to [ScrollView.center], and so to cause
-        // the above [SliverStickyHeaderList] to run from bottom to top.
-        const SliverToBoxAdapter(key: centerSliverKey),
+        topSliver,
+        bottomSliver,
       ]);
   }