Handle moved messages for unreads #1311
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -259,10 +259,8 @@ class Unreads extends ChangeNotifier { | |
| (f) => f == MessageFlag.mentioned || f == MessageFlag.wildcardMentioned, | ||
| ); | ||
|
|
||
| // 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); | ||
| } | ||
|
|
||
| madeAnyUpdate |= _handleMessageMove(event); | ||
|
Comment on lines
-299
to
+308
There was a problem hiding this comment. The commit message can get a "fixes" line. 🎉 There was a problem hiding this comment. We will get there soon! #901 also requires updates to recent senders data model. |
||
|
|
||
| if (madeAnyUpdate) { | ||
| notifyListeners(); | ||
| } | ||
| } | ||
|
|
||
| bool _handleMessageMove(UpdateMessageEvent event) { | ||
| if (event.moveData == null) { | ||
| // No moved messages. | ||
| return false; | ||
| } | ||
| final UpdateMessageMoveData( | ||
| :origStreamId, :newStreamId, :origTopic, :newTopic) = event.moveData!; | ||
|
|
||
|
There was a problem hiding this comment. Could say final UpdateMessageMoveData(
:origStreamId, :newStreamId, :origTopic, :newTopic) = messageMove;to avoid repetition of |
||
| final messageToMoveIds = _removeAllInStreamTopic( | ||
| event.messageIds.toSet(), origStreamId, origTopic); | ||
| if (messageToMoveIds == null || messageToMoveIds.isEmpty) { | ||
| // No known unreads affected by move; nothing to do. | ||
| return false; | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
Comment on lines
+330
to
+335
There was a problem hiding this comment. Quoting your comment above, #1311 (comment) :
Ah I think my code comment— // 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
final bool isRead = event.flags.contains(MessageFlag.read);—isn't quite accurate. The event can signal a change in a message's 'read' flag; it does in this case, because the API says it does (or anyway it will once the API doc is updated). The flag did change, on the server, and this event is the only thing that tells us about it ('signals' it). It's just that the event's 'read' flag isn't part of that signal. So I agree we should remove the TODO. But let's reword the remaining part so that it's accurate, maybe like this: - // We assume this event can't signal a change in a message's 'read' flag.
+ // We expect the event's 'read' flag to be boring,
+ // matching the message's local unread state.I think that expectation happens to hold in the moved-to-unsubscribed-channel case. But we don't need to care if it does, right—the API is clear that we should just drop the unreads—so how about adding another early return before the 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;
}There was a problem hiding this comment. Oh, I see that the move event isn't actually arriving in this case, in your testing: https://chat.zulip.org/#narrow/channel/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/2103863 I'll join that discussion. |
||
|
|
||
| _addAllInStreamTopic(messageToMoveIds..sort(), newStreamId, newTopic); | ||
| return true; | ||
| } | ||
|
|
||
| void handleDeleteMessageEvent(DeleteMessageEvent event) { | ||
| mentions.removeAll(event.messageIds); | ||
| final messageIdsSet = Set.of(event.messageIds); | ||
|
|
@@ -455,6 +490,8 @@ class Unreads extends ChangeNotifier { | |
|
|
||
| // [messageIds] must be sorted ascending and without duplicates. | ||
| void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, TopicName topic) { | ||
| assert(messageIds.isNotEmpty); | ||
| assert(isSortedWithoutDuplicates(messageIds)); | ||
| final topics = streams[streamId] ??= {}; | ||
| topics.update(topic, | ||
| ifAbsent: () => messageIds, | ||
|
|
@@ -488,20 +525,28 @@ class Unreads extends ChangeNotifier { | |
| } | ||
| } | ||
|
|
||
| QueueList<int>? _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) { | ||
| final topics = streams[streamId]; | ||
| if (topics == null) return null; | ||
| final messageIds = topics[topic]; | ||
| if (messageIds == null) return null; | ||
|
|
||
| // ([QueueList] doesn't have a `removeAll`) | ||
| final removedMessageIds = QueueList<int>(); | ||
| messageIds.removeWhere((id) { | ||
| if (incomingMessageIds.contains(id)) { | ||
| removedMessageIds.add(id); | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| if (messageIds.isEmpty) { | ||
| topics.remove(topic); | ||
| if (topics.isEmpty) { | ||
| streams.remove(streamId); | ||
| } | ||
| } | ||
| return removedMessageIds; | ||
| } | ||
|
|
||
| // TODO use efficient model lookups | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this commit the
UpdateMessageMoveDatadartdoc isn't accurate, because we still instantiate the class when there was no move, or when the fields have the correct types but are incoherent about whether there was a move (e.g.propagate_modepresent but the stream/topic fields all absent).How about:
/// Data structure holding the fields about a message move.(or similar), until the "representing a message move" becomes accurate in a later commit.