msglist: Handle updated events in MessageListView (zulip#118).

oxling · oxling · commit c463d5d8060f · 2023-07-28T17:06:33.000-04:00
Processes an UpdateMessageEvent and hands it off to the MessageListView to update, if the message is visible in the MessageListView. This completes the changes required for issue zulip#118.
diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart
@@ -250,13 +250,13 @@ class Subscription {
 sealed class Message {
   final String? avatarUrl;
   final String client;
-  final String content;
+  String content;
   final String contentType;
 
   // final List<MessageEditHistory> editHistory; // TODO handle
   final int id;
-  final bool isMeMessage;
-  final int? lastEditTimestamp;
+  bool isMeMessage;
+  int? lastEditTimestamp;
 
   // final List<Reaction> reactions; // TODO handle
   final int recipientId;
@@ -271,7 +271,7 @@ sealed class Message {
 
   // final List<TopicLink> topicLinks; // TODO handle
   // final string type; // handled by runtime type of object
-  final List<String> flags; // TODO enum
+  List<String> flags; // TODO enum
   final String? matchContent;
   final String? matchSubject;
 
diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart
@@ -1,5 +1,7 @@
+import 'package:collection/collection.dart';
 import 'package:flutter/foundation.dart';
 
+import '../api/model/events.dart';
 import '../api/model/model.dart';
 import '../api/route/messages.dart';
 import 'content.dart';
@@ -86,6 +88,75 @@ class MessageListView extends ChangeNotifier {
     notifyListeners();
   }
 
+  _applyChangesToMessage(UpdateMessageEvent event, Message message) {
+    // In earlier server versions, omitting the userId indicates that this is a
+    // rendering-only update. That means this change was initiated by the server,
+    // not the user.
+
+    // TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
+    final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
+
+    if (event.editTimestamp != null && !isRenderingOnly) {
+      // Only update the timestamp if this was a user-led update,
+      // not a server-only update
+      message.lastEditTimestamp = event.editTimestamp;
+    }
+
+    message.flags = event.flags;
+
+    if (event.renderedContent != null) {
+      assert(message.contentType == 'text/html', "Expected message to have contentType 'text/html'. Instead, got ${message.contentType}");
+      message.content = event.renderedContent!;
+    }
+
+    if (event.isMeMessage != null) {
+      message.isMeMessage = event.isMeMessage!;
+    }
+
+  }
+
+  // This is almost directly copied from package:collection/src/algorithms.dart
+  // The way that package was set up doesn't allow us to search
+  // for a message ID among a bunch of message objects - this is a quick
+  // modification of that method to work here for us.
+  @visibleForTesting
+  int findMessageWithId(int messageId) {
+    var min = 0;
+    var max = messages.length;
+    while (min < max) {
+      var mid = min + ((max - min) >> 1);
+      final message = messages[mid];
+      var comp = message.id.compareTo(messageId);
+      if (comp == 0) return mid;
+      if (comp < 0) {
+        min = mid + 1;
+      } else {
+        max = mid;
+      }
+    }
+    return -1;
+  }
+
+  /// Update the message the given event applies to, if present in this view.
+  ///
+  /// This method only handles the case where the message's contents
+  /// were changed, and ignores any changes to its stream or topic.
+  ///
+  /// TODO(#150): Handle message moves.
+  void maybeUpdateMessage(UpdateMessageEvent event) {
+    final idx = findMessageWithId(event.messageId);
+
+    if (idx == -1)  {
+      return;
+    }
+
+    final message = messages[idx];
+    _applyChangesToMessage(event, message);
+
+    contents[idx] = parseContent(message.content);
+    notifyListeners();
+  }
+
   /// Called when the app is reassembled during debugging, e.g. for hot reload.
   ///
   /// This will redo from scratch any computations we can, such as parsing
diff --git a/lib/model/store.dart b/lib/model/store.dart
@@ -272,7 +272,9 @@ class PerAccountStore extends ChangeNotifier {
       }
     } else if (event is UpdateMessageEvent) {
       assert(debugLog("server event: update_message ${event.messageId}"));
-      // TODO handle
+      for (final view in _messageListViews) {
+        view.maybeUpdateMessage(event);
+      }
     } else if (event is DeleteMessageEvent) {
       assert(debugLog("server event: delete_message ${event.messageIds}"));
       // TODO handle
diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart
@@ -8,6 +8,9 @@ extension MessageChecks on Subject<Message> {
     toJson.deepEquals(expected.toJson());
   }
 
+  Subject<String> get content => has((e) => e.content, 'content');
+  Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
+  Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');
   Subject<List<String>> get flags => has((e) => e.flags, 'flags');
 
   // TODO accessors for other fields
diff --git a/test/example_data.dart b/test/example_data.dart
@@ -127,6 +127,7 @@ StreamMessage streamMessage({
   String? topic,
   String? content,
   String? contentMarkdown,
+  List<String>? flags,
 }) {
   final effectiveStream = stream ?? _stream();
   // The use of JSON here is convenient in order to delegate parts of the data
@@ -140,7 +141,7 @@ StreamMessage streamMessage({
     ..._messagePropertiesFromContent(content, contentMarkdown),
     'display_recipient': effectiveStream.name,
     'stream_id': effectiveStream.streamId,
-    'flags': [],
+    'flags': flags ?? [],
     'id': id ?? 1234567, // TODO generate example IDs
     'subject': topic ?? 'example topic',
     'timestamp': 1678139636,
@@ -158,6 +159,7 @@ DmMessage dmMessage({
   required List<User> to,
   String? content,
   String? contentMarkdown,
+  List<String>? flags,
 }) {
   assert(!to.any((user) => user.userId == from.userId));
   return DmMessage.fromJson({
@@ -168,7 +170,7 @@ DmMessage dmMessage({
       .map((u) => {'id': u.userId, 'email': u.email, 'full_name': u.fullName})
       .toList(growable: false),
 
-    'flags': [],
+    'flags': flags ?? [],
     'id': id ?? 1234567, // TODO generate example IDs
     'subject': '',
     'timestamp': 1678139636,
diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart
@@ -0,0 +1,224 @@
+import 'package:checks/checks.dart';
+import 'package:test/scaffolding.dart';
+import 'package:zulip/api/model/events.dart';
+import 'package:zulip/api/model/model.dart';
+import 'package:zulip/api/route/messages.dart';
+import 'package:zulip/model/message_list.dart';
+import 'package:zulip/model/narrow.dart';
+import 'package:zulip/model/store.dart';
+
+import '../api/fake_api.dart';
+import '../api/model/model_checks.dart';
+import '../model/binding.dart';
+import '../model/test_store.dart';
+import '../example_data.dart' as eg;
+
+const int userId = 1;
+
+Future<PerAccountStore> setupStore(ZulipStream stream) async {
+  addTearDown(TestZulipBinding.instance.reset);
+
+  await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot());
+
+  final store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id);
+  store.addUser(eg.user(userId: userId));
+  store.addStream(stream);
+
+  return store;
+}
+
+Future<MessageListView> messageListViewWithMessages(List<Message> messages, PerAccountStore store, Narrow narrow) async {
+  final messageList = MessageListView.init(store: store, narrow: narrow);
+
+  final connection = store.connection as FakeApiConnection;
+
+  connection.prepare(json: GetMessagesResult(
+    anchor: messages.first.id,
+    foundNewest: true,
+    foundOldest: true,
+    foundAnchor: true,
+    historyLimited: false,
+    messages: messages,
+  ).toJson());
+
+  await messageList.fetch();
+
+  check(messageList.messages.length).equals(messages.length);
+
+  return messageList;
+}
+
+void main() async {
+  TestZulipBinding.ensureInitialized();
+
+  final stream = eg.stream();
+  final narrow = StreamNarrow(stream.streamId);
+
+  group('update message tests', () {
+
+    test('find message in message list returns index of message', () async {
+      final store = await setupStore(stream);
+
+      final m1 = eg.streamMessage(id: 2, stream: stream);
+      final m2 = eg.streamMessage(id: 4, stream: stream);
+      final m3 = eg.streamMessage(id: 6, stream: stream);
+
+      final messageList = await messageListViewWithMessages([m1, m2, m3], store, narrow);
+      // The implementation of this uses a binary search, so let's test it
+      // a bit more exhaustively.
+
+      check(messageList.findMessageWithId(1)).equals(-1);
+      check(messageList.findMessageWithId(2)).equals(0);
+      check(messageList.findMessageWithId(3)).equals(-1);
+      check(messageList.findMessageWithId(4)).equals(1);
+      check(messageList.findMessageWithId(5)).equals(-1);
+      check(messageList.findMessageWithId(6)).equals(2);
+      check(messageList.findMessageWithId(7)).equals(-1);
+
+      // Invalid IDs
+      check(messageList.findMessageWithId(-8409)).equals(-1);
+      check(messageList.findMessageWithId(0)).equals(-1);
+    });
+
+    test('update events are correctly applied to message when it is in the stream', () async {
+      final store = await setupStore(stream);
+
+      const oldContent = "<p>Hello, world</p>";
+      const newContent = "<p>Hello, edited</p>";
+      const newTimestamp = 99999;
+
+      List<String> oldFlags = [];
+      List<String> newFlags = ["starred"];
+
+      final originalMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent, flags: oldFlags);
+      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
+
+      final updateEvent = UpdateMessageEvent(
+        id: 1,
+        messageId: originalMessage.id,
+        messageIds: [originalMessage.id],
+        flags: newFlags,
+        renderedContent: newContent,
+        editTimestamp: newTimestamp,
+        isMeMessage: true,
+        userId: userId,
+        renderingOnly: false,
+      );
+
+      final message = messageList.messages.single;
+      check(message)
+        ..content.equals(oldContent)
+        ..flags.deepEquals(oldFlags)
+        ..isMeMessage.isFalse();
+
+      var listenersNotified = false;
+
+      messageList.addListener(() { listenersNotified = true; });
+      messageList.maybeUpdateMessage(updateEvent);
+
+      check(listenersNotified).isTrue();
+
+      check(message)
+        ..identicalTo(messageList.messages.single)
+        ..content.equals(newContent)
+        ..lastEditTimestamp.equals(newTimestamp)
+        ..flags.equals(newFlags)
+        ..isMeMessage.isTrue();
+    });
+
+    test('update event is ignored when message is not in the message list', () async {
+      final store = await setupStore(stream);
+
+      const oldContent = "<p>Hello, world</p>";
+      const newContent = "<p>Hello, edited</p>";
+      const newTimestamp = 99999;
+
+      final originalMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent);
+      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
+
+      final updateEvent = UpdateMessageEvent(
+        id: 1,
+        messageId: originalMessage.id + 1,
+        messageIds: [originalMessage.id + 1],
+        flags: originalMessage.flags,
+        renderedContent: newContent,
+        editTimestamp: newTimestamp,
+        userId: userId,
+        renderingOnly: false,
+      );
+
+      final message = messageList.messages.single;
+      check(message).content.equals(oldContent);
+
+      var listenersNotified = false;
+
+      messageList.addListener(() { listenersNotified = true; });
+      messageList.maybeUpdateMessage(updateEvent);
+
+      check(listenersNotified).isFalse();
+      check(message).content.equals(oldContent);
+    });
+
+    test('rendering-only update does not change timestamp', () async {
+      final store = await setupStore(stream);
+
+      const oldContent = "<p>Hello, world</p>";
+      const oldTimestamp = 78492;
+      const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
+      const newTimestamp = 99999;
+
+      final originalMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
+      originalMessage.lastEditTimestamp = oldTimestamp;
+
+      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
+
+      final updateEvent = UpdateMessageEvent(
+        id: 1,
+        messageId: originalMessage.id,
+        messageIds: [originalMessage.id],
+        flags: originalMessage.flags,
+        renderedContent: newContent,
+        editTimestamp: newTimestamp,
+        renderingOnly: true,
+        userId: null,
+      );
+
+      final message = messageList.messages[0];
+      messageList.maybeUpdateMessage(updateEvent);
+      check(message)
+        ..content.equals(newContent)
+        ..lastEditTimestamp.equals(oldTimestamp);
+    });
+
+    // TODO(server-5): Cut this test; rely on renderingOnly from FL 114
+    test('rendering-only update does not change timestamp (for old server versions)', () async {
+      final store = await setupStore(stream);
+
+      const oldContent = "<p>Hello, world</p>";
+      const oldTimestamp = 78492;
+      const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
+      const newTimestamp = 99999;
+
+      final originalMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
+      originalMessage.lastEditTimestamp = oldTimestamp;
+
+      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
+
+      final updateEvent = UpdateMessageEvent(
+        id: 1,
+        messageId: originalMessage.id,
+        messageIds: [originalMessage.id],
+        flags: originalMessage.flags,
+        renderedContent: newContent,
+        editTimestamp: newTimestamp,
+        userId: null,
+      );
+
+      final message = messageList.messages.single;
+      messageList.maybeUpdateMessage(updateEvent);
+      check(message)
+        ..content.equals(newContent)
+        ..lastEditTimestamp.equals(oldTimestamp);
+    });
+  });
+}

Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,9 @@ extension MessageChecks on Subject<Message> {`
`8`	`8`	`toJson.deepEquals(expected.toJson());`
`9`	`9`	`}`
`10`	`10`
	`11`	`+ Subject<String> get content => has((e) => e.content, 'content');`
	`12`	`+ Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');`
	`13`	`+ Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');`
`11`	`14`	`Subject<List<String>> get flags => has((e) => e.flags, 'flags');`
`12`	`15`
`13`	`16`	`// TODO accessors for other fields`