msglist: Split into back-to-back slivers

Thanks to all the preparatory changes we've made in this commit
series and those that came before it, this change should have only
subtle effects on user-visible behavior.

This therefore serves as a testing ground for all the ways that the
message list's scrolling behavior can become more complicated when
two (nontrivial) slivers are involved.  If we find a situation where
the behavior does change noticeably (beyond what's described below),
that will be something to investigate and fix.

In particular:

 * The sticky headers should behave exactly as they did before,
   even when the sliver boundary lies under the sticky header,
   thanks to several previous commit series.

 * On first loading a given message list, it should start out
   scrolled to the end, just as before.

 * When already scrolled to the end, the message list should stay
   there when a new message arrives, or a message is edited, etc.,
   even if the (new) latest message is taller than it was.
   This commit adds a test to confirm that.

Subtle differences include:

 * When scrolled up from the bottom and a new message comes in,
   the behavior is slightly different from before.  The current
   exact behavior is something we probably want to change anyway,
   and I think the new behavior isn't particularly better or worse.

 * The behavior upon overscroll might be slightly different;
   I'm not sure.

 * When a new message arrives, the previously-latest message gets a
   fresh element in the tree, and any UI state on it is lost.  This
   happens because it moves between one sliver-list and the other,
   and the `findChildIndexCallback` mechanism only works within a
   single sliver-list.

   This causes a couple of visible glitches, but they seem tolerable.
   This will also naturally get fixed in the next PR or two toward
   #82: we'll start adding new messages to the bottom sliver, rather
   than having them push anything from the bottom to the top sliver.

   Known symptoms of this are:

   * When the user has just marked messages as read and a new message
     arrives while the unread markers are fading, the marker on the
     previously-latest message will (if it was present) abruptly
     vanish while the others smoothly continue fading.

     We have a test for the smooth fading, and it happened to test
     the latest message, so this commit adjusts the test to avoid
     triggering this issue.

   * If the user had a spoiler expanded on the previously-latest
     message, it will reset to collapsed.

Author: gnprice

lib/widgets/message_list.dart

@@ -581,11 +581,18 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   }
 
   Widget _buildListView(BuildContext context) {
+    const bottomSize = 1;
     final length = model!.items.length;
+    final bottomLength = length <= bottomSize ? length : bottomSize;
+    final topLength = length - bottomLength;
     const centerSliverKey = ValueKey('center sliver');
     final zulipLocalizations = ZulipLocalizations.of(context);
 
-    Widget sliver = SliverStickyHeaderList(
+    // 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
+    final needSafeArea = !ComposeBox.hasComposeBox(widget.narrow);
+
+    final topSliver = SliverStickyHeaderList(
       headerPlacement: HeaderPlacement.scrollingStart,
       delegate: SliverChildBuilderDelegate(
         // To preserve state across rebuilds for individual [MessageItem]
@@ -607,26 +614,60 @@ 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);
+          final i = length - 1 - (index + bottomLength);
+          if (i < 0) return null;
+          return i;
+        },
+        childCount: topLength,
+        (context, i) {
+          final data = model!.items[length - 1 - (i + bottomLength)];
+          final item = _buildItem(zulipLocalizations, data);
+          return item;
+        }));
+
+    Widget bottomSliver = SliverStickyHeaderList(
+      key: needSafeArea ? null : centerSliverKey,
+      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 All Messages 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;
+          final i = index - topLength;
+          if (i < 0) return null;
+          return i;
         },
-        childCount: length + 3,
+        childCount: bottomLength + 3,
         (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);
+          if (i == bottomLength + 2) return const SizedBox(height: 36);
 
-          if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);
+          if (i == bottomLength + 1) return MarkAsReadWidget(narrow: widget.narrow);
 
-          if (i == 2) return TypingStatusWidget(narrow: widget.narrow);
+          if (i == bottomLength) return TypingStatusWidget(narrow: widget.narrow);
 
-          final data = model!.items[length - 1 - (i - 3)];
+          final data = model!.items[topLength + i];
           return _buildItem(zulipLocalizations, data);
         }));
 
-    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 (needSafeArea) {
+      bottomSliver = SliverSafeArea(key: centerSliverKey, sliver: bottomSliver);
     }
 
     return MessageListScrollView(
@@ -647,12 +688,8 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
       paintOrder: SliverPaintOrder.firstIsTop,
 
       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,
       ]);
   }
 

test/widgets/message_list_test.dart

@@ -453,6 +453,29 @@ void main() {
     });
   });
 
+  group('scroll position', () {
+    // The scrolling behavior is tested in more detail in the tests of
+    // [MessageListScrollView], in scrolling_test.dart .
+
+    testWidgets('sticks to end upon new message', (tester) async {
+      await setupMessageListPage(tester,
+        messages: List.generate(10, (_) => eg.streamMessage(content: '<p>a</p>')));
+      final controller = findMessageListScrollController(tester)!;
+
+      // Starts at end, and with room to scroll up.
+      check(controller.position.extentAfter).equals(0);
+      check(controller.position.extentBefore).isGreaterThan(0);
+      final oldPosition = controller.position.pixels;
+
+      // On new message, position remains at end…
+      await store.addMessage(eg.streamMessage(content: '<p>a</p><p>b</p>'));
+      await tester.pump();
+      check(controller.position.extentAfter).equals(0);
+      // … even though that means a bigger number now.
+      check(controller.position.pixels).isGreaterThan(oldPosition);
+    });
+  });
+
   group('ScrollToBottomButton interactions', () {
     bool isButtonVisible(WidgetTester tester) {
       return tester.any(find.descendant(
@@ -1490,8 +1513,15 @@ void main() {
       // as the number of items changes in MessageList. See
       // `findChildIndexCallback` passed into [SliverStickyHeaderList]
       // at [_MessageListState._buildListView].
+
+      // TODO(#82): Cut paddingMessage.  It's there to paper over a glitch:
+      //   the _UnreadMarker animation *does* get interrupted in the case where
+      //   the message gets pushed from one sliver to the other.  See:
+      //     https://github.com/zulip/zulip-flutter/pull/1436#issuecomment-2756738779
+      //   That case will no longer exist when #82 is complete.
       final message = eg.streamMessage(flags: []);
-      await setupMessageListPage(tester, messages: [message]);
+      final paddingMessage = eg.streamMessage();
+      await setupMessageListPage(tester, messages: [message, paddingMessage]);
       check(getAnimation(tester, message.id))
         ..value.equals(1.0)
         ..status.equals(AnimationStatus.dismissed);
@@ -1515,10 +1545,11 @@ void main() {
         ..status.equals(AnimationStatus.forward);
 
       // introduce new message
+      check(find.byType(MessageItem)).findsExactly(2);
       final newMessage = eg.streamMessage(flags:[MessageFlag.read]);
       await store.addMessage(newMessage);
       await tester.pump(); // process handleEvent
-      check(find.byType(MessageItem).evaluate()).length.equals(2);
+      check(find.byType(MessageItem)).findsExactly(3);
       check(getAnimation(tester, message.id))
         ..value.isGreaterThan(0.0)
         ..value.isLessThan(1.0)