Skip to content

Commit 0a8347f

Browse files
PIG208PIG208
committed
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 <[email protected]>
1 parent 1c850f9 commit 0a8347f

File tree

2 files changed

+252
-5
lines changed

2 files changed

+252
-5
lines changed

‎lib/model/unreads.dart‎

Lines changed: 83 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -259,10 +259,8 @@ class Unreads extends ChangeNotifier {
259259
(f) => f == MessageFlag.mentioned || f == MessageFlag.wildcardMentioned,
260260
);
261261

262-
// We assume this event can't signal a change in a message's 'read' flag.
263-
// TODO can it actually though, when it's about messages being moved into an
264-
// unsubscribed stream?
265-
// https://chat.zulip.org/#narrow/stream/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/1639957
262+
// We expect the event's 'read' flag to be boring,
263+
// matching the message's local unread state.
266264
final bool isRead = event.flags.contains(MessageFlag.read);
267265
assert(() {
268266
final isUnreadLocally = isUnread(messageId);
@@ -272,6 +270,17 @@ class Unreads extends ChangeNotifier {
272270
// We were going to check something but can't; shrug.
273271
if (isUnreadLocally == null) return true;
274272

273+
final newChannelId = event.moveData?.newStreamId;
274+
if (newChannelId != null && !channelStore.subscriptions.containsKey(newChannelId)) {
275+
// When unread messages are moved to an unsubscribed channel, the server
276+
// marks them as read without sending a mark-as-read event. Clients are
277+
// asked to special-case this by marking them as read, which we do in
278+
// _handleMessageMove. That contract is clear enough and doesn't involve
279+
// this event's 'read' flag, so don't bother logging about the flag;
280+
// its behavior seems like an implementation detail that could change.
281+
return true;
282+
}
283+
275284
if (isUnreadLocally != isUnreadInEvent) {
276285
// If this happens, then either:
277286
// - the server and client have been out of sync about the message's
@@ -296,13 +305,39 @@ class Unreads extends ChangeNotifier {
296305
madeAnyUpdate |= mentions.add(messageId);
297306
}
298307

299-
// TODO(#901) handle moved messages
308+
madeAnyUpdate |= _handleMessageMove(event);
300309

301310
if (madeAnyUpdate) {
302311
notifyListeners();
303312
}
304313
}
305314

315+
bool _handleMessageMove(UpdateMessageEvent event) {
316+
if (event.moveData == null) {
317+
// No moved messages.
318+
return false;
319+
}
320+
final UpdateMessageMoveData(
321+
:origStreamId, :newStreamId, :origTopic, :newTopic) = event.moveData!;
322+
323+
final messageToMoveIds = _popAllInStreamTopic(
324+
event.messageIds.toSet(), origStreamId, origTopic)?..sort();
325+
326+
if (messageToMoveIds == null || messageToMoveIds.isEmpty) return false;
327+
assert(event.messageIds.toSet().containsAll(messageToMoveIds));
328+
329+
if (!channelStore.subscriptions.containsKey(newStreamId)) {
330+
// Unreads moved to an unsubscribed channel; just drop them.
331+
// See also:
332+
// https://chat.zulip.org/#narrow/channel/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/2101926
333+
return true;
334+
}
335+
336+
_addAllInStreamTopic(messageToMoveIds, newStreamId, newTopic);
337+
338+
return true;
339+
}
340+
306341
void handleDeleteMessageEvent(DeleteMessageEvent event) {
307342
mentions.removeAll(event.messageIds);
308343
final messageIdsSet = Set.of(event.messageIds);
@@ -506,6 +541,49 @@ class Unreads extends ChangeNotifier {
506541
}
507542
}
508543

544+
/// Remove unread stream messages contained in `incomingMessageIds`, with
545+
/// the matching `streamId` and `topic`.
546+
///
547+
/// Returns the removed message IDs, or `null` if no messages are affected.
548+
///
549+
/// Use [_removeAllInStreamTopic] if the removed message IDs are not needed.
550+
// Part of this is adapted from [ListBase.removeWhere].
551+
QueueList<int>? _popAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
552+
final topics = streams[streamId];
553+
if (topics == null) return null;
554+
final messageIds = topics[topic];
555+
if (messageIds == null) return null;
556+
557+
final retainedMessageIds = messageIds.whereNot(
558+
(id) => incomingMessageIds.contains(id)).toList();
559+
560+
if (retainedMessageIds.isEmpty) {
561+
// This is an optimization for the case when all messages in the
562+
// conversation are removed, which avoids making a copy of `messageIds`
563+
// unnecessarily.
564+
topics.remove(topic);
565+
if (topics.isEmpty) {
566+
streams.remove(streamId);
567+
}
568+
return messageIds;
569+
}
570+
571+
QueueList<int>? poppedMessageIds;
572+
if (retainedMessageIds.length != messageIds.length) {
573+
poppedMessageIds = QueueList.from(
574+
messageIds.where((id) => incomingMessageIds.contains(id)));
575+
messageIds.setRange(0, retainedMessageIds.length, retainedMessageIds);
576+
messageIds.length = retainedMessageIds.length;
577+
}
578+
if (messageIds.isEmpty) {
579+
topics.remove(topic);
580+
if (topics.isEmpty) {
581+
streams.remove(streamId);
582+
}
583+
}
584+
return poppedMessageIds;
585+
}
586+
509587
// TODO use efficient model lookups
510588
bool _slowIsPresentInDms(int messageId) {
511589
return dms.values.any((ids) => ids.contains(messageId));

‎test/model/unreads_test.dart‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,175 @@ void main() {
469469
}
470470
}
471471
});
472+
473+
group('moves', () {
474+
final origChannel = eg.stream();
475+
const origTopic = 'origTopic';
476+
const newTopic = 'newTopic';
477+
478+
late List<StreamMessage> readMessages;
479+
late List<StreamMessage> unreadMessages;
480+
481+
Future<void> prepareStore() async {
482+
prepare();
483+
await channelStore.addStream(origChannel);
484+
await channelStore.addSubscription(eg.subscription(origChannel));
485+
readMessages = List<StreamMessage>.generate(10,
486+
(_) => eg.streamMessage(stream: origChannel, topic: origTopic,
487+
flags: [MessageFlag.read]));
488+
unreadMessages = List<StreamMessage>.generate(10,
489+
(_) => eg.streamMessage(stream: origChannel, topic: origTopic));
490+
}
491+
492+
List<StreamMessage> copyMessagesWith(Iterable<StreamMessage> messages, {
493+
ZulipStream? newChannel,
494+
String? newTopic,
495+
}) {
496+
assert(newChannel != null || newTopic != null);
497+
return messages.map((message) => StreamMessage.fromJson(
498+
message.toJson()
499+
..['stream_id'] = newChannel?.streamId ?? message.streamId
500+
..['subject'] = newTopic ?? message.topic
501+
)).toList();
502+
}
503+
504+
test('moved messages = unread messages', () async {
505+
await prepareStore();
506+
final newChannel = eg.stream();
507+
await channelStore.addStream(newChannel);
508+
await channelStore.addSubscription(eg.subscription(newChannel));
509+
fillWithMessages(unreadMessages);
510+
final originalMessageIds =
511+
model.streams[origChannel.streamId]![TopicName(origTopic)]!;
512+
513+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
514+
origMessages: unreadMessages,
515+
newStreamId: newChannel.streamId,
516+
newTopicStr: newTopic));
517+
checkNotifiedOnce();
518+
checkMatchesMessages(copyMessagesWith(unreadMessages,
519+
newChannel: newChannel, newTopic: newTopic));
520+
final newMessageIds =
521+
model.streams[newChannel.streamId]![TopicName(newTopic)]!;
522+
// Check we successfully avoided making a copy of the list.
523+
check(originalMessageIds).identicalTo(newMessageIds);
524+
});
525+
526+
test('moved messages ⊂ read messages', () async {
527+
await prepareStore();
528+
final messagesToMove = readMessages.take(2).toList();
529+
fillWithMessages(unreadMessages + readMessages);
530+
531+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
532+
origMessages: messagesToMove,
533+
newTopicStr: newTopic));
534+
checkNotNotified();
535+
checkMatchesMessages(unreadMessages);
536+
});
537+
538+
test('moved messages ⊂ unread messages', () async {
539+
await prepareStore();
540+
final messagesToMove = unreadMessages.take(2).toList();
541+
fillWithMessages(unreadMessages + readMessages);
542+
543+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
544+
origMessages: messagesToMove,
545+
newTopicStr: newTopic));
546+
checkNotifiedOnce();
547+
checkMatchesMessages([
548+
...copyMessagesWith(messagesToMove, newTopic: newTopic),
549+
...unreadMessages.skip(2),
550+
]);
551+
});
552+
553+
test('moved messages ∩ unread messages ≠ Ø, moved messages ∩ read messages ≠ Ø, moved messages ⊅ unread messages', () async {
554+
await prepareStore();
555+
final messagesToMove = [unreadMessages.first, readMessages.first];
556+
fillWithMessages(unreadMessages + readMessages);
557+
558+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
559+
origMessages: messagesToMove,
560+
newTopicStr: newTopic));
561+
checkNotifiedOnce();
562+
checkMatchesMessages([
563+
...copyMessagesWith(unreadMessages.take(1), newTopic: newTopic),
564+
...unreadMessages.skip(1),
565+
]);
566+
});
567+
568+
test('moved messages ⊃ unread messages', () async {
569+
await prepareStore();
570+
final messagesToMove = unreadMessages + readMessages.take(2).toList();
571+
fillWithMessages(unreadMessages + readMessages);
572+
final originalMessageIds =
573+
model.streams[origChannel.streamId]![TopicName(origTopic)]!;
574+
575+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
576+
origMessages: messagesToMove,
577+
newTopicStr: newTopic));
578+
checkNotifiedOnce();
579+
checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic));
580+
final newMessageIds =
581+
model.streams[origChannel.streamId]![TopicName(newTopic)]!;
582+
// Check we successfully avoided making a copy of the list.
583+
check(originalMessageIds).identicalTo(newMessageIds);
584+
});
585+
586+
test('moving to unsubscribed channels drops the unreads', () async {
587+
await prepareStore();
588+
final unsubscribedChannel = eg.stream();
589+
await channelStore.addStream(unsubscribedChannel);
590+
assert(!channelStore.subscriptions.containsKey(
591+
unsubscribedChannel.streamId));
592+
fillWithMessages(unreadMessages);
593+
594+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
595+
origMessages: unreadMessages,
596+
newStreamId: unsubscribedChannel.streamId));
597+
checkNotifiedOnce();
598+
checkMatchesMessages([]);
599+
});
600+
601+
test('tolerates unsorted messages', () async {
602+
await prepareStore();
603+
final unreadMessages = List.generate(10, (i) =>
604+
eg.streamMessage(
605+
id: 1000 - i, stream: origChannel, topic: origTopic));
606+
fillWithMessages(unreadMessages);
607+
608+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
609+
origMessages: unreadMessages,
610+
newTopicStr: newTopic));
611+
checkNotifiedOnce();
612+
checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic));
613+
});
614+
615+
test('tolerates unreads unknown to the model', () async {
616+
await prepareStore();
617+
fillWithMessages(unreadMessages);
618+
619+
final unknownChannel = eg.stream();
620+
assert(!channelStore.streams.containsKey(unknownChannel.streamId));
621+
final unknownUnreadMessage = eg.streamMessage(
622+
stream: unknownChannel, topic: origTopic);
623+
624+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
625+
origMessages: [unknownUnreadMessage],
626+
newTopicStr: newTopic));
627+
checkNotNotified();
628+
checkMatchesMessages(unreadMessages);
629+
});
630+
631+
test('message edit but no move', () async {
632+
await prepareStore();
633+
fillWithMessages(unreadMessages);
634+
635+
model.handleUpdateMessageEvent(eg.updateMessageEditEvent(
636+
unreadMessages.first));
637+
checkNotNotified();
638+
checkMatchesMessages(unreadMessages);
639+
});
640+
});
472641
});
473642

474643

0 commit comments

Comments
 (0)