msglist: Update narrow when propagateMode is changeAll or changeLater.

Navigation change is only intended for TopicNarrow. For StreamNarrow, propagateMode is unused. See also: - zulip/zulip-mobile#5251 - #787 (comment) Fixes: #150 Signed-off-by: Zixuan James Li <[email protected]>
zulip · Aug 9, 2024 · e9b5f6f · e9b5f6f
1 parent 838d5a7
commit e9b5f6f
Show file tree

Hide file tree

Showing 7 changed files with 236 additions and 5 deletions.
diff --git a/lib/model/message.dart b/lib/model/message.dart
@@ -152,6 +152,7 @@ class MessageStoreImpl with MessageStore {
     final newStreamId = event.newStreamId; // null if topic-only move
     final origTopic = event.origTopic;
     final newTopic = event.newTopic;
+    final propagateMode = event.propagateMode;
 
     if (origTopic == null) {
       // There was no move.
@@ -178,6 +179,11 @@ class MessageStoreImpl with MessageStore {
       assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
       return;
     }
+    if (propagateMode == null) {
+      // The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
+      assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log)
+      return;
+    }
 
     final wasResolveOrUnresolve = (newStreamId == null
       && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!));
@@ -215,6 +221,7 @@ class MessageStoreImpl with MessageStore {
         origTopic: origTopic,
         newTopic: newTopic ?? origTopic,
         messageIds: event.messageIds,
+        propagateMode: propagateMode,
       );
     }
   }

diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart
@@ -359,7 +359,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   }
 
   final PerAccountStore store;
-  final Narrow narrow;
+  Narrow narrow;
 
   /// Whether [message] should actually appear in this message list,
   /// given that it does belong to the narrow.
@@ -535,12 +535,24 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
   }
 
+  void _handlePropagateMode(PropagateMode propagateMode, Narrow newNarrow) {
+    switch (propagateMode) {
+      case PropagateMode.changeAll:
+      case PropagateMode.changeLater:
+        narrow = newNarrow;
+        _reset();
+        fetchInitial();
+      case PropagateMode.changeOne:
+    }
+  }
+
   void messagesMoved({
     required int origStreamId,
     required int newStreamId,
     required String origTopic,
     required String newTopic,
     required List<int> messageIds,
+    required PropagateMode propagateMode,
   }) {
     switch (narrow) {
       case DmNarrow():
@@ -571,7 +583,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
           case (false, false): return;
           case (true,  true ): return; // TODO(log) no-op move
           case (false, true ): _messagesMovedIntoNarrow();
-          case (true,  false): _messagesMovedFromNarrow(messageIds); // TODO handle propagateMode
+          case (true,  false):
+            _messagesMovedFromNarrow(messageIds);
+            _handlePropagateMode(propagateMode, TopicNarrow(newStreamId, newTopic));
         }
     }
   }

diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
@@ -232,6 +232,12 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
     narrow = widget.initNarrow;
   }
 
+  void _narrowChanged(Narrow newNarrow) {
+    setState(() {
+      narrow = newNarrow;
+    });
+  }
+
   @override
   Widget build(BuildContext context) {
     final store = PerAccountStoreWidget.of(context);
@@ -289,7 +295,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
               removeBottom: ComposeBox.hasComposeBox(narrow),
 
               child: Expanded(
-                child: MessageList(narrow: narrow))),
+                child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))),
             ComposeBox(controllerKey: _composeBoxKey, narrow: narrow),
           ]))));
   }
@@ -368,9 +374,10 @@ const _kShortMessageHeight = 80;
 const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMessageHeight;
 
 class MessageList extends StatefulWidget {
-  const MessageList({super.key, required this.narrow});
+  const MessageList({super.key, required this.narrow, required this.onNarrowChanged});
 
   final Narrow narrow;
+  final void Function(Narrow newNarrow) onNarrowChanged;
 
   @override
   State<StatefulWidget> createState() => _MessageListState();
@@ -407,6 +414,11 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
   }
 
   void _modelChanged() {
+    if (model!.narrow != widget.narrow) {
+      // A message move event occurred, where propagate mode is
+      // [PropagateMode.changeAll] or [PropagateMode.changeLater].
+      widget.onNarrowChanged(model!.narrow);
+    }
     setState(() {
       // The actual state lives in the [MessageListView] model.
       // This method was called because that just changed.

diff --git a/test/example_data.dart b/test/example_data.dart
@@ -472,6 +472,7 @@ UpdateMessageEvent _updateMessageMoveEvent(
   String? origContent,
   String? newContent,
   required List<MessageFlag> flags,
+  PropagateMode propagateMode = PropagateMode.changeOne,
 }) {
   assert(newTopic != origTopic
          || (newStreamId != null && newStreamId != origStreamId));
@@ -486,7 +487,7 @@ UpdateMessageEvent _updateMessageMoveEvent(
     editTimestamp: 1234567890, // TODO generate timestamp
     origStreamId: origStreamId,
     newStreamId: newStreamId,
-    propagateMode: null,
+    propagateMode: propagateMode,
     origTopic: origTopic,
     newTopic: newTopic,
     origContent: origContent,
@@ -503,6 +504,7 @@ UpdateMessageEvent updateMessageEventMoveFrom({
   int? newStreamId,
   String? newTopic,
   String? newContent,
+  PropagateMode propagateMode = PropagateMode.changeOne,
 }) {
   assert(origMessages.isNotEmpty);
   final origMessage = origMessages.first;
@@ -516,6 +518,7 @@ UpdateMessageEvent updateMessageEventMoveFrom({
     origContent: origContent,
     newContent: newContent,
     flags: origMessage.flags,
+    propagateMode: propagateMode,
   );
 }
 
@@ -525,6 +528,7 @@ UpdateMessageEvent updateMessageEventMoveTo({
   int? origStreamId,
   String? origTopic,
   String? origContent,
+  PropagateMode propagateMode = PropagateMode.changeOne,
 }) {
   assert(newMessages.isNotEmpty);
   final newMessage = newMessages.first;
@@ -542,6 +546,7 @@ UpdateMessageEvent updateMessageEventMoveTo({
     origContent: origContent,
     newContent: newContent,
     flags: newMessage.flags,
+    propagateMode: propagateMode,
   );
 }
 

diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart
@@ -60,8 +60,14 @@ extension TextChecks on Subject<Text> {
   Subject<TextStyle?> get style => has((t) => t.style, 'style');
 }
 
+extension TextEditingControllerChecks on Subject<TextEditingController> {
+  Subject<String?> get text => has((t) => t.text, 'text');
+}
+
 extension TextFieldChecks on Subject<TextField> {
   Subject<TextCapitalization?> get textCapitalization => has((t) => t.textCapitalization, 'textCapitalization');
+  Subject<InputDecoration?> get decoration => has((t) => t.decoration, 'decoration');
+  Subject<TextEditingController?> get controller => has((t) => t.controller, 'controller');
 }
 
 extension TextStyleChecks on Subject<TextStyle> {
@@ -131,3 +137,7 @@ extension MaterialChecks on Subject<Material> {
   Subject<Color?> get color => has((x) => x.color, 'color');
   // TODO more
 }
+
+extension InputDecorationChecks on Subject<InputDecoration> {
+  Subject<String?> get hintText => has((x) => x.hintText, 'hintText');
+}
diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart
@@ -587,6 +587,37 @@ void main() {
         checkHasMessages(initialMessages);
         checkNotNotified();
       });
+
+      void testMessageMove(PropagateMode propagateMode) => awaitFakeAsync((async) async {
+        await prepareNarrow(narrow, initialMessages + movedMessages);
+        connection.prepare(delay: const Duration(seconds: 1), json: newestResult(
+          foundOldest: false,
+          messages: movedMessages,
+        ).toJson());
+        await store.handleEvent(eg.updateMessageEventMoveFrom(
+          origMessages: movedMessages,
+          newTopic: 'new',
+          newStreamId: otherStream.streamId,
+          propagateMode: propagateMode,
+        ));
+        checkNotifiedOnce();
+        async.elapse(const Duration(seconds: 1));
+        checkHasMessages(initialMessages);
+        check(model).narrow.equals(ChannelNarrow(stream.streamId));
+        checkNotNotified();
+      });
+
+      test('do not follow when propagateMode = changeOne', () {
+        testMessageMove(PropagateMode.changeOne);
+      });
+
+      test('do not follow when propagateMode = changeLater', () {
+        testMessageMove(PropagateMode.changeLater);
+      });
+
+      test('do not follow when propagateMode = changeAll', () {
+        testMessageMove(PropagateMode.changeAll);
+      });
     });
 
     group('in topic narrow', () {
@@ -674,6 +705,68 @@ void main() {
           checkNotNotified();
         }));
       });
+
+      void handleMoveEvent(PropagateMode propagateMode) => awaitFakeAsync((async) async {
+        await prepareNarrow(narrow, initialMessages + movedMessages);
+        connection.prepare(delay: const Duration(seconds: 1), json: newestResult(
+          foundOldest: false,
+          messages: movedMessages,
+        ).toJson());
+        await store.handleEvent(eg.updateMessageEventMoveFrom(
+          origMessages: movedMessages,
+          newTopic: 'new',
+          newStreamId: otherStream.streamId,
+          propagateMode: propagateMode,
+        ));
+        checkNotifiedOnce();
+        async.elapse(const Duration(seconds: 1));
+      });
+
+      test('do not follow to the new narrow when propagateMode = changeOne', () {
+        handleMoveEvent(PropagateMode.changeOne);
+        checkNotNotified();
+        checkHasMessages(initialMessages);
+        check(model).narrow.equals(TopicNarrow(stream.streamId, 'topic'));
+      });
+
+      test('follow to the new narrow when propagateMode = changeLater', () {
+        handleMoveEvent(PropagateMode.changeLater);
+        checkNotifiedOnce();
+        checkHasMessages(movedMessages);
+        check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new'));
+      });
+
+      test('follow to the new narrow when propagateMode = changeAll', () {
+        handleMoveEvent(PropagateMode.changeAll);
+        checkNotifiedOnce();
+        checkHasMessages(movedMessages);
+        check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new'));
+      });
+
+      test('handle move event before initial fetch', () => awaitFakeAsync((async) async {
+        await prepare(narrow: narrow);
+        final subscription = eg.subscription(stream);
+        await store.addStream(stream);
+        await store.addSubscription(subscription);
+        final followedMessage = eg.streamMessage(stream: stream, topic: 'new');
+
+        connection.prepare(delay: const Duration(seconds: 2), json: newestResult(
+          foundOldest: true,
+          messages: [followedMessage],
+        ).toJson());
+
+        check(model).fetched.isFalse();
+        checkHasMessages([]);
+        await store.handleEvent(eg.updateMessageEventMoveTo(
+          origTopic: 'topic',
+          newMessages: [followedMessage],
+          propagateMode: PropagateMode.changeAll,
+        ));
+        check(model).narrow.equals(TopicNarrow(stream.streamId, 'new'));
+
+        async.elapse(const Duration(seconds: 2));
+        checkHasMessages([followedMessage]);
+      }));
     });
 
     group('fetch races', () {