From 37680b187648293c50ef9fb8ec2305e029dd4b99 Mon Sep 17 00:00:00 2001 From: Amy Dyer Date: Mon, 24 Jul 2023 11:18:10 -0400 Subject: [PATCH] msglist: Handle updated events in MessageListView (#118). 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 #118. --- lib/api/model/model.dart | 8 +- lib/model/message_list.dart | 66 ++++++++++ lib/model/store.dart | 4 +- test/api/model/model_checks.dart | 3 + test/example_data.dart | 3 +- test/model/message_list_test.dart | 200 ++++++++++++++++++++++++++++++ 6 files changed, 278 insertions(+), 6 deletions(-) create mode 100644 test/model/message_list_test.dart diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 6dbdec8d13f..e7e81495016 100644 --- 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 editHistory; // TODO handle final int id; - final bool isMeMessage; - final int? lastEditTimestamp; + bool isMeMessage; + int? lastEditTimestamp; // final List reactions; // TODO handle final int recipientId; @@ -271,7 +271,7 @@ sealed class Message { // final List topicLinks; // TODO handle // final string type; // handled by runtime type of object - final List flags; // TODO enum + List flags; // TODO enum final String? matchContent; final String? matchSubject; diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 952b00ebbd4..5e2512a7d72 100644 --- 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,70 @@ 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. + bool isRenderingOnly = (event.renderingOnly != null && event.renderingOnly == true) || + 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; + } + + if (!event.flags.equals(message.flags)) { + message.flags = event.flags; + } + + if (event.renderedContent != null) { + assert(message.contentType == 'text/html', "Expected message to have html contentType. Instead, got ${message.contentType}"); + message.content = event.renderedContent!; + } + + if (event.isMeMessage != null) { + message.isMeMessage = event.isMeMessage!; + } + + } + + ///This is almost directly copied from package:collection/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); + Message 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; + } + + void maybeUpdateMessage(UpdateMessageEvent event) { + int idx = findMessageWithId(event.messageId); + + if (idx == -1) { + return; + } + + Message 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 index d9de7200207..f5121f3da82 100644 --- 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 index 1b41954791b..c9eb326cee6 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -9,6 +9,9 @@ extension MessageChecks on Subject { } Subject> get flags => has((e) => e.flags, 'flags'); + Subject get content => has((e) => e.content, 'content'); + Subject get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage'); + Subject get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp'); // TODO accessors for other fields } diff --git a/test/example_data.dart b/test/example_data.dart index da24aa274d2..14d3ef6a598 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -127,6 +127,7 @@ StreamMessage streamMessage({ String? topic, String? content, String? contentMarkdown, + List? 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, diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart new file mode 100644 index 00000000000..7baec5b94f7 --- /dev/null +++ b/test/model/message_list_test.dart @@ -0,0 +1,200 @@ +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; +const int streamId = 2; + +Future setupStore(ZulipStream stream) async { + await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + PerAccountStore store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id); + store.addUser(eg.user(userId: userId)); + store.addStream(stream); + return store; +} + +Future messageListViewWithMessages(List messages, PerAccountStore store, Narrow narrow) async { + MessageListView 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(); + const narrow = StreamNarrow(streamId); + + final ZulipStream stream = eg.stream(streamId: streamId); + PerAccountStore store = await setupStore(stream); + + group('update message tests', () { + + test('find message in message list returns index of message', () async { + Message m1 = eg.streamMessage(id: 792, stream: stream); + Message m2 = eg.streamMessage(id: 793, stream: stream); + Message m3 = eg.streamMessage(id: 794, stream: stream); + + MessageListView messageList = await messageListViewWithMessages([m1, m2, m3], store, narrow); + + int idx = messageList.findMessageWithId(793); + check(idx).equals(1); + + idx = messageList.findMessageWithId(999); + check(idx).equals(-1); + }); + + test('update events are correctly applied to message when it is in the stream', () async { + String oldContent = "

Hello, world

"; + String newContent = "

Hello, edited

"; + int newTimestamp = 99999; + + List oldFlags = []; + List newFlags = ["starred"]; + + Message mockMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent, flags: oldFlags); + MessageListView messageList = await messageListViewWithMessages([mockMessage], store, narrow); + + UpdateMessageEvent updateEvent = UpdateMessageEvent( + id: 1, + messageId: mockMessage.id, + messageIds: [mockMessage.id], + flags: newFlags, + renderedContent: newContent, + editTimestamp: newTimestamp, + isMeMessage: true, + userId: userId + ); + + Message message = messageList.messages[0]; + check(message) + ..content.equals(oldContent) + ..flags.deepEquals(oldFlags) + ..isMeMessage.equals(false); + + bool listenersNotified = false; + + messageList.addListener(() { listenersNotified = true; }); + messageList.maybeUpdateMessage(updateEvent); + + Message updatedMessage = messageList.messages[0]; + check(updatedMessage).identicalTo(message); + check(listenersNotified).equals(true); + + check(message) + ..content.equals(newContent) + ..lastEditTimestamp.equals(newTimestamp) + ..flags.equals(newFlags) + ..isMeMessage.equals(true); + }); + + test('update event is ignored when message is not in the message list', () async { + String oldContent = "

Hello, world

"; + String newContent = "

Hello, edited

"; + int newTimestamp = 99999; + + Message mockMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent); + MessageListView messageList = await messageListViewWithMessages([mockMessage], store, narrow); + + UpdateMessageEvent updateEvent = UpdateMessageEvent( + id: 1, + messageId: 972, + messageIds: [972], + flags: mockMessage.flags, + renderedContent: newContent, + editTimestamp: newTimestamp, + userId: userId, + ); + + Message message = messageList.messages[0]; + check(message).content.equals(oldContent); + + bool listenersNotified = false; + + messageList.addListener(() { listenersNotified = true; }); + messageList.maybeUpdateMessage(updateEvent); + + Message updatedMessage = messageList.messages[0]; + + check(listenersNotified).equals(false); + check(updatedMessage).identicalTo(message); + check(message).content.equals(oldContent); + + }); + test('rendering-only update does not change timestamp', () async { + String oldContent = "

Hello, world

"; + String newContent = "

Hello, world

Some link preview
"; + int newTimestamp = 99999; + + Message mockMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent); + MessageListView messageList = await messageListViewWithMessages([mockMessage], store, narrow); + + UpdateMessageEvent updateEvent = UpdateMessageEvent( + id: 1, + messageId: 972, + messageIds: [972], + flags: mockMessage.flags, + renderedContent: newContent, + editTimestamp: newTimestamp, + renderingOnly: true, + userId: null, + ); + + Message message = messageList.messages[0]; + messageList.maybeUpdateMessage(updateEvent); + check(message) + ..content.equals(newContent) + ..lastEditTimestamp.isNull(); + }); + + test('rendering-only update does not change timestamp (for old server versions)', () async { + String oldContent = "

Hello, world

"; + String newContent = "

Hello, world

Some link preview
"; + int newTimestamp = 99999; + + Message mockMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent); + MessageListView messageList = await messageListViewWithMessages([mockMessage], store, narrow); + + UpdateMessageEvent updateEvent = UpdateMessageEvent( + id: 1, + messageId: 972, + messageIds: [972], + flags: mockMessage.flags, + renderedContent: newContent, + editTimestamp: newTimestamp, + userId: null, + ); + + Message message = messageList.messages[0]; + messageList.maybeUpdateMessage(updateEvent); + check(message) + ..content.equals(newContent) + ..lastEditTimestamp.isNull(); + }); + + + }); +} \ No newline at end of file