diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index 5d4cd1471b..fdb99bff1e 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -17,12 +17,13 @@ import 'narrow.dart'; /// Callers should do their own filtering based on other state, like muting, /// as desired. /// -/// In each component of this model ([streams], [dms], [mentions]), +/// In each component of this model ([_totalCount], [streams], [dms], [mentions]), /// if a message is not represented, its status is either read /// or unknown to the component. In all components, /// a message's status will be unknown if, at /register time, /// it was very old by the server's reckoning. See [oldUnreadsMissing]. -/// It may also be unknown for other reasons that differ by component; see each. +/// In [mentions], there's another more complex reason +/// the state might be unknown; see there. /// /// Messages in unsubscribed streams, and messages sent by muted users, /// are generally deemed read by the server and shouldn't be expected to appear. @@ -39,28 +40,37 @@ import 'narrow.dart'; // messages and refresh [mentions] (see [mentions] dartdoc). class Unreads extends ChangeNotifier { factory Unreads({required UnreadMessagesSnapshot initial, required selfUserId}) { - final streams = >>{}; + int totalCount = 0; + final streams = {}; final dms = >{}; final mentions = Set.of(initial.mentions); for (final unreadStreamSnapshot in initial.streams) { final streamId = unreadStreamSnapshot.streamId; final topic = unreadStreamSnapshot.topic; - (streams[streamId] ??= {})[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds); + final streamUnreads = streams[streamId] ??= StreamUnreads.empty(); + final unreadInTopic = unreadStreamSnapshot.unreadMessageIds.length; + streamUnreads + ..topics[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds) + ..count += unreadInTopic; + totalCount += unreadInTopic; } for (final unreadDmSnapshot in initial.dms) { final otherUserId = unreadDmSnapshot.otherUserId; final narrow = DmNarrow.withUser(otherUserId, selfUserId: selfUserId); dms[narrow] = QueueList.from(unreadDmSnapshot.unreadMessageIds); + totalCount += unreadDmSnapshot.unreadMessageIds.length; } for (final unreadHuddleSnapshot in initial.huddles) { final narrow = DmNarrow.ofUnreadHuddleSnapshot(unreadHuddleSnapshot, selfUserId: selfUserId); dms[narrow] = QueueList.from(unreadHuddleSnapshot.unreadMessageIds); + totalCount += unreadHuddleSnapshot.unreadMessageIds.length; } return Unreads._( + totalCount: totalCount, streams: streams, dms: dms, mentions: mentions, @@ -70,18 +80,27 @@ class Unreads extends ChangeNotifier { } Unreads._({ + required totalCount, required this.streams, required this.dms, required this.mentions, required this.oldUnreadsMissing, required this.selfUserId, - }); + }) : _totalCount = totalCount; - // TODO excluded for now; would need to handle nuances around muting etc. - // int count; + /// Total unread messages. + /// + /// Prefer this when possible over traversing the model to make a sum. + /// + /// We initialize and maintain this ourselves, + /// ignoring [UnreadMessagesSnapshot.count] because it has information gaps + /// beyond the ones explained in [oldUnreadsMissing]: + /// https://chat.zulip.org/#narrow/stream/378-api-design/topic/register.3A.20maintaining.20.60unread_msgs.2Ementions.60.20correctly/near/1668449 + int get totalCount => _totalCount; + int _totalCount; /// Unread stream messages, as: stream ID → topic → message ID. - final Map>> streams; + final Map streams; /// Unread DM messages, as: DM narrow → message ID. final Map> dms; @@ -250,6 +269,7 @@ class Unreads extends ChangeNotifier { switch (event) { case UpdateMessageFlagsAddEvent(): if (event.all) { + _totalCount = 0; streams.clear(); dms.clear(); mentions.clear(); @@ -286,14 +306,23 @@ class Unreads extends ChangeNotifier { .add(messageId); } } - newlyUnreadInStreams.forEach((incomingStreamId, incomingTopics) { - incomingTopics.forEach((incomingTopic, incomingMessageIds) { + for ( + final MapEntry(key: incomingStreamId, value: incomingTopics) + in newlyUnreadInStreams.entries + ) { + for ( + final MapEntry(key: incomingTopic, value: incomingMessageIds) + in incomingTopics.entries + ) { _addAllInStreamTopic(incomingMessageIds..sort(), incomingStreamId, incomingTopic); - }); - }); - newlyUnreadInDms.forEach((incomingDmNarrow, incomingMessageIds) { + } + } + for ( + final MapEntry(key: incomingDmNarrow, value: incomingMessageIds) + in newlyUnreadInDms.entries + ) { _addAllInDm(incomingMessageIds..sort(), incomingDmNarrow); - }); + } } } notifyListeners(); @@ -302,62 +331,87 @@ class Unreads extends ChangeNotifier { // TODO use efficient lookups bool _slowIsPresentInStreams(int messageId) { return streams.values.any( - (topics) => topics.values.any( + (streamUnreads) => streamUnreads.topics.values.any( (messageIds) => messageIds.contains(messageId), ), ); } void _addLastInStreamTopic(int messageId, int streamId, String topic) { - ((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId); + final streamUnreads = streams[streamId] ??= StreamUnreads.empty(); + (streamUnreads.topics[topic] ??= QueueList()).addLast(messageId); + streamUnreads.count++; + _totalCount += 1; } // [messageIds] must be sorted ascending and without duplicates. void _addAllInStreamTopic(QueueList messageIds, int streamId, String topic) { - final topics = streams[streamId] ??= {}; - topics.update(topic, - ifAbsent: () => messageIds, + int numAdded = 0; + final streamUnreads = streams[streamId] ??= StreamUnreads.empty(); + streamUnreads.topics.update(topic, + ifAbsent: () { + numAdded = messageIds.length; + return messageIds; + }, // setUnion dedupes existing and incoming unread IDs, // so we tolerate zulip/zulip#22164, fixed in 6.0 // TODO(server-6) remove 6.0 comment - (existing) => setUnion(existing, messageIds), + (existing) { + final result = setUnion(existing, messageIds); + numAdded = result.length - messageIds.length; + return result; + }, ); + streamUnreads.count += numAdded; + _totalCount += numAdded; } // TODO use efficient model lookups void _slowRemoveAllInStreams(Set idsToRemove) { + int totalRemoved = 0; final newlyEmptyStreams = []; - streams.forEach((streamId, topics) { + for (final MapEntry(key: streamId, value: streamUnreads) in streams.entries) { + int removedInStream = 0; final newlyEmptyTopics = []; - topics.forEach((topic, messageIds) { + for (final MapEntry(key: topic, value: messageIds) in streamUnreads.topics.entries) { + final lengthBefore = messageIds.length; messageIds.removeWhere((id) => idsToRemove.contains(id)); + final removedInTopic = lengthBefore - messageIds.length; + removedInStream += removedInTopic; + totalRemoved += removedInTopic; if (messageIds.isEmpty) { newlyEmptyTopics.add(topic); } - }); + } for (final topic in newlyEmptyTopics) { - topics.remove(topic); + streamUnreads.topics.remove(topic); } - if (topics.isEmpty) { + if (streamUnreads.topics.isEmpty) { newlyEmptyStreams.add(streamId); } - }); + streamUnreads.count -= removedInStream; + } for (final streamId in newlyEmptyStreams) { streams.remove(streamId); } + _totalCount -= totalRemoved; } void _removeAllInStreamTopic(Set incomingMessageIds, int streamId, String topic) { - final topics = streams[streamId]; - if (topics == null) return; - final messageIds = topics[topic]; + final streamUnreads = streams[streamId]; + if (streamUnreads == null) return; + final messageIds = streamUnreads.topics[topic]; if (messageIds == null) return; // ([QueueList] doesn't have a `removeAll`) + final lengthBefore = messageIds.length; messageIds.removeWhere((id) => incomingMessageIds.contains(id)); + final numRemoved = lengthBefore - messageIds.length; + streamUnreads.count -= numRemoved; + _totalCount -= numRemoved; if (messageIds.isEmpty) { - topics.remove(topic); - if (topics.isEmpty) { + streamUnreads.topics.remove(topic); + if (streamUnreads.topics.isEmpty) { streams.remove(streamId); } } @@ -370,30 +424,61 @@ class Unreads extends ChangeNotifier { void _addLastInDm(int messageId, DmNarrow narrow) { (dms[narrow] ??= QueueList()).addLast(messageId); + _totalCount += 1; } // [messageIds] must be sorted ascending and without duplicates. void _addAllInDm(QueueList messageIds, DmNarrow dmNarrow) { + int numAdded = 0; dms.update(dmNarrow, - ifAbsent: () => messageIds, + ifAbsent: () { + numAdded = messageIds.length; + return messageIds; + }, // setUnion dedupes existing and incoming unread IDs, // so we tolerate zulip/zulip#22164, fixed in 6.0 // TODO(server-6) remove 6.0 comment - (existing) => setUnion(existing, messageIds), + (existing) { + final result = setUnion(existing, messageIds); + numAdded = result.length - messageIds.length; + return result; + }, ); + _totalCount += numAdded; } // TODO use efficient model lookups void _slowRemoveAllInDms(Set idsToRemove) { + int numRemoved = 0; final newlyEmptyDms = []; - dms.forEach((dmNarrow, messageIds) { + for (final MapEntry(key: dmNarrow, value: messageIds) in dms.entries) { + final lengthBefore = messageIds.length; messageIds.removeWhere((id) => idsToRemove.contains(id)); + numRemoved += lengthBefore - messageIds.length; if (messageIds.isEmpty) { newlyEmptyDms.add(dmNarrow); } - }); + } for (final dmNarrow in newlyEmptyDms) { dms.remove(dmNarrow); } + _totalCount -= numRemoved; } } + +class StreamUnreads { + StreamUnreads({required this.count, required this.topics}); + StreamUnreads.empty() + : count = 0, + topics = {}; + + /// Total unread messages in this stream. + /// + /// Prefer this when possible over traversing [topics] to make a sum. + int count; + + Map> topics; + + @visibleForTesting + Map toJson() => {'count': count, 'topics': topics}; +} diff --git a/test/model/unreads_checks.dart b/test/model/unreads_checks.dart index 836e497b2b..3718999dc0 100644 --- a/test/model/unreads_checks.dart +++ b/test/model/unreads_checks.dart @@ -4,7 +4,8 @@ import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/unreads.dart'; extension UnreadsChecks on Subject { - Subject>>> get streams => has((u) => u.streams, 'streams'); + Subject get count => has((u) => u.totalCount, 'count'); + Subject> get streams => has((u) => u.streams, 'streams'); Subject>> get dms => has((u) => u.dms, 'dms'); Subject> get mentions => has((u) => u.mentions, 'mentions'); Subject get oldUnreadsMissing => has((u) => u.oldUnreadsMissing, 'oldUnreadsMissing'); diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 3d60170b3f..765b0e8fd9 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -8,6 +8,7 @@ import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/unreads.dart'; import '../example_data.dart' as eg; +import '../stdlib_checks.dart'; import 'unreads_checks.dart'; void main() { @@ -53,7 +54,7 @@ void main() { assert(Set.of(messages.map((m) => m.id)).length == messages.length, 'checkMatchesMessages: duplicate messages in test input'); - final Map>> expectedStreams = {}; + final Map expectedStreams = {}; final Map> expectedDms = {}; final Set expectedMentions = {}; for (final message in messages) { @@ -62,9 +63,10 @@ void main() { } switch (message) { case StreamMessage(): - final perTopic = expectedStreams[message.streamId] ??= {}; - final messageIds = perTopic[message.subject] ??= QueueList(); + final streamUnreads = expectedStreams[message.streamId] ??= StreamUnreads.empty(); + final messageIds = streamUnreads.topics[message.subject] ??= QueueList(); messageIds.add(message.id); + streamUnreads.count++; case DmMessage(): final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); final messageIds = expectedDms[narrow] ??= QueueList(); @@ -77,8 +79,8 @@ void main() { expectedMentions.add(message.id); } } - for (final perTopic in expectedStreams.values) { - for (final messageIds in perTopic.values) { + for (final streamUnreads in expectedStreams.values) { + for (final messageIds in streamUnreads.topics.values) { messageIds.sort(); } } @@ -87,7 +89,8 @@ void main() { } check(model) - ..streams.deepEquals(expectedStreams) + ..count.equals(messages.where((m) => !m.flags.contains(MessageFlag.read)).length) + ..streams.jsonEquals(expectedStreams) ..dms.deepEquals(expectedDms) ..mentions.unorderedEquals(expectedMentions); } diff --git a/test/stdlib_checks.dart b/test/stdlib_checks.dart index 9d87e1131e..97be0ca164 100644 --- a/test/stdlib_checks.dart +++ b/test/stdlib_checks.dart @@ -63,6 +63,8 @@ Object? deepToJson(Object? object) { result = object.map((x) => deepToJson(x)).toList(); case Map() when object.keys.every((k) => k is String): result = object.map((k, v) => MapEntry(k, deepToJson(v))); + case Map() when object.keys.every((k) => k is int): + result = object.map((k, v) => MapEntry(k.toString(), deepToJson(v))); default: return (null, false); }