Skip to content

Commit 7381b80

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). The new helper is designed to avoid making copies of message IDs when possible. As for 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 d57cf21 commit 7381b80

File tree

2 files changed

+297
-5
lines changed

2 files changed

+297
-5
lines changed

‎lib/model/unreads.dart

Lines changed: 88 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,40 @@ 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 messageToMoveIds = _popAllInStreamTopic(
325+
event.messageIds.toSet(), origStreamId, origTopic)?..sort();
326+
327+
if (messageToMoveIds == null || messageToMoveIds.isEmpty) return false;
328+
assert(event.messageIds.toSet().containsAll(messageToMoveIds));
329+
330+
if (!channelStore.subscriptions.containsKey(newStreamId)) {
331+
// Unreads moved to an unsubscribed channel; just drop them.
332+
// See also:
333+
// https://chat.zulip.org/#narrow/channel/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/2101926
334+
return true;
335+
}
336+
337+
_addAllInStreamTopic(messageToMoveIds, newStreamId, newTopic);
338+
339+
return true;
340+
}
341+
306342
void handleDeleteMessageEvent(DeleteMessageEvent event) {
307343
mentions.removeAll(event.messageIds);
308344
final messageIdsSet = Set.of(event.messageIds);
@@ -506,6 +542,53 @@ class Unreads extends ChangeNotifier {
506542
}
507543
}
508544

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

‎test/model/unreads_test.dart

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

474683

0 commit comments

Comments
 (0)