message: Handle moved messages from UpdateMessageEvent.

We already handle the case where only a message's content is edited.
This handles the case where messages are moved too, between topics
and/or channels.

This introduces more notifyListener calls, and the listeners can be
notified more than once per UpdateMessage event. This is expected.

---

If the `generation += 1` line is commented out, the message list
has a race bug where a fetchOlder starts; we reset (because
messages were moved into the narrow); and then the fetch returns
and appends in the wrong spot. These races are detailed in the "fetch
races" test group.

---

StreamMessage.displayRecipient no longer carries the up-to-date display
name of the stream when the message has been moved to another stream. We
invalidate it by setting it to null.

The only time StreamMessage.displayRecipient is useful is when we
don't have data on that stream. This is the reason why we don't go ahead
and lookup the stream store when such a stream move happen, because the
stream store likely will not have that information if we ever need to
use displayRecipient as the fallback.

---

We have relatively more tests for the topic moves, because there are
more cases that make a difference to consider. There is some overlap
between the tests like "(channel, topic) -> (new channel, new topic)" and
other tests where only one of topic/channel is changed. We include both
because the parameterized test cases are easy to modify and it doesn't
hurt to cover another realistic scenario handled by the same code path.

Co-authored-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>

Author: gnprice

lib/api/model/model.dart

@@ -480,7 +480,7 @@ sealed class Message {
   final int senderId;
   final String senderRealmStr;
   @JsonKey(name: 'subject')
-  final String topic;
+  String topic;
   // final List<string> submessages; // TODO handle
   final int timestamp;
   String get type;
@@ -581,7 +581,7 @@ class StreamMessage extends Message {
   // invalidated.
   @JsonKey(required: true, disallowNullValue: true)
   String? displayRecipient;
-  final int streamId;
+  int streamId;
 
   StreamMessage({
     required super.client,

lib/model/message.dart

@@ -179,22 +179,43 @@ class MessageStoreImpl with MessageStore {
       return;
     }
 
-    if (newStreamId == null
-        && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!)) {
-      // The topic was only resolved/unresolved.
-      // No change to the messages' editState.
-      return;
-    }
+    final wasResolveOrUnresolve = (newStreamId == null
+      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!));
 
-    // TODO(#150): Handle message moves.  The views' recipient headers
-    //   may need updating, and consequently showSender too.
-    //   Currently only editState gets updated.
     for (final messageId in event.messageIds) {
       final message = messages[messageId];
       if (message == null) continue;
-      // Do not override the edited marker if the message has also been moved.
-      if (message.editState == MessageEditState.edited) continue;
-      message.editState = MessageEditState.moved;
+
+      if (message is! StreamMessage) {
+        assert(debugLog('Bad UpdateMessageEvent: stream/topic move on a DM')); // TODO(log)
+        continue;
+      }
+
+      if (newStreamId != null) {
+        message.streamId = newStreamId;
+        // See [StreamMessage.displayRecipient] on why the invalidation is
+        // needed.
+        message.displayRecipient = null;
+      }
+
+      if (newTopic != null) {
+        message.topic = newTopic;
+      }
+
+      if (!wasResolveOrUnresolve
+          && message.editState == MessageEditState.none) {
+        message.editState = MessageEditState.moved;
+      }
+    }
+
+    for (final view in _messageListViews) {
+      view.messagesMoved(
+        origStreamId: origStreamId,
+        newStreamId: newStreamId ?? origStreamId,
+        origTopic: origTopic,
+        newTopic: newTopic ?? origTopic,
+        messageIds: event.messageIds,
+      );
     }
   }
 

lib/model/message_list.dart

@@ -65,6 +65,9 @@ class MessageListHistoryStartItem extends MessageListItem {
 ///
 /// This comprises much of the guts of [MessageListView].
 mixin _MessageSequence {
+  /// A sequence number for invalidating stale fetches.
+  int generation = 0;
+
   /// The messages.
   ///
   /// See also [contents] and [items].
@@ -192,6 +195,17 @@ mixin _MessageSequence {
     _reprocessAll();
   }
 
+  /// Reset all [_MessageSequence] data, and cancel any active fetches.
+  void _reset() {
+    generation += 1;
+    messages.clear();
+    _fetched = false;
+    _haveOldest = false;
+    _fetchingOlder = false;
+    contents.clear();
+    items.clear();
+  }
+
   /// Redo all computations from scratch, based on [messages].
   void _recompute() {
     assert(contents.length == messages.length);
@@ -398,12 +412,14 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     assert(!fetched && !haveOldest && !fetchingOlder);
     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,
       numBefore: kMessageListFetchBatchSize,
       numAfter: 0,
     );
+    if (this.generation > generation) return;
     store.reconcileMessages(result.messages);
     store.recentSenders.handleMessages(result.messages); // TODO(#824)
     for (final message in result.messages) {
@@ -426,6 +442,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     _fetchingOlder = true;
     _updateEndMarkers();
     notifyListeners();
+    final generation = this.generation;
     try {
       final result = await getMessages(store.connection,
         narrow: narrow.apiEncode(),
@@ -434,6 +451,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
         numBefore: kMessageListFetchBatchSize,
         numAfter: 0,
       );
+      if (this.generation > generation) return;
 
       if (result.messages.isNotEmpty
           && result.messages.last.id == messages[0].id) {
@@ -451,9 +469,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       _insertAllMessages(0, fetchedMessages);
       _haveOldest = result.foundOldest;
     } finally {
-      _fetchingOlder = false;
-      _updateEndMarkers();
-      notifyListeners();
+      if (this.generation == generation) {
+        _fetchingOlder = false;
+        _updateEndMarkers();
+        notifyListeners();
+      }
     }
   }
 
@@ -489,6 +509,73 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
   }
 
+  void _messagesMovedInternally(List<int> messageIds) {
+    for (final messageId in messageIds) {
+      if (_findMessageWithId(messageId) != -1) {
+        _reprocessAll();
+        notifyListeners();
+        return;
+      }
+    }
+  }
+
+  void _messagesMovedIntoNarrow() {
+    // If there are some messages we don't have in [MessageStore], and they
+    // occur later than the messages we have here, then we just have to
+    // re-fetch from scratch.  That's always valid, so just do that always.
+    // TODO in cases where we do have data to do better, do better.
+    _reset();
+    notifyListeners();
+    fetchInitial();
+  }
+
+  void _messagesMovedFromNarrow(List<int> messageIds) {
+    if (_removeMessagesById(messageIds)) {
+      notifyListeners();
+    }
+  }
+
+  void messagesMoved({
+    required int origStreamId,
+    required int newStreamId,
+    required String origTopic,
+    required String newTopic,
+    required List<int> messageIds,
+  }) {
+    switch (narrow) {
+      case DmNarrow():
+        // DMs can't be moved (nor created by moves),
+        // so the messages weren't in this narrow and still aren't.
+        return;
+
+      case CombinedFeedNarrow():
+      case MentionsNarrow():
+        // The messages were and remain in this narrow.
+        // TODO(#421): … except they may have become muted or not.
+        //   We'll handle that at the same time as we handle muting itself changing.
+        // Recipient headers, and downstream of those, may change, though.
+        _messagesMovedInternally(messageIds);
+
+      case ChannelNarrow(:final streamId):
+        switch ((origStreamId == streamId, newStreamId == streamId)) {
+          case (false, false): return;
+          case (true,  true ): _messagesMovedInternally(messageIds);
+          case (false, true ): _messagesMovedIntoNarrow();
+          case (true,  false): _messagesMovedFromNarrow(messageIds);
+        }
+
+      case TopicNarrow(:final streamId, :final topic):
+        final oldMatch = (origStreamId == streamId && origTopic == topic);
+        final newMatch = (newStreamId == streamId && newTopic == topic);
+        switch ((oldMatch, newMatch)) {
+          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
+        }
+    }
+  }
+
   // Repeal the `@protected` annotation that applies on the base implementation,
   // so we can call this method from [MessageStoreImpl].
   @override