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 = 20; // TODO tune
 
 /// A message, or one of its siblings shown in the message list.
 ///
@@ -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].
@@ -269,6 +300,11 @@ mixin _MessageSequence {
     _fetchingOlder = false;
     _fetchOlderCoolingDown = false;
     _fetchOlderCooldownBackoffMachine = null;
+    _haveNewest = false;
+    _fetchingNewer = false;
+    _fetchNewerCoolingDown = false;
+    _fetchNewerCooldownBackoffMachine = null;
+    _firstUnreadMessageId = null;
     contents.clear();
     items.clear();
   }
@@ -500,15 +536,16 @@ 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,
+      numAfter: kMessageListFetchBatchSize,
     );
     if (this.generation > generation) return;
     store.reconcileMessages(result.messages);
@@ -520,6 +557,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
     _fetched = true;
     _haveOldest = result.foundOldest;
+    _haveNewest = result.foundNewest;
+    _firstUnreadMessageId = result.anchor;
     _updateEndMarkers();
     notifyListeners();
   }
@@ -541,7 +580,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 +592,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 +628,69 @@ 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;
+    // TODO handle markers
+    _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: true,
+          numBefore: 0,
+          numAfter: kMessageListFetchBatchSize,
+        );
+      } catch (e) {
+        hasFetchError = true;
+        rethrow;
+      }
+      if (this.generation > generation) return;
+
+      // Remove the anchor.
+      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

@@ -510,6 +510,9 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       //   still not yet updated to account for the newly-added messages.
       model?.fetchOlder();
     }
+    if (scrollMetrics.extentAfter != 0 && scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {
+      model?.fetchNewer();
+    }
   }
 
   void _scrollChanged() {
@@ -562,10 +565,17 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   }
 
   Widget _buildListView(BuildContext context) {
-    final length = model!.items.length;
     const centerSliverKey = ValueKey('center sliver');
 
-    Widget sliver = SliverStickyHeaderList(
+    // TODO fix borked header
+    // TODO handle firstUnread == newest
+    // TODO maybe handle firstUnread == oldest
+    final firstUnreadItemIndex = model!.findItemWithMessageId(model!.firstUnreadMessageId!);
+    final olderItems = model!.items.sublist(0, firstUnreadItemIndex + 1);
+    final newerItems = model!.items.sublist(firstUnreadItemIndex + 1);
+
+    // TODO dedup code
+    Widget olderItemsSliver = SliverStickyHeaderList(
       headerPlacement: HeaderPlacement.scrollingStart,
       delegate: SliverChildBuilderDelegate(
         // To preserve state across rebuilds for individual [MessageItem]
@@ -587,26 +597,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);
+        }));
+
+    Widget newerItemsSliver = SliverStickyHeaderList(
+      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 == 1) return MarkAsReadWidget(narrow: widget.narrow);
+          if (i == newerItems.length + 1) return MarkAsReadWidget(narrow: widget.narrow);
 
-          if (i == 2) return TypingStatusWidget(narrow: widget.narrow);
+          // 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 = model!.items[length - 1 - (i - 3)];
+          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 shouldn't be necessary
-      sliver = SliverSafeArea(sliver: sliver);
+      newerItemsSliver = SliverSafeArea(sliver: newerItemsSliver);
     }
 
     return CustomScrollView(
@@ -622,17 +662,13 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       },
 
       controller: scrollController,
-      semanticChildCount: length + 2,
+      semanticChildCount: model!.items.length + 2,
       anchor: 1.0,
       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.
+        olderItemsSliver,
         const SliverToBoxAdapter(key: centerSliverKey),
+        newerItemsSliver,
       ]);
   }
 

test/example_data.dart

@@ -477,6 +477,25 @@ GetMessagesResult newestGetMessagesResult({
   );
 }
 
+/// A GetMessagesResult the server might return on an `anchor=oldest` request.
+GetMessagesResult oldestGetMessagesResult({
+  required bool foundNewest,
+  bool historyLimited = false,
+  required List<Message> messages,
+}) {
+  return GetMessagesResult(
+    // These anchor, foundAnchor, and foundOldest values are what the server
+    // appears to always return when the request had `anchor=oldest`.
+    anchor: 0,
+    foundAnchor: false,
+    foundOldest: true,
+
+    foundNewest: foundNewest,
+    historyLimited: historyLimited,
+    messages: messages,
+  );
+}
+
 /// A GetMessagesResult the server might return when we request older messages.
 GetMessagesResult olderGetMessagesResult({
   required int anchor,
@@ -495,6 +514,24 @@ GetMessagesResult olderGetMessagesResult({
   );
 }
 
+/// A GetMessagesResult the server might return when we request newer messages.
+GetMessagesResult newerGetMessagesResult({
+  required int anchor,
+  bool foundAnchor = false, // the value if the server understood includeAnchor false
+  required bool foundNewest,
+  bool historyLimited = false,
+  required List<Message> messages,
+}) {
+  return GetMessagesResult(
+    anchor: anchor,
+    foundAnchor: foundAnchor,
+    foundNewest: foundNewest,
+    foundOldest: false, // empirically always this, even when anchor happens to be oldest
+    historyLimited: historyLimited,
+    messages: messages,
+  );
+}
+
 PollWidgetData pollWidgetData({
   required String question,
   required List<String> options,