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, } 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/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/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'}]); + }); + }); + }); } 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', () {