Skip to content

Commit 05b9b23

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 zulip#81). These new helpers are designed to avoid making copies of message IDs when possible. As for names, 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 855068e commit 05b9b23

File tree

2 files changed

+296
-5
lines changed

2 files changed

+296
-5
lines changed

‎lib/model/unreads.dart

+90-5
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,45 @@ 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, :propagateMode, :origTopic, :newTopic,
322+
) = event.moveData!;
323+
324+
final QueueList<int>? messageToMoveIds;
325+
if (propagateMode == PropagateMode.changeAll) {
326+
messageToMoveIds = _popStreamTopic(origStreamId, origTopic);
327+
} else {
328+
messageToMoveIds = _popAllInStreamTopic(
329+
event.messageIds.toSet(), origStreamId, origTopic)?..sort();
330+
}
331+
332+
if (messageToMoveIds == null || messageToMoveIds.isEmpty) return false;
333+
assert(event.messageIds.toSet().containsAll(messageToMoveIds));
334+
335+
if (!channelStore.subscriptions.containsKey(newStreamId)) {
336+
// Unreads moved to an unsubscribed channel; just drop them.
337+
// See also:
338+
// https://chat.zulip.org/#narrow/channel/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/2101926
339+
return true;
340+
}
341+
342+
_addAllInStreamTopic(messageToMoveIds, newStreamId, newTopic);
343+
344+
return true;
345+
}
346+
306347
void handleDeleteMessageEvent(DeleteMessageEvent event) {
307348
mentions.removeAll(event.messageIds);
308349
final messageIdsSet = Set.of(event.messageIds);
@@ -506,6 +547,50 @@ class Unreads extends ChangeNotifier {
506547
}
507548
}
508549

550+
/// Remove unread stream messages contained in `incomingMessageIds`, with
551+
/// the matching `streamId` and `topic`.
552+
///
553+
/// Returns the removed message IDs, or `null` if no messages are affected.
554+
QueueList<int>? _popAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
555+
final topics = streams[streamId];
556+
if (topics == null) return null;
557+
final messageIds = topics[topic];
558+
if (messageIds == null) return null;
559+
560+
final removedMessageIds = QueueList<int>();
561+
messageIds.removeWhere((id) {
562+
if (incomingMessageIds.contains(id)) {
563+
removedMessageIds.add(id);
564+
return true;
565+
}
566+
return false;
567+
});
568+
if (messageIds.isEmpty) {
569+
topics.remove(topic);
570+
if (topics.isEmpty) {
571+
streams.remove(streamId);
572+
}
573+
}
574+
return removedMessageIds;
575+
}
576+
577+
/// Remove all unread stream messages with the matching `streamId` and
578+
/// `topic`.
579+
///
580+
/// Returns the removed message IDs, or `null` if no messages are affected.
581+
QueueList<int>? _popStreamTopic(int streamId, TopicName topic) {
582+
final topics = streams[streamId];
583+
if (topics == null) return null;
584+
final messageIds = topics[topic];
585+
if (messageIds == null) return null;
586+
587+
topics.remove(topic);
588+
if (topics.isEmpty) {
589+
streams.remove(streamId);
590+
}
591+
return messageIds;
592+
}
593+
509594
// TODO use efficient model lookups
510595
bool _slowIsPresentInDms(int messageId) {
511596
return dms.values.any((ids) => ids.contains(messageId));

‎test/model/unreads_test.dart

+206
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,212 @@ void main() {
469469
}
470470
}
471471
});
472+
473+
group('moves', () {
474+
final origChannel = eg.stream();
475+
const origTopic = 'origTopic';
476+
const newTopic = 'newTopic';
477+
478+
Future<void> prepareStore() async {
479+
prepare();
480+
await channelStore.addStream(origChannel);
481+
await channelStore.addSubscription(eg.subscription(origChannel));
482+
}
483+
484+
group('move read messages', () {
485+
final readMessages = List<StreamMessage>.generate(10,
486+
(_) => eg.streamMessage(
487+
stream: origChannel, topic: origTopic, flags: [MessageFlag.read]));
488+
489+
test('to new channel', () async {
490+
await prepareStore();
491+
final newChannel = eg.stream();
492+
await channelStore.addStream(newChannel);
493+
await channelStore.addSubscription(eg.subscription(newChannel));
494+
fillWithMessages(readMessages);
495+
496+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
497+
origMessages: readMessages,
498+
newStreamId: newChannel.streamId));
499+
checkNotNotified();
500+
checkMatchesMessages([]);
501+
});
502+
503+
test('to new topic', () async {
504+
await prepareStore();
505+
fillWithMessages(readMessages);
506+
507+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
508+
origMessages: readMessages,
509+
newTopicStr: newTopic));
510+
checkNotNotified();
511+
checkMatchesMessages([]);
512+
});
513+
514+
test('from topic with unreads', () async {
515+
await prepareStore();
516+
final unreadMessage = eg.streamMessage(
517+
stream: origChannel, topic: origTopic);
518+
fillWithMessages([...readMessages, unreadMessage]);
519+
520+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
521+
origMessages: readMessages,
522+
newTopicStr: newTopic));
523+
checkNotNotified();
524+
checkMatchesMessages([unreadMessage]);
525+
});
526+
527+
test('to topic with unreads', () async {
528+
await prepareStore();
529+
final unreadMessage = eg.streamMessage(
530+
stream: origChannel, topic: newTopic);
531+
fillWithMessages([...readMessages, unreadMessage]);
532+
533+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
534+
origMessages: readMessages,
535+
newTopicStr: newTopic,
536+
));
537+
checkNotNotified();
538+
checkMatchesMessages([unreadMessage]);
539+
});
540+
});
541+
542+
group('move unread messages', () {
543+
final unreadMessages = List<StreamMessage>.generate(10,
544+
(_) => eg.streamMessage(stream: origChannel, topic: origTopic));
545+
546+
test('to another subscribed channel; same topic name', () async {
547+
await prepareStore();
548+
final newChannel = eg.stream();
549+
await channelStore.addStream(newChannel);
550+
await channelStore.addSubscription(eg.subscription(newChannel));
551+
fillWithMessages(unreadMessages);
552+
553+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
554+
origMessages: unreadMessages,
555+
newStreamId: newChannel.streamId));
556+
checkNotifiedOnce();
557+
checkMatchesMessages([
558+
for (final message in unreadMessages)
559+
Message.fromJson(
560+
message.toJson()..['stream_id'] = newChannel.streamId),
561+
]);
562+
});
563+
564+
test('to another subscribed channel; different topic name', () async {
565+
await prepareStore();
566+
final newChannel = eg.stream();
567+
await channelStore.addStream(newChannel);
568+
await channelStore.addSubscription(eg.subscription(newChannel));
569+
fillWithMessages(unreadMessages);
570+
571+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
572+
origMessages: unreadMessages,
573+
newStreamId: newChannel.streamId,
574+
newTopicStr: newTopic));
575+
checkNotifiedOnce();
576+
checkMatchesMessages([
577+
for (final message in unreadMessages)
578+
Message.fromJson(
579+
message.toJson()
580+
..['stream_id'] = newChannel.streamId
581+
..['subject'] = newTopic
582+
),
583+
]);
584+
});
585+
586+
test('to unsubscribed channel', () async {
587+
await prepareStore();
588+
final newChannel = eg.stream();
589+
await channelStore.addStream(newChannel);
590+
assert(!channelStore.subscriptions.containsKey(newChannel.streamId));
591+
fillWithMessages(unreadMessages);
592+
593+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
594+
origMessages: unreadMessages,
595+
newStreamId: newChannel.streamId));
596+
checkNotifiedOnce();
597+
checkMatchesMessages([]);
598+
});
599+
600+
test('to new topic', () async {
601+
await prepareStore();
602+
fillWithMessages(unreadMessages);
603+
604+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
605+
origMessages: unreadMessages,
606+
newTopicStr: newTopic));
607+
checkNotifiedOnce();
608+
checkMatchesMessages([
609+
for (final message in unreadMessages)
610+
Message.fromJson(message.toJson()..['subject'] = newTopic),
611+
]);
612+
});
613+
614+
test('from topic containing other unreads', () async {
615+
await prepareStore();
616+
final unreadMessage = eg.streamMessage(
617+
stream: origChannel, topic: origTopic);
618+
fillWithMessages([...unreadMessages, unreadMessage]);
619+
620+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
621+
origMessages: unreadMessages,
622+
newTopicStr: newTopic));
623+
checkNotifiedOnce();
624+
checkMatchesMessages([
625+
for (final message in unreadMessages)
626+
Message.fromJson(message.toJson()..['subject'] = newTopic),
627+
unreadMessage,
628+
]);
629+
});
630+
631+
test('to topic containing other unreads', () async {
632+
await prepareStore();
633+
final unreadMessage = eg.streamMessage(
634+
stream: origChannel, topic: newTopic);
635+
fillWithMessages([...unreadMessages, unreadMessage]);
636+
637+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
638+
origMessages: unreadMessages,
639+
newTopicStr: newTopic));
640+
checkNotifiedOnce();
641+
checkMatchesMessages([
642+
for (final message in unreadMessages)
643+
Message.fromJson(message.toJson()..['subject'] = newTopic),
644+
unreadMessage,
645+
]);
646+
});
647+
648+
test('tolerates unsorted messages', () async {
649+
await prepareStore();
650+
final unreadMessages = List.generate(10,
651+
(i) => eg.streamMessage(id: 1000-i, stream: origChannel, topic: origTopic));
652+
fillWithMessages(unreadMessages);
653+
654+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
655+
origMessages: unreadMessages,
656+
newTopicStr: newTopic));
657+
checkNotifiedOnce();
658+
checkMatchesMessages([
659+
for (final message in unreadMessages)
660+
Message.fromJson(message.toJson()..['subject'] = newTopic)
661+
]);
662+
});
663+
664+
test('tolerates unreads unknown to the model', () async {
665+
await prepareStore();
666+
final unknownUnreadMessage = eg.streamMessage(
667+
stream: eg.stream(), topic: origTopic);
668+
fillWithMessages(unreadMessages);
669+
670+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
671+
origMessages: [unknownUnreadMessage],
672+
newTopicStr: newTopic));
673+
checkNotNotified();
674+
checkMatchesMessages(unreadMessages);
675+
});
676+
});
677+
});
472678
});
473679

474680

0 commit comments

Comments
 (0)