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..43e058fcd97 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,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 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..a049df975a3 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -8,6 +8,9 @@ extension MessageChecks on Subject { toJson.deepEquals(expected.toJson()); } + Subject get content => has((e) => e.content, 'content'); + Subject get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage'); + Subject get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp'); Subject> get flags => has((e) => e.flags, 'flags'); // TODO accessors for other fields diff --git a/test/example_data.dart b/test/example_data.dart index da24aa274d2..0e0e87171ea 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, @@ -158,6 +159,7 @@ DmMessage dmMessage({ required List to, String? content, String? contentMarkdown, + List? 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 new file mode 100644 index 00000000000..06458b6e1f3 --- /dev/null +++ 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 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 messageListViewWithMessages(List 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 = "

Hello, world

"; + const newContent = "

Hello, edited

"; + const newTimestamp = 99999; + + List oldFlags = []; + List 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 = "

Hello, world

"; + const newContent = "

Hello, edited

"; + 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 = "

Hello, world

"; + const oldTimestamp = 78492; + const newContent = "

Hello, world

Some link preview
"; + 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 = "

Hello, world

"; + const oldTimestamp = 78492; + const newContent = "

Hello, world

Some link preview
"; + 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); + }); + }); +}