From 5f7cccd3840d81a7fa3b494e0e46ad3bdb3d4114 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 28 Jan 2025 17:15:58 -0500 Subject: [PATCH 01/10] api [nfc]: Add PropagateMode.fromRawString Signed-off-by: Zixuan James Li --- lib/api/model/model.dart | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index fad8ddc5bc..5c94626584 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -954,4 +954,15 @@ enum PropagateMode { changeAll; String toJson() => _$PropagateModeEnumMap[this]!; + + /// Get a [PropagateMode] from a raw string. Throws if the string is + /// unrecognized. + /// + /// Example: + /// 'change_one' -> PropagateMode.changeOne + static PropagateMode fromRawString(String raw) => _byRawString[raw]!; + + // _$…EnumMap is thanks to `alwaysCreate: true` and `fieldRename: FieldRename.snake` + static final _byRawString = _$PropagateModeEnumMap + .map((key, value) => MapEntry(value, key)); } From 26413203ca05b31598cd1f682c7a372eb00b1958 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 19 Feb 2025 20:53:21 -0500 Subject: [PATCH 02/10] msglist test: Fix invalid message move The streamId of the moved messages does not change as intended, and the topic remains the same; this is not a valid message move. We will later introduce a data structure that captures inconsistencies of this kind. Signed-off-by: Zixuan James Li --- test/model/message_list_test.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index da286f3ef5..92c08ddd6b 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -825,11 +825,14 @@ void main() { }); test('unrelated channel -> new channel: unaffected', () async { + final thirdStream = eg.stream(); await prepareNarrow(narrow, initialMessages); + await store.addStream(thirdStream); + await store.addSubscription(eg.subscription(thirdStream)); await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: otherChannelMovedMessages, - newStreamId: otherStream.streamId, + newStreamId: thirdStream.streamId, )); checkHasMessages(initialMessages); checkNotNotified(); From 00685c50c5bc283804a22080aa890e2185f42743 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 19 Feb 2025 22:36:49 -0500 Subject: [PATCH 03/10] events test: Make test data more realistic The fields orig_subject and propagate_mode are always present on message moves. See also API documentation: https://zulip.com/api/get-events#update_message Signed-off-by: Zixuan James Li --- test/api/model/events_test.dart | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/api/model/events_test.dart b/test/api/model/events_test.dart index 51b36350cc..a0d52257f8 100644 --- a/test/api/model/events_test.dart +++ b/test/api/model/events_test.dart @@ -101,21 +101,25 @@ void main() { 'edit_timestamp': 1718741351, 'stream_id': eg.stream().streamId, }; + final baseMoveJson = { ...baseJson, + 'orig_subject': 'foo', + 'propagate_mode': 'change_all', + }; test('stream_id -> origStreamId', () { - check(Event.fromJson({ ...baseJson, + check(Event.fromJson({ ...baseMoveJson, 'stream_id': 1, 'new_stream_id': 2, - }) as UpdateMessageEvent) + })).isA() ..origStreamId.equals(1) ..newStreamId.equals(2); }); test('orig_subject -> origTopic, subject -> newTopic', () { - check(Event.fromJson({ ...baseJson, + check(Event.fromJson({ ...baseMoveJson, 'orig_subject': 'foo', 'subject': 'bar', - }) as UpdateMessageEvent) + })).isA() ..origTopic.equals(const TopicName('foo')) ..newTopic.equals(const TopicName('bar')); }); From d396e287bf30e4776fc24e625645681b7c27fd1d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Mar 2025 22:45:43 -0700 Subject: [PATCH 04/10] message: Identify moves by inequality new vs orig, not just non-nullity This is NFC except in certain malformed cases: where either newStreamId is present and equals origStreamId, or newTopic is present and equals origTopic. The API docs say neither of those should happen. This version is useful because it brings this code closer to how it will look when we move this logic of "were the messages moved, and how" into the API parsing code so that other parts of the data model can reuse the results. --- lib/model/message.dart | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 84f3bbc3e5..01c8617907 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -173,16 +173,14 @@ class MessageStoreImpl with MessageStore { // For reference, see: https://zulip.com/api/get-events#update_message final origStreamId = event.origStreamId; - final newStreamId = event.newStreamId; // null if topic-only move final origTopic = event.origTopic; - final newTopic = event.newTopic; final propagateMode = event.propagateMode; if (origTopic == null) { // There was no move. assert(() { - if (newStreamId != null && origStreamId != null - && newStreamId != origStreamId) { + if (event.newStreamId != null && origStreamId != null + && event.newStreamId != origStreamId) { // This should be impossible; `orig_subject` (aka origTopic) is // documented to be present when either the stream or topic changed. debugLog('Malformed UpdateMessageEvent: stream move but no origTopic'); // TODO(log) @@ -192,12 +190,6 @@ class MessageStoreImpl with MessageStore { return; } - if (newStreamId == null && newTopic == null) { - // If neither the channel nor topic name changed, nothing moved. - // In that case `orig_subject` (aka origTopic) should have been null. - assert(debugLog('Malformed UpdateMessageEvent: move but no newStreamId or newTopic')); // TODO(log) - return; - } if (origStreamId == null) { // The `stream_id` field (aka origStreamId) is documented to be present on moves. assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log) @@ -209,8 +201,17 @@ class MessageStoreImpl with MessageStore { return; } - final wasResolveOrUnresolve = (newStreamId == null - && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!)); + final newStreamId = event.newStreamId ?? origStreamId; + final newTopic = event.newTopic ?? origTopic; + if (newStreamId == origStreamId && newTopic == origTopic) { + // If neither the channel nor topic name changed, nothing moved. + // In that case `orig_subject` (aka origTopic) should have been null. + assert(debugLog('Malformed UpdateMessageEvent: move but no newStreamId or newTopic')); // TODO(log) + return; + } + + final wasResolveOrUnresolve = newStreamId == origStreamId + && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic); for (final messageId in event.messageIds) { final message = messages[messageId]; @@ -221,14 +222,14 @@ class MessageStoreImpl with MessageStore { continue; } - if (newStreamId != null) { + if (newStreamId != origStreamId) { message.streamId = newStreamId; // See [StreamMessage.displayRecipient] on why the invalidation is // needed. message.displayRecipient = null; } - if (newTopic != null) { + if (newTopic != origTopic) { message.topic = newTopic; } @@ -241,9 +242,9 @@ class MessageStoreImpl with MessageStore { for (final view in _messageListViews) { view.messagesMoved( origStreamId: origStreamId, - newStreamId: newStreamId ?? origStreamId, + newStreamId: newStreamId, origTopic: origTopic, - newTopic: newTopic ?? origTopic, + newTopic: newTopic, messageIds: event.messageIds, propagateMode: propagateMode, ); From 5149268b52819287e1fd40b52823f09be1dacc71 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Mar 2025 23:02:23 -0700 Subject: [PATCH 05/10] message [nfc]: In moves, give newStreamId/newTopic their fallback values sooner To see that this is NFC, first observe that the only possible behavior change this could cause is to change some event from being ignored (hitting a `return` in this section of code) to being processed by the code below, or vice versa. (It might also change what particular message gets printed to the debug log; but that doesn't count as a behavior change.) Then both before and after this change, the events that this overall set of conditions accepts are those that (a) have all of origTopic, origStreamId, and propagateMode non-null, and (b) have at least one of newTopic or newStreamId with a non-null value different from origTopic or origStreamId respectively. This change is useful because it brings this logic closer to how it will look when we move it into the API parsing code. While here, we add a debug log message on unexpected propagateMode, to further bring this closer to how it will look in the API parsing code. Co-authored-by: Zixuan James Li --- lib/model/message.dart | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 01c8617907..d32918b7e5 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -173,43 +173,39 @@ class MessageStoreImpl with MessageStore { // For reference, see: https://zulip.com/api/get-events#update_message final origStreamId = event.origStreamId; + final newStreamId = event.newStreamId ?? origStreamId; final origTopic = event.origTopic; + final newTopic = event.newTopic ?? origTopic; final propagateMode = event.propagateMode; - if (origTopic == null) { + if (newStreamId == origStreamId && newTopic == origTopic) { // There was no move. - assert(() { - if (event.newStreamId != null && origStreamId != null - && event.newStreamId != origStreamId) { - // This should be impossible; `orig_subject` (aka origTopic) is - // documented to be present when either the stream or topic changed. - debugLog('Malformed UpdateMessageEvent: stream move but no origTopic'); // TODO(log) - } - return true; - }()); + if (propagateMode != null) { + assert(debugLog( + 'Malformed UpdateMessageEvent: incoherent message-move fields; ' + 'propagate_mode present but no new channel or topic')); // TODO(log) + } return; } - if (origStreamId == null) { + if (origStreamId == null || newStreamId == null) { // The `stream_id` field (aka origStreamId) is documented to be present on moves. + // newStreamId should not be null either because it falls back to origStreamId. assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log) return; } + if (origTopic == null || newTopic == null) { + // The `orig_subject` field (aka origTopic) is documented to be present on moves. + // newTopic should not be null either because it falls back to origTopic. + assert(debugLog('Malformed UpdateMessageEvent: move but no origTopic')); // TODO(log) + return; + } if (propagateMode == null) { // The `propagate_mode` field (aka propagateMode) is documented to be present on moves. assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log) return; } - final newStreamId = event.newStreamId ?? origStreamId; - final newTopic = event.newTopic ?? origTopic; - if (newStreamId == origStreamId && newTopic == origTopic) { - // If neither the channel nor topic name changed, nothing moved. - // In that case `orig_subject` (aka origTopic) should have been null. - assert(debugLog('Malformed UpdateMessageEvent: move but no newStreamId or newTopic')); // TODO(log) - return; - } - final wasResolveOrUnresolve = newStreamId == origStreamId && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic); From 6c3ee145119cb5fd00327451170073ba85bb5558 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 27 Jan 2025 18:05:21 -0500 Subject: [PATCH 06/10] api: Extract and parse UpdateMessageMoveData from UpdateMessageEvent This is mostly NFC except that we were logging and ignoring malformed event data before, and now start to throw errors within the parsing code. This data structure encapsulates some checks so that we can make all fields non-nullable, with reasonable fallback values. As of writing, we do not use origStreamId (a.k.a.: 'stream_id') when there was no message move, even though it is present if there were content edits This makes dropping 'stream_id' when parsing `moveData` into `null` acceptable for now. Signed-off-by: Zixuan James Li --- lib/api/model/events.dart | 98 ++++++++++++++++++++++++++----- lib/api/model/events.g.dart | 27 +-------- lib/model/message.dart | 45 ++------------ lib/model/message_list.dart | 9 ++- test/api/model/events_checks.dart | 14 +++-- test/api/model/events_test.dart | 82 +++++++++++++++++++++++++- test/api/model/model_checks.dart | 1 + test/example_data.dart | 18 +++--- 8 files changed, 193 insertions(+), 101 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index 9faa3d367e..b756804dcc 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -718,16 +718,8 @@ class UpdateMessageEvent extends Event { // final String? streamName; // ignore - @JsonKey(name: 'stream_id') - final int? origStreamId; - final int? newStreamId; - - final PropagateMode? propagateMode; - - @JsonKey(name: 'orig_subject') - final TopicName? origTopic; - @JsonKey(name: 'subject') - final TopicName? newTopic; + @JsonKey(readValue: _readMoveData, fromJson: UpdateMessageMoveData.tryParseFromJson, includeToJson: false) + final UpdateMessageMoveData? moveData; // final List topicLinks; // TODO handle @@ -747,11 +739,7 @@ class UpdateMessageEvent extends Event { required this.messageIds, required this.flags, required this.editTimestamp, - required this.origStreamId, - required this.newStreamId, - required this.propagateMode, - required this.origTopic, - required this.newTopic, + required this.moveData, required this.origContent, required this.origRenderedContent, required this.content, @@ -759,6 +747,12 @@ class UpdateMessageEvent extends Event { required this.isMeMessage, }); + static Map _readMoveData(Map json, String key) { + // Parsing [UpdateMessageMoveData] requires `json`, not the default `json[key]`. + assert(json is Map); // value came through `fromJson` with this type + return json as Map; + } + factory UpdateMessageEvent.fromJson(Map json) => _$UpdateMessageEventFromJson(json); @@ -766,6 +760,80 @@ class UpdateMessageEvent extends Event { Map toJson() => _$UpdateMessageEventToJson(this); } +/// Data structure representing a message move. +class UpdateMessageMoveData { + final int origStreamId; + final int newStreamId; + final TopicName origTopic; + final TopicName newTopic; + final PropagateMode propagateMode; + + UpdateMessageMoveData({ + required this.origStreamId, + required this.newStreamId, + required this.origTopic, + required this.newTopic, + required this.propagateMode, + }) : assert(newStreamId != origStreamId || newTopic != origTopic); + + /// Try to extract [UpdateMessageMoveData] from the JSON object for an + /// [UpdateMessageEvent]. + /// + /// Returns `null` if there was no message move. + /// + /// Throws an error if the data is malformed. + // When parsing this, 'stream_id', which is also present when there was only + // a content edit, cannot be recovered if this ends up returning `null`. + // This may matter if we ever need 'stream_id' when no message move occurred. + static UpdateMessageMoveData? tryParseFromJson(Map json) { + final origStreamId = (json['stream_id'] as num?)?.toInt(); + final newStreamIdRaw = (json['new_stream_id'] as num?)?.toInt(); + final newStreamId = newStreamIdRaw ?? origStreamId; + + final origTopic = json['orig_subject'] == null ? null + : TopicName.fromJson(json['orig_subject'] as String); + final newTopicRaw = json['subject'] == null ? null + : TopicName.fromJson(json['subject'] as String); + final newTopic = newTopicRaw ?? origTopic; + + final propagateModeString = json['propagate_mode'] as String?; + final propagateMode = propagateModeString == null ? null + : PropagateMode.fromRawString(propagateModeString); + + if (newStreamId == origStreamId && newTopic == origTopic) { + if (propagateMode != null) { + throw FormatException( + 'Malformed UpdateMessageEvent: incoherent message-move fields; ' + 'propagate_mode present but no new channel or topic'); + } + return null; + } + + if (origStreamId == null || newStreamId == null) { + // The `stream_id` field (aka origStreamId) is documented to be present on moves; + // newStreamId should not be null either because it falls back to origStreamId. + throw FormatException('Malformed UpdateMessageEvent: move but no origStreamId'); + } + if (origTopic == null || newTopic == null) { + // The `orig_subject` field (aka origTopic) is documented to be present on moves; + // newTopic should not be null either because it falls back to origTopic. + throw FormatException('Malformed UpdateMessageEvent: move but no origTopic'); + } + if (propagateMode == null) { + // The `propagate_mode` field (aka propagateMode) is documented to be present on moves. + throw FormatException('Malformed UpdateMessageEvent: move but no propagateMode'); + } + + return UpdateMessageMoveData( + origStreamId: origStreamId, + newStreamId: newStreamId, + origTopic: origTopic, + newTopic: newTopic, + propagateMode: propagateMode, + ); + } +} + /// A Zulip event of type `delete_message`: https://zulip.com/api/get-events#delete_message @JsonSerializable(fieldRename: FieldRename.snake) class DeleteMessageEvent extends Event { diff --git a/lib/api/model/events.g.dart b/lib/api/model/events.g.dart index d0b0cc7b1b..c5b56d013a 100644 --- a/lib/api/model/events.g.dart +++ b/lib/api/model/events.g.dart @@ -443,20 +443,10 @@ UpdateMessageEvent _$UpdateMessageEventFromJson(Map json) => .map((e) => $enumDecode(_$MessageFlagEnumMap, e)) .toList(), editTimestamp: (json['edit_timestamp'] as num?)?.toInt(), - origStreamId: (json['stream_id'] as num?)?.toInt(), - newStreamId: (json['new_stream_id'] as num?)?.toInt(), - propagateMode: $enumDecodeNullable( - _$PropagateModeEnumMap, - json['propagate_mode'], + moveData: UpdateMessageMoveData.tryParseFromJson( + UpdateMessageEvent._readMoveData(json, 'move_data') + as Map, ), - origTopic: - json['orig_subject'] == null - ? null - : TopicName.fromJson(json['orig_subject'] as String), - newTopic: - json['subject'] == null - ? null - : TopicName.fromJson(json['subject'] as String), origContent: json['orig_content'] as String?, origRenderedContent: json['orig_rendered_content'] as String?, content: json['content'] as String?, @@ -474,11 +464,6 @@ Map _$UpdateMessageEventToJson(UpdateMessageEvent instance) => 'message_ids': instance.messageIds, 'flags': instance.flags, 'edit_timestamp': instance.editTimestamp, - 'stream_id': instance.origStreamId, - 'new_stream_id': instance.newStreamId, - 'propagate_mode': instance.propagateMode, - 'orig_subject': instance.origTopic, - 'subject': instance.newTopic, 'orig_content': instance.origContent, 'orig_rendered_content': instance.origRenderedContent, 'content': instance.content, @@ -497,12 +482,6 @@ const _$MessageFlagEnumMap = { MessageFlag.unknown: 'unknown', }; -const _$PropagateModeEnumMap = { - PropagateMode.changeOne: 'change_one', - PropagateMode.changeLater: 'change_later', - PropagateMode.changeAll: 'change_all', -}; - DeleteMessageEvent _$DeleteMessageEventFromJson(Map json) => DeleteMessageEvent( id: (json['id'] as num).toInt(), diff --git a/lib/model/message.dart b/lib/model/message.dart index d32918b7e5..d1886b810f 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -169,42 +169,14 @@ class MessageStoreImpl with MessageStore { } void _handleUpdateMessageEventMove(UpdateMessageEvent event) { - // The interaction between the fields of these events are a bit tricky. - // For reference, see: https://zulip.com/api/get-events#update_message - - final origStreamId = event.origStreamId; - final newStreamId = event.newStreamId ?? origStreamId; - final origTopic = event.origTopic; - final newTopic = event.newTopic ?? origTopic; - final propagateMode = event.propagateMode; - - if (newStreamId == origStreamId && newTopic == origTopic) { + final messageMove = event.moveData; + if (messageMove == null) { // There was no move. - if (propagateMode != null) { - assert(debugLog( - 'Malformed UpdateMessageEvent: incoherent message-move fields; ' - 'propagate_mode present but no new channel or topic')); // TODO(log) - } return; } - if (origStreamId == null || newStreamId == null) { - // The `stream_id` field (aka origStreamId) is documented to be present on moves. - // newStreamId should not be null either because it falls back to origStreamId. - assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log) - return; - } - if (origTopic == null || newTopic == null) { - // The `orig_subject` field (aka origTopic) is documented to be present on moves. - // newTopic should not be null either because it falls back to origTopic. - assert(debugLog('Malformed UpdateMessageEvent: move but no origTopic')); // TODO(log) - return; - } - if (propagateMode == null) { - // The `propagate_mode` field (aka propagateMode) is documented to be present on moves. - assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log) - return; - } + final UpdateMessageMoveData( + :origStreamId, :newStreamId, :origTopic, :newTopic) = messageMove; final wasResolveOrUnresolve = newStreamId == origStreamId && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic); @@ -236,14 +208,7 @@ class MessageStoreImpl with MessageStore { } for (final view in _messageListViews) { - view.messagesMoved( - origStreamId: origStreamId, - newStreamId: newStreamId, - origTopic: origTopic, - newTopic: newTopic, - messageIds: event.messageIds, - propagateMode: propagateMode, - ); + view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds); } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index ea120cd72e..a1df255cb2 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -720,13 +720,12 @@ class MessageListView with ChangeNotifier, _MessageSequence { } void messagesMoved({ - required int origStreamId, - required int newStreamId, - required TopicName origTopic, - required TopicName newTopic, + required UpdateMessageMoveData messageMove, required List messageIds, - required PropagateMode propagateMode, }) { + final UpdateMessageMoveData( + :origStreamId, :newStreamId, :origTopic, :newTopic, :propagateMode, + ) = messageMove; switch (narrow) { case DmNarrow(): // DMs can't be moved (nor created by moves), diff --git a/test/api/model/events_checks.dart b/test/api/model/events_checks.dart index c1fa0a117f..12d74479de 100644 --- a/test/api/model/events_checks.dart +++ b/test/api/model/events_checks.dart @@ -48,11 +48,7 @@ extension UpdateMessageEventChecks on Subject { Subject> get messageIds => has((e) => e.messageIds, 'messageIds'); Subject> get flags => has((e) => e.flags, 'flags'); Subject get editTimestamp => has((e) => e.editTimestamp, 'editTimestamp'); - Subject get origStreamId => has((e) => e.origStreamId, 'origStreamId'); - Subject get newStreamId => has((e) => e.newStreamId, 'newStreamId'); - Subject get propagateMode => has((e) => e.propagateMode, 'propagateMode'); - Subject get origTopic => has((e) => e.origTopic, 'origTopic'); - Subject get newTopic => has((e) => e.newTopic, 'newTopic'); + Subject get moveData => has((e) => e.moveData, 'moveData'); Subject get origContent => has((e) => e.origContent, 'origContent'); Subject get origRenderedContent => has((e) => e.origRenderedContent, 'origRenderedContent'); Subject get content => has((e) => e.content, 'content'); @@ -60,6 +56,14 @@ extension UpdateMessageEventChecks on Subject { Subject get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage'); } +extension UpdateMessageMoveDataChecks on Subject { + Subject get origStreamId => has((e) => e.origStreamId, 'origStreamId'); + Subject get newStreamId => has((e) => e.newStreamId, 'newStreamId'); + Subject get origTopic => has((e) => e.origTopic, 'origTopic'); + Subject get newTopic => has((e) => e.newTopic, 'newTopic'); + Subject get propagateMode => has((e) => e.propagateMode, 'propagateMode'); +} + extension DeleteMessageEventChecks on Subject { Subject get messageType => has((e) => e.messageType, 'messageType'); } diff --git a/test/api/model/events_test.dart b/test/api/model/events_test.dart index a0d52257f8..7cdfa94eff 100644 --- a/test/api/model/events_test.dart +++ b/test/api/model/events_test.dart @@ -106,11 +106,26 @@ void main() { 'propagate_mode': 'change_all', }; + test('smoke moveData', () { + check(Event.fromJson({ ...baseMoveJson, + 'stream_id': 1, + 'new_stream_id': 2, + 'orig_subject': 'foo', + 'subject': 'bar', + 'propagate_mode': 'change_all', + })).isA().moveData.isNotNull() + ..origStreamId.equals(1) + ..newStreamId.equals(2) + ..origTopic.equals(const TopicName('foo')) + ..newTopic.equals(const TopicName('bar')) + ..propagateMode.equals(PropagateMode.changeAll); + }); + test('stream_id -> origStreamId', () { check(Event.fromJson({ ...baseMoveJson, 'stream_id': 1, 'new_stream_id': 2, - })).isA() + })).isA().moveData.isNotNull() ..origStreamId.equals(1) ..newStreamId.equals(2); }); @@ -119,10 +134,73 @@ void main() { check(Event.fromJson({ ...baseMoveJson, 'orig_subject': 'foo', 'subject': 'bar', - })).isA() + })).isA().moveData.isNotNull() ..origTopic.equals(const TopicName('foo')) ..newTopic.equals(const TopicName('bar')); }); + + test('new channel, same topic: fill in newTopic', () { + // The server omits 'subject' in this situation. + check(Event.fromJson({ ...baseMoveJson, + 'stream_id': 1, + 'new_stream_id': 2, + 'orig_subject': 'foo', + 'subject': null, + })).isA().moveData.isNotNull() + ..origTopic.equals(const TopicName('foo')) + ..newTopic.equals(const TopicName('foo')); + }); + + test('same channel, new topic; fill in newStreamId', () { + // The server omits 'new_stream_id' in this situation. + check(Event.fromJson({ ...baseMoveJson, + 'stream_id': 1, + 'new_stream_id': null, + 'orig_subject': 'foo', + 'subject': 'bar', + })).isA().moveData.isNotNull() + ..origStreamId.equals(1) + ..newStreamId.equals(1); + }); + + test('no message move', () { + check(Event.fromJson({ ...baseJson, + 'orig_content': 'foo', + 'orig_rendered_content': 'foo', + 'content': 'bar', + 'rendered_content': 'bar', + })).isA().moveData.isNull(); + }); + + test('stream move but no orig_subject', () { + check(() => Event.fromJson({ ...baseMoveJson, + 'stream_id': 1, + 'new_stream_id': 2, + 'orig_subject': null, + })).throws(); + }); + + test('move but no subject or new_stream_id', () { + check(() => Event.fromJson({ ...baseMoveJson, + 'new_stream_id': null, + 'subject': null, + })).throws(); + }); + + test('move but no orig_stream_id', () { + check(() => Event.fromJson({ ...baseMoveJson, + 'stream_id': null, + 'new_stream_id': 2, + })).throws(); + }); + + test('move but no propagate_mode', () { + check(() => Event.fromJson({ ...baseMoveJson, + 'orig_subject': 'foo', + 'subject': 'bar', + 'propagate_mode': null, + })).throws(); + }); }); test('delete_message: require streamId and topic for stream messages', () { diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 8b39b1ad57..77995466ce 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -53,6 +53,7 @@ extension TopicNameChecks on Subject { extension StreamMessageChecks on Subject { Subject get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient'); + Subject get streamId => has((e) => e.streamId, 'streamId'); Subject get topic => has((e) => e.topic, 'topic'); } diff --git a/test/example_data.dart b/test/example_data.dart index 955f5dacfb..0d76f98d4e 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -655,11 +655,7 @@ UpdateMessageEvent updateMessageEditEvent( messageIds: [messageId], flags: flags ?? origMessage.flags, editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp - origStreamId: origMessage is StreamMessage ? origMessage.streamId : null, - newStreamId: null, - propagateMode: null, - origTopic: null, - newTopic: null, + moveData: null, origContent: 'some probably-mismatched old Markdown', origRenderedContent: origMessage.content, content: 'some probably-mismatched new Markdown', @@ -692,11 +688,13 @@ UpdateMessageEvent _updateMessageMoveEvent( messageIds: messageIds, flags: flags, editTimestamp: 1234567890, // TODO generate timestamp - origStreamId: origStreamId, - newStreamId: newStreamId, - propagateMode: propagateMode, - origTopic: origTopic, - newTopic: newTopic, + moveData: UpdateMessageMoveData( + origStreamId: origStreamId, + newStreamId: newStreamId ?? origStreamId, + origTopic: origTopic, + newTopic: newTopic ?? origTopic, + propagateMode: propagateMode, + ), origContent: origContent, origRenderedContent: origContent, content: newContent, From 0e4f455e11c76e22d00ecc5428898bc0c7d11bbb Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 12 Mar 2025 17:13:39 -0400 Subject: [PATCH 07/10] api [nfc]: Simplify move data parsing with null-checks Signed-off-by: Zixuan James Li --- lib/api/model/events.dart | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index b756804dcc..985f5b5803 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -809,27 +809,19 @@ class UpdateMessageMoveData { return null; } - if (origStreamId == null || newStreamId == null) { + return UpdateMessageMoveData( // The `stream_id` field (aka origStreamId) is documented to be present on moves; // newStreamId should not be null either because it falls back to origStreamId. - throw FormatException('Malformed UpdateMessageEvent: move but no origStreamId'); - } - if (origTopic == null || newTopic == null) { + origStreamId: origStreamId!, + newStreamId: newStreamId!, + // The `orig_subject` field (aka origTopic) is documented to be present on moves; // newTopic should not be null either because it falls back to origTopic. - throw FormatException('Malformed UpdateMessageEvent: move but no origTopic'); - } - if (propagateMode == null) { - // The `propagate_mode` field (aka propagateMode) is documented to be present on moves. - throw FormatException('Malformed UpdateMessageEvent: move but no propagateMode'); - } + origTopic: origTopic!, + newTopic: newTopic!, - return UpdateMessageMoveData( - origStreamId: origStreamId, - newStreamId: newStreamId, - origTopic: origTopic, - newTopic: newTopic, - propagateMode: propagateMode, + // The `propagate_mode` field (aka propagateMode) is documented to be present on moves. + propagateMode: propagateMode!, ); } } From 5dc21f73ff69763d70ba2627ee54d639f0dcabd2 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 19 Feb 2025 21:00:57 -0500 Subject: [PATCH 08/10] example data [nfc]: Remove incorrect assertion for message move This does not correctly catch the case when `origTopic='topic'`, and `newTopic=origStreamId=newStreamId=null`. Instead, remove it and rely on the assertions from UpdateMessageMoveData's constructor. Signed-off-by: Zixuan James Li --- test/example_data.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 0d76f98d4e..cfb333baec 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -677,8 +677,6 @@ UpdateMessageEvent _updateMessageMoveEvent( }) { _checkPositive(origStreamId, 'stream ID'); _checkPositive(newStreamId, 'stream ID'); - assert(newTopic != origTopic - || (newStreamId != null && newStreamId != origStreamId)); assert(messageIds.isNotEmpty); return UpdateMessageEvent( id: 0, From 98d6ec1d866218b96c19fd9d1e4bf0fe3ecae271 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 28 Jan 2025 14:07:14 -0500 Subject: [PATCH 09/10] unreads [nfc]: Add assertions for _addAllInStreamTopic Signed-off-by: Zixuan James Li --- lib/model/unreads.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index 1fcea3f83c..1e997ea67e 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -455,6 +455,8 @@ class Unreads extends ChangeNotifier { // [messageIds] must be sorted ascending and without duplicates. void _addAllInStreamTopic(QueueList messageIds, int streamId, TopicName topic) { + assert(messageIds.isNotEmpty); + assert(isSortedWithoutDuplicates(messageIds)); final topics = streams[streamId] ??= {}; topics.update(topic, ifAbsent: () => messageIds, From 34e6201fa2fada44854d6094b80cae3394496286 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 27 Jan 2025 19:56:16 -0500 Subject: [PATCH 10/10] unreads: Handle updates when there are moved messages We could have extend `_removeAllInStreamTopic` to fit the needs of handling moved unreads, but it is used in a code path where messages are marked as read (which will become hotter after #81). The new helper is designed to avoid making copies of message IDs when possible. As for the name, we use "pop" (inspired by Python) to indicate that the removed items are returned, since "removeAll" in Dart doesn't carry that meaning. Signed-off-by: Zixuan James Li --- lib/model/unreads.dart | 88 ++++++++++++++++-- test/model/unreads_test.dart | 169 +++++++++++++++++++++++++++++++++++ 2 files changed, 252 insertions(+), 5 deletions(-) diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index 1e997ea67e..34b4492013 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -259,10 +259,8 @@ class Unreads extends ChangeNotifier { (f) => f == MessageFlag.mentioned || f == MessageFlag.wildcardMentioned, ); - // We assume this event can't signal a change in a message's 'read' flag. - // TODO can it actually though, when it's about messages being moved into an - // unsubscribed stream? - // https://chat.zulip.org/#narrow/stream/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/1639957 + // We expect the event's 'read' flag to be boring, + // matching the message's local unread state. final bool isRead = event.flags.contains(MessageFlag.read); assert(() { final isUnreadLocally = isUnread(messageId); @@ -272,6 +270,17 @@ class Unreads extends ChangeNotifier { // We were going to check something but can't; shrug. if (isUnreadLocally == null) return true; + final newChannelId = event.moveData?.newStreamId; + if (newChannelId != null && !channelStore.subscriptions.containsKey(newChannelId)) { + // When unread messages are moved to an unsubscribed channel, the server + // marks them as read without sending a mark-as-read event. Clients are + // asked to special-case this by marking them as read, which we do in + // _handleMessageMove. That contract is clear enough and doesn't involve + // this event's 'read' flag, so don't bother logging about the flag; + // its behavior seems like an implementation detail that could change. + return true; + } + if (isUnreadLocally != isUnreadInEvent) { // If this happens, then either: // - the server and client have been out of sync about the message's @@ -296,13 +305,39 @@ class Unreads extends ChangeNotifier { madeAnyUpdate |= mentions.add(messageId); } - // TODO(#901) handle moved messages + madeAnyUpdate |= _handleMessageMove(event); if (madeAnyUpdate) { notifyListeners(); } } + bool _handleMessageMove(UpdateMessageEvent event) { + if (event.moveData == null) { + // No moved messages. + return false; + } + final UpdateMessageMoveData( + :origStreamId, :newStreamId, :origTopic, :newTopic) = event.moveData!; + + final messageToMoveIds = _popAllInStreamTopic( + event.messageIds.toSet(), origStreamId, origTopic)?..sort(); + + if (messageToMoveIds == null || messageToMoveIds.isEmpty) return false; + assert(event.messageIds.toSet().containsAll(messageToMoveIds)); + + if (!channelStore.subscriptions.containsKey(newStreamId)) { + // Unreads moved to an unsubscribed channel; just drop them. + // See also: + // https://chat.zulip.org/#narrow/channel/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/2101926 + return true; + } + + _addAllInStreamTopic(messageToMoveIds, newStreamId, newTopic); + + return true; + } + void handleDeleteMessageEvent(DeleteMessageEvent event) { mentions.removeAll(event.messageIds); final messageIdsSet = Set.of(event.messageIds); @@ -506,6 +541,49 @@ class Unreads extends ChangeNotifier { } } + /// Remove unread stream messages contained in `incomingMessageIds`, with + /// the matching `streamId` and `topic`. + /// + /// Returns the removed message IDs, or `null` if no messages are affected. + /// + /// Use [_removeAllInStreamTopic] if the removed message IDs are not needed. + // Part of this is adapted from [ListBase.removeWhere]. + QueueList? _popAllInStreamTopic(Set incomingMessageIds, int streamId, TopicName topic) { + final topics = streams[streamId]; + if (topics == null) return null; + final messageIds = topics[topic]; + if (messageIds == null) return null; + + final retainedMessageIds = messageIds.whereNot( + (id) => incomingMessageIds.contains(id)).toList(); + + if (retainedMessageIds.isEmpty) { + // This is an optimization for the case when all messages in the + // conversation are removed, which avoids making a copy of `messageIds` + // unnecessarily. + topics.remove(topic); + if (topics.isEmpty) { + streams.remove(streamId); + } + return messageIds; + } + + QueueList? poppedMessageIds; + if (retainedMessageIds.length != messageIds.length) { + poppedMessageIds = QueueList.from( + messageIds.where((id) => incomingMessageIds.contains(id))); + messageIds.setRange(0, retainedMessageIds.length, retainedMessageIds); + messageIds.length = retainedMessageIds.length; + } + if (messageIds.isEmpty) { + topics.remove(topic); + if (topics.isEmpty) { + streams.remove(streamId); + } + } + return poppedMessageIds; + } + // TODO use efficient model lookups bool _slowIsPresentInDms(int messageId) { return dms.values.any((ids) => ids.contains(messageId)); diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 40e074dcaa..9a2e899ba7 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -469,6 +469,175 @@ void main() { } } }); + + group('moves', () { + final origChannel = eg.stream(); + const origTopic = 'origTopic'; + const newTopic = 'newTopic'; + + late List readMessages; + late List unreadMessages; + + Future prepareStore() async { + prepare(); + await channelStore.addStream(origChannel); + await channelStore.addSubscription(eg.subscription(origChannel)); + readMessages = List.generate(10, + (_) => eg.streamMessage(stream: origChannel, topic: origTopic, + flags: [MessageFlag.read])); + unreadMessages = List.generate(10, + (_) => eg.streamMessage(stream: origChannel, topic: origTopic)); + } + + List copyMessagesWith(Iterable messages, { + ZulipStream? newChannel, + String? newTopic, + }) { + assert(newChannel != null || newTopic != null); + return messages.map((message) => StreamMessage.fromJson( + message.toJson() + ..['stream_id'] = newChannel?.streamId ?? message.streamId + ..['subject'] = newTopic ?? message.topic + )).toList(); + } + + test('moved messages = unread messages', () async { + await prepareStore(); + final newChannel = eg.stream(); + await channelStore.addStream(newChannel); + await channelStore.addSubscription(eg.subscription(newChannel)); + fillWithMessages(unreadMessages); + final originalMessageIds = + model.streams[origChannel.streamId]![TopicName(origTopic)]!; + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: unreadMessages, + newStreamId: newChannel.streamId, + newTopicStr: newTopic)); + checkNotifiedOnce(); + checkMatchesMessages(copyMessagesWith(unreadMessages, + newChannel: newChannel, newTopic: newTopic)); + final newMessageIds = + model.streams[newChannel.streamId]![TopicName(newTopic)]!; + // Check we successfully avoided making a copy of the list. + check(originalMessageIds).identicalTo(newMessageIds); + }); + + test('moved messages ⊂ read messages', () async { + await prepareStore(); + final messagesToMove = readMessages.take(2).toList(); + fillWithMessages(unreadMessages + readMessages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: newTopic)); + checkNotNotified(); + checkMatchesMessages(unreadMessages); + }); + + test('moved messages ⊂ unread messages', () async { + await prepareStore(); + final messagesToMove = unreadMessages.take(2).toList(); + fillWithMessages(unreadMessages + readMessages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: newTopic)); + checkNotifiedOnce(); + checkMatchesMessages([ + ...copyMessagesWith(messagesToMove, newTopic: newTopic), + ...unreadMessages.skip(2), + ]); + }); + + test('moved messages ∩ unread messages ≠ Ø, moved messages ∩ read messages ≠ Ø, moved messages ⊅ unread messages', () async { + await prepareStore(); + final messagesToMove = [unreadMessages.first, readMessages.first]; + fillWithMessages(unreadMessages + readMessages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: newTopic)); + checkNotifiedOnce(); + checkMatchesMessages([ + ...copyMessagesWith(unreadMessages.take(1), newTopic: newTopic), + ...unreadMessages.skip(1), + ]); + }); + + test('moved messages ⊃ unread messages', () async { + await prepareStore(); + final messagesToMove = unreadMessages + readMessages.take(2).toList(); + fillWithMessages(unreadMessages + readMessages); + final originalMessageIds = + model.streams[origChannel.streamId]![TopicName(origTopic)]!; + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: newTopic)); + checkNotifiedOnce(); + checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic)); + final newMessageIds = + model.streams[origChannel.streamId]![TopicName(newTopic)]!; + // Check we successfully avoided making a copy of the list. + check(originalMessageIds).identicalTo(newMessageIds); + }); + + test('moving to unsubscribed channels drops the unreads', () async { + await prepareStore(); + final unsubscribedChannel = eg.stream(); + await channelStore.addStream(unsubscribedChannel); + assert(!channelStore.subscriptions.containsKey( + unsubscribedChannel.streamId)); + fillWithMessages(unreadMessages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: unreadMessages, + newStreamId: unsubscribedChannel.streamId)); + checkNotifiedOnce(); + checkMatchesMessages([]); + }); + + test('tolerates unsorted messages', () async { + await prepareStore(); + final unreadMessages = List.generate(10, (i) => + eg.streamMessage( + id: 1000 - i, stream: origChannel, topic: origTopic)); + fillWithMessages(unreadMessages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: unreadMessages, + newTopicStr: newTopic)); + checkNotifiedOnce(); + checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic)); + }); + + test('tolerates unreads unknown to the model', () async { + await prepareStore(); + fillWithMessages(unreadMessages); + + final unknownChannel = eg.stream(); + assert(!channelStore.streams.containsKey(unknownChannel.streamId)); + final unknownUnreadMessage = eg.streamMessage( + stream: unknownChannel, topic: origTopic); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: [unknownUnreadMessage], + newTopicStr: newTopic)); + checkNotNotified(); + checkMatchesMessages(unreadMessages); + }); + + test('message edit but no move', () async { + await prepareStore(); + fillWithMessages(unreadMessages); + + model.handleUpdateMessageEvent(eg.updateMessageEditEvent( + unreadMessages.first)); + checkNotNotified(); + checkMatchesMessages(unreadMessages); + }); + }); });