From cac4c5a3670ffe5eb38fa92bdf3fe324717836f1 Mon Sep 17 00:00:00 2001
From: Shu Chen <shu@zulip.com>
Date: Wed, 25 Oct 2023 13:51:14 +0100
Subject: [PATCH] msglist: Maintain _UnreadMarker animation state when
 MessageListView.items changes size

---
 lib/widgets/message_list.dart       | 25 +++++++++++++++
 test/widgets/message_list_test.dart | 47 +++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
index 25baadce68..ce26ad91bb 100644
--- a/lib/widgets/message_list.dart
+++ b/lib/widgets/message_list.dart
@@ -244,6 +244,30 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   Widget _buildListView(context) {
     final length = model!.items.length;
     return StickyHeaderListView.builder(
+      findChildIndexCallback: (Key key) {
+        // 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.
+        final valueKey = key as ValueKey;
+        final index = model!.findItemWithMessageId(valueKey.value);
+        if (index == -1) {
+          return null;
+        }
+        // Note `itemBuilder` pulls from `model.items` in reverse order.
+        return length - 1 - index;
+      },
       // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or
       //   similar) if that is ever offered:
       //     https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849
@@ -283,6 +307,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
               header: header, child: header);
           case MessageListMessageItem():
             return MessageItem(
+              key: ValueKey(data.message.id),
               trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11),
               item: data);
         }
diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart
index c54d3cb763..6ce0393e47 100644
--- a/test/widgets/message_list_test.dart
+++ b/test/widgets/message_list_test.dart
@@ -344,5 +344,52 @@ void main() {
 
       check(getAnimation(tester).value).equals(0);
     });
+
+    testWidgets('animation state persistence', (WidgetTester tester) async {
+      // Check that _UnreadMarker maintains its in-progress animation
+      // as the number of items change in MessageList.
+
+      // TODO: pull this out into group
+      Animation getAnimation(WidgetTester tester, int messageId) {
+        final widget = tester.widget<FadeTransition>(find.descendant(
+            of: find.byKey(ValueKey(messageId)),
+            matching: find.byType(FadeTransition)));
+        return widget.opacity;
+      }
+
+      final message = eg.streamMessage(id: 1, flags: []);
+      await setupMessageListPage(tester, messages: [message]);
+
+      // initial state: marker is visible
+      check(getAnimation(tester, message.id).value).equals(1.0);
+
+      store.handleEvent(UpdateMessageFlagsAddEvent(
+        id: 0,
+        flag: MessageFlag.read,
+        messages: [message.id],
+        all: false,
+      ));
+      await tester.pump(); // process handleEvent
+      await tester.pump(const Duration(milliseconds: 30)); // run animation partially
+
+      final newMessage = eg.streamMessage(id: 2, flags:[MessageFlag.read]);
+      store.handleEvent(MessageEvent(id: 0, message: newMessage));
+      await tester.pump(); // process handleEvent
+
+      // in-progress state: original marker is still
+      // animating but new marker is not
+      check(find.byType(MessageItem).evaluate()).length.equals(2);
+      check(getAnimation(tester, message.id).status)
+        .equals(AnimationStatus.forward);
+      check(getAnimation(tester, newMessage.id).status)
+        .equals(AnimationStatus.dismissed);
+
+      // If we introduce an animation bug (remove findChildIndexCallback
+      // on StickyHeaderListView.builder) this returns 1.
+      final frames = await tester.pumpAndSettle();
+      check(frames).isGreaterThan(1);
+      check(getAnimation(tester, message.id).status)
+        .equals(AnimationStatus.completed);
+    });
   });
 }