From 701f463e9bbefad3e74550888ad2e901518ed7c1 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 18 Jun 2024 16:43:32 -0400 Subject: [PATCH 1/3] vscode: Disable warnings for TODOs. Since we manage TODOs in a long term manner and as reminders until a migration can actually happen, it would be less annoying if those warnings are disabled by default. Signed-off-by: Zixuan James Li --- .vscode/settings.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.vscode/settings.json b/.vscode/settings.json index 730c9474c9..06f50b753f 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -17,4 +17,7 @@ // This much more focused automatic fix is helpful, though. "files.trimTrailingWhitespace": true, + + // Suppress the warnings that appear for TODOs throughout the codebase. + "dart.showTodos": false, } From 072ad37de3fcab0f4d699b26e8b84b0b286b5f71 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 18 Jun 2024 16:45:32 -0400 Subject: [PATCH 2/3] model: Add MessageEditState to Message. This new field is computed from edit_history provided by the API. We create a readValue function that processes the list of edits and determine if message has been edited or moved. Some special handling was needed because topic being marked as resolved should not be considered "moved". Signed-off-by: Zixuan James Li --- lib/api/model/model.dart | 87 ++++++++++++++++++++++++ lib/api/model/model.g.dart | 12 ++++ test/api/model/model_checks.dart | 1 + test/api/model/model_test.dart | 109 +++++++++++++++++++++++++++++++ 4 files changed, 209 insertions(+) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 5e5c1f482e..b5abdf7905 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -464,6 +464,9 @@ sealed class Message { final String contentType; // final List editHistory; // TODO handle + @JsonKey(readValue: MessageEditState._readFromMessage, fromJson: Message._messageEditStateFromJson) + MessageEditState editState; + final int id; bool isMeMessage; int? lastEditTimestamp; @@ -490,6 +493,12 @@ sealed class Message { @JsonKey(name: 'match_subject') final String? matchTopic; + static MessageEditState _messageEditStateFromJson(dynamic json) { + // This is a no-op so that [MessageEditState._readFromMessage] + // can return the enum value directly. + return json as MessageEditState; + } + static Reactions? _reactionsFromJson(dynamic json) { final list = (json as List); return list.isNotEmpty ? Reactions.fromJson(list) : null; @@ -508,6 +517,7 @@ sealed class Message { required this.client, required this.content, required this.contentType, + required this.editState, required this.id, required this.isMeMessage, required this.lastEditTimestamp, @@ -573,6 +583,7 @@ class StreamMessage extends Message { required super.client, required super.content, required super.contentType, + required super.editState, required super.id, required super.isMeMessage, required super.lastEditTimestamp, @@ -675,6 +686,7 @@ class DmMessage extends Message { required super.client, required super.content, required super.contentType, + required super.editState, required super.id, required super.isMeMessage, required super.lastEditTimestamp, @@ -698,3 +710,78 @@ class DmMessage extends Message { @override Map toJson() => _$DmMessageToJson(this); } + +enum MessageEditState { + none, + edited, + moved; + + // Adapted from the shared code: + // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts + // The canonical resolved-topic prefix. + static const String _resolvedTopicPrefix = '✔ '; + + /// Whether the given topic move reflected either a "resolve topic" + /// or "unresolve topic" operation. + /// + /// The Zulip "resolved topics" feature is implemented by renaming the topic; + /// but for purposes of [Message.editState], we want to ignore such renames. + /// This method identifies topic moves that should be ignored in that context. + static bool topicMoveWasResolveOrUnresolve(String topic, String prevTopic) { + if (topic.startsWith(_resolvedTopicPrefix) + && topic.substring(_resolvedTopicPrefix.length) == prevTopic) { + return true; + } + + if (prevTopic.startsWith(_resolvedTopicPrefix) + && prevTopic.substring(_resolvedTopicPrefix.length) == topic) { + return true; + } + + return false; + } + + static MessageEditState _readFromMessage(Map json, String key) { + // Adapted from `analyze_edit_history` in the web app: + // https://github.com/zulip/zulip/blob/c31cebbf68a93927d41e9947427c2dd4d46503e3/web/src/message_list_view.js#L68-L118 + final editHistory = json['edit_history'] as List?; + final lastEditTimestamp = json['last_edit_timestamp'] as int?; + if (editHistory == null) { + return (lastEditTimestamp != null) + ? MessageEditState.edited + : MessageEditState.none; + } + + // Edit history should never be empty whenever it is present + assert(editHistory.isNotEmpty); + + bool hasMoved = false; + for (final entry in editHistory) { + if (entry['prev_content'] != null) { + return MessageEditState.edited; + } + + if (entry['prev_stream'] != null) { + hasMoved = true; + continue; + } + + // TODO(server-5) prev_subject was the old name of prev_topic on pre-5.0 servers + final prevTopic = (entry['prev_topic'] ?? entry['prev_subject']) as String?; + final topic = entry['topic'] as String?; + if (prevTopic != null) { + // TODO(server-5) pre-5.0 servers do not have the 'topic' field + if (topic == null) { + hasMoved = true; + } else { + hasMoved |= !topicMoveWasResolveOrUnresolve(topic, prevTopic); + } + } + } + + if (hasMoved) return MessageEditState.moved; + + // This can happen when a topic is resolved but nothing else has been edited + return MessageEditState.none; + } +} diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index e6b60b62d3..456107ab85 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -265,6 +265,8 @@ StreamMessage _$StreamMessageFromJson(Map json) => client: json['client'] as String, content: json['content'] as String, contentType: json['content_type'] as String, + editState: Message._messageEditStateFromJson( + MessageEditState._readFromMessage(json, 'edit_state')), id: (json['id'] as num).toInt(), isMeMessage: json['is_me_message'] as bool, lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), @@ -288,6 +290,7 @@ Map _$StreamMessageToJson(StreamMessage instance) => 'client': instance.client, 'content': instance.content, 'content_type': instance.contentType, + 'edit_state': _$MessageEditStateEnumMap[instance.editState]!, 'id': instance.id, 'is_me_message': instance.isMeMessage, 'last_edit_timestamp': instance.lastEditTimestamp, @@ -307,6 +310,12 @@ Map _$StreamMessageToJson(StreamMessage instance) => 'stream_id': instance.streamId, }; +const _$MessageEditStateEnumMap = { + MessageEditState.none: 'none', + MessageEditState.edited: 'edited', + MessageEditState.moved: 'moved', +}; + DmRecipient _$DmRecipientFromJson(Map json) => DmRecipient( id: (json['id'] as num).toInt(), email: json['email'] as String, @@ -324,6 +333,8 @@ DmMessage _$DmMessageFromJson(Map json) => DmMessage( client: json['client'] as String, content: json['content'] as String, contentType: json['content_type'] as String, + editState: Message._messageEditStateFromJson( + MessageEditState._readFromMessage(json, 'edit_state')), id: (json['id'] as num).toInt(), isMeMessage: json['is_me_message'] as bool, lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), @@ -346,6 +357,7 @@ Map _$DmMessageToJson(DmMessage instance) => { 'client': instance.client, 'content': instance.content, 'content_type': instance.contentType, + 'edit_state': _$MessageEditStateEnumMap[instance.editState]!, 'id': instance.id, 'is_me_message': instance.isMeMessage, 'last_edit_timestamp': instance.lastEditTimestamp, diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 76756693a0..436e3569c6 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -34,6 +34,7 @@ extension MessageChecks on Subject { Subject get id => has((e) => e.id, 'id'); Subject get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage'); Subject get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp'); + Subject get editState => has((e) => e.editState, 'editState'); Subject get reactions => has((e) => e.reactions, 'reactions'); Subject get recipientId => has((e) => e.recipientId, 'recipientId'); Subject get senderEmail => has((e) => e.senderEmail, 'senderEmail'); diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index e97ea882b9..3e2f9f82f7 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -148,6 +148,9 @@ void main() { ); check(m2).flags.deepEquals([MessageFlag.read, MessageFlag.unknown]); }); + + // Code relevant to messageEditState is tested separately in the + // MessageEditState group. }); group('DmMessage', () { @@ -212,4 +215,110 @@ void main() { .deepEquals([2, 3, 11]); }); }); + + group('MessageEditState', () { + Map baseJson() => deepToJson(eg.streamMessage()) as Map; + + group('Edit history is absent', () { + test('Message with no evidence of an edit history -> none', () { + check(Message.fromJson(baseJson()..['edit_history'] = null)) + .editState.equals(MessageEditState.none); + }); + + test('Message without edit history has last edit timestamp -> edited', () { + check(Message.fromJson(baseJson() + ..['edit_history'] = null + ..['last_edit_timestamp'] = 1678139636)) + .editState.equals(MessageEditState.edited); + }); + }); + + void checkEditState(MessageEditState editState, List> editHistory){ + check(Message.fromJson(baseJson()..['edit_history'] = editHistory)) + .editState.equals(editState); + } + + group('edit history exists', () { + test('Moved message has last edit timestamp but no actual edits -> moved', () { + check(Message.fromJson(baseJson() + ..['edit_history'] = [{'prev_stream': 5, 'stream': 7}] + ..['last_edit_timestamp'] = 1678139636)) + .editState.equals(MessageEditState.moved); + }); + + test('Channel change only -> moved', () { + checkEditState(MessageEditState.moved, + [{'prev_stream': 5, 'stream': 7}]); + }); + + test('Topic name change only -> moved', () { + checkEditState(MessageEditState.moved, + [{'prev_topic': 'old_topic', 'topic': 'new_topic'}]); + }); + + test('Both topic and content changed -> edited', () { + checkEditState(MessageEditState.edited, [ + {'prev_topic': 'old_topic', 'topic': 'new_topic'}, + {'prev_content': 'old_content'}, + ]); + checkEditState(MessageEditState.edited, [ + {'prev_content': 'old_content'}, + {'prev_topic': 'old_topic', 'topic': 'new_topic'}, + ]); + }); + + test('Both topic and content changed in a single edit -> edited', () { + checkEditState(MessageEditState.edited, + [{'prev_topic': 'old_topic', 'topic': 'new_topic', 'prev_content': 'old_content'}]); + }); + + test('Content change only -> edited', () { + checkEditState(MessageEditState.edited, + [{'prev_content': 'old_content'}]); + }); + + test("'prev_topic' present without the 'topic' field -> moved", () { + checkEditState(MessageEditState.moved, + [{'prev_topic': 'old_topic'}]); + }); + + test("'prev_subject' present from a pre-5.0 server -> moved", () { + checkEditState(MessageEditState.moved, + [{'prev_subject': 'old_topic'}]); + }); + }); + + group('topic resolved in edit history', () { + test('Topic was only resolved -> none', () { + checkEditState(MessageEditState.none, + [{'prev_topic': 'old_topic', 'topic': '✔ old_topic'}]); + }); + + test('Topic was resolved but the content changed in the history -> edited', () { + checkEditState(MessageEditState.edited, [ + {'prev_topic': 'old_topic', 'topic': '✔ old_topic'}, + {'prev_content': 'old_content'}, + ]); + }); + + test('Topic was resolved but it also moved in the history -> moved', () { + checkEditState(MessageEditState.moved, [ + {'prev_topic': 'old_topic', 'topic': 'new_topic'}, + {'prev_topic': '✔ old_topic', 'topic': 'old_topic'}, + ]); + }); + + test('Topic was moved but it also was resolved in the history -> moved', () { + checkEditState(MessageEditState.moved, [ + {'prev_topic': '✔ old_topic', 'topic': 'old_topic'}, + {'prev_topic': 'old_topic', 'topic': 'new_topic'}, + ]); + }); + + test('Unresolving topic with a weird prefix -> moved', () { + checkEditState(MessageEditState.moved, + [{'prev_topic': '✔ ✔old_topic', 'topic': 'old_topic'}]); + }); + }); + }); } From 3409127c8938534eb8bff6a048cac85e14b2b479 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 20 Jun 2024 21:43:23 -0400 Subject: [PATCH 3/3] message: Update editState on events. When a channel (a.k.a stream) or a topic gets updated on a message, we get a UpdateMessageEvent from the server. The same happens when the message's content is updated. This updates the edit state to show the "edited" or "moved" marker properly on the messages affected, and leave other move related states (like streamId) untouched until we get to #150. Signed-off-by: Zixuan James Li --- lib/model/message.dart | 25 +++++++++++- test/example_data.dart | 32 +++++++++++++++ test/model/message_test.dart | 79 +++++++++++++++++++++++++++++++++++- 3 files changed, 132 insertions(+), 4 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 75e22f6bde..7ad0fe3ce2 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -125,6 +125,11 @@ class MessageStoreImpl with MessageStore { if (message == null) return; message.flags = event.flags; + if (event.origContent != null) { + // The message is guaranteed to be edited. + // See also: https://zulip.com/api/get-events#update_message + message.editState = MessageEditState.edited; + } if (event.renderedContent != null) { assert(message.contentType == 'text/html', "Message contentType was ${message.contentType}; expected text/html."); @@ -168,10 +173,26 @@ class MessageStoreImpl with MessageStore { return; } - // final newStreamId = event.newStreamId; // null if topic-only move - // final newTopic = event.newTopic!; + final newTopic = event.newTopic!; + final newChannelId = event.newStreamId; // null if topic-only move + + if (newChannelId == null + && MessageEditState.topicMoveWasResolveOrUnresolve(event.origTopic!, newTopic)) { + // The topic was only resolved/unresolved. + // No change to the messages' editState. + return; + } + // 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; + } } void handleDeleteMessageEvent(DeleteMessageEvent event) { diff --git a/test/example_data.dart b/test/example_data.dart index adc2da4246..b563ad5e7a 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -437,6 +437,38 @@ UpdateMessageEvent updateMessageEditEvent( ); } +UpdateMessageEvent updateMessageMoveEvent( + List messages, { + int? newStreamId, + String? origTopic, + String? newTopic, + String? origContent, + String? newContent, +}) { + assert(messages.isNotEmpty); + final origMessage = messages[0]; + final messageId = origMessage.id; + return UpdateMessageEvent( + id: 0, + userId: selfUser.userId, + renderingOnly: false, + messageId: messageId, + messageIds: messages.map((message) => message.id).toList(), + flags: origMessage.flags, + editTimestamp: 1234567890, // TODO generate timestamp + origStreamId: origMessage is StreamMessage ? origMessage.streamId : null, + newStreamId: newStreamId, + propagateMode: null, + origTopic: origTopic, + newTopic: newTopic, + origContent: origContent, + origRenderedContent: origContent, + content: newContent, + renderedContent: newContent, + isMeMessage: false, + ); +} + UpdateMessageFlagsRemoveEvent updateMessageFlagsRemoveEvent( MessageFlag flag, Iterable messages, { diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 248d9fcfc3..7d65975f84 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -207,7 +207,8 @@ void main() { ..content.not((it) => it.equals(updateEvent.renderedContent!)) ..lastEditTimestamp.isNull() ..flags.not((it) => it.deepEquals(updateEvent.flags)) - ..isMeMessage.not((it) => it.equals(updateEvent.isMeMessage!)); + ..isMeMessage.not((it) => it.equals(updateEvent.isMeMessage!)) + ..editState.equals(MessageEditState.none); await store.handleEvent(updateEvent); checkNotifiedOnce(); @@ -216,7 +217,8 @@ void main() { ..content.equals(updateEvent.renderedContent!) ..lastEditTimestamp.equals(updateEvent.editTimestamp) ..flags.equals(updateEvent.flags) - ..isMeMessage.equals(updateEvent.isMeMessage!); + ..isMeMessage.equals(updateEvent.isMeMessage!) + ..editState.equals(MessageEditState.edited); }); test('ignore when message unknown', () async { @@ -269,6 +271,79 @@ void main() { test('rendering-only update does not change timestamp (for old server versions)', () async { await checkRenderingOnly(legacy: true); }); + + group('Handle message edit state update', () { + final message = eg.streamMessage(); + final otherMessage = eg.streamMessage(); + + Future sendEvent(Message message, UpdateMessageEvent event) async { + await prepare(); + await prepareMessages([message, otherMessage]); + + await store.handleEvent(event); + checkNotifiedOnce(); + } + + test('message not moved update', () async { + await sendEvent(message, eg.updateMessageEditEvent(message)); + check(store).messages[message.id].editState.equals(MessageEditState.edited); + check(store).messages[otherMessage.id].editState.equals(MessageEditState.none); + }); + + test('message topic moved update', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: 'old topic', + newTopic: 'new topic')); + check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); + }); + + test('message topic resolved update', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: 'new topic', + newTopic: '✔ new topic')); + check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); + }); + + test('message topic unresolved update', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: '✔ new topic', + newTopic: 'new topic')); + check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); + }); + + test('message topic both resolved and edited update', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: 'new topic', + newTopic: '✔ new topic 2')); + check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); + }); + + test('message topic both unresolved and edited update', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: '✔ new topic', + newTopic: 'new topic 2')); + check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); + }); + + test('message stream moved update', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: 'topic', + newTopic: 'topic', + newStreamId: 20)); + check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); + }); + + test('message is both moved and updated', () async { + await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + origTopic: 'topic', + newTopic: 'topic', + newStreamId: 20, + origContent: 'old content', + newContent: 'new content')); + check(store).messages[message.id].editState.equals(MessageEditState.edited); + check(store).messages[otherMessage.id].editState.equals(MessageEditState.moved); + }); + }); }); group('handleUpdateMessageFlagsEvent', () {