diff --git a/lib/model/channel.dart b/lib/model/channel.dart index 3f5fbe33af..b1aeeb7e9b 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -361,3 +361,51 @@ class ChannelStoreImpl with ChannelStore { } } } + +class TopicMap { + final Map> _topicMessageMap = {}; + + int get size => _topicMessageMap.length; + + T? get(TopicName key) { + return _topicMessageMap[_munge(key)]?.v; + } + + void set(TopicName key, T value) { + _topicMessageMap[_munge(key)] = _KeyValue(k: key, v: value); + } + + bool containsKey(TopicName key) => _topicMessageMap.containsKey(_munge(key)); + + bool remove(TopicName key) => _topicMessageMap.remove(_munge(key)) != null; + + Iterable keys() => _topicMessageMap.values.map((kv) => kv.k); + + Iterable values() => _topicMessageMap.values.map((kv) => kv.v); + + Iterable> get entries => + _topicMessageMap.entries.map((kv) => MapEntry(kv.value.k, kv.value.v)); + + void clear() => _topicMessageMap.clear(); + + String _munge(TopicName key) => key.toString().toLowerCase(); +} + +extension TopicMapExtensions on TopicMap { + // Converts this `TopicMap` into a plain `Map`, + // making it easier to do standard deep equality checks in tests. + Map toMap() { + final result = {}; + for (final entry in entries) { + result[entry.key] = entry.value; + } + return result; + } +} + +class _KeyValue { + final TopicName k; + final T v; + + _KeyValue({required this.k, required this.v}); +} \ No newline at end of file diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index 1fcea3f83c..5f6a960c58 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -40,14 +40,14 @@ class Unreads extends ChangeNotifier { required int selfUserId, required ChannelStore channelStore, }) { - final streams = >>{}; + final streams = >>{}; final dms = >{}; final mentions = Set.of(initial.mentions); for (final unreadChannelSnapshot in initial.channels) { final streamId = unreadChannelSnapshot.streamId; final topic = unreadChannelSnapshot.topic; - (streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds); + (streams[streamId] ??= TopicMap>()).set(topic,QueueList.from(unreadChannelSnapshot.unreadMessageIds)); } for (final unreadDmSnapshot in initial.dms) { @@ -86,7 +86,7 @@ class Unreads extends ChangeNotifier { // int count; /// Unread stream messages, as: stream ID → topic → message IDs (sorted). - final Map>> streams; + final Map>> streams; /// Unread DM messages, as: DM narrow → message IDs (sorted). final Map> dms; @@ -187,7 +187,9 @@ class Unreads extends ChangeNotifier { int countInTopicNarrow(int streamId, TopicName topic) { final topics = streams[streamId]; - return topics?[topic]?.length ?? 0; + if (topics == null) return 0; + final qlist = topics.get(topic); + return qlist?.length ?? 0; } int countInDmNarrow(DmNarrow narrow) => dms[narrow]?.length ?? 0; @@ -443,26 +445,30 @@ class Unreads extends ChangeNotifier { // TODO use efficient lookups bool _slowIsPresentInStreams(int messageId) { return streams.values.any( - (topics) => topics.values.any( + (topics) => topics.values().any( (messageIds) => messageIds.contains(messageId), ), ); } void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) { - ((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId); + if ((streams[streamId] ??= TopicMap>()).get(topic) == null) { + streams[streamId]?.set(topic, QueueList()); + } + (streams[streamId] ??= TopicMap>()).get(topic)?.addLast(messageId); } // [messageIds] must be sorted ascending and without duplicates. void _addAllInStreamTopic(QueueList messageIds, int streamId, TopicName topic) { - final topics = streams[streamId] ??= {}; - topics.update(topic, - ifAbsent: () => 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), - ); + final topics = streams[streamId] ??= TopicMap>(); + if (topics.containsKey(topic)) { + // If the topic already exists, update its message ids with setUnion + topics.set(topic, setUnion(topics.get(topic) ?? QueueList(), messageIds)); + } else { + // If the topic does not exist, insert the new messageIds list + topics.set(topic, messageIds); + } + } // TODO use efficient model lookups @@ -479,7 +485,7 @@ class Unreads extends ChangeNotifier { for (final topic in newlyEmptyTopics) { topics.remove(topic); } - if (topics.isEmpty) { + if (topics.size==0) { newlyEmptyStreams.add(streamId); } } @@ -491,14 +497,14 @@ class Unreads extends ChangeNotifier { void _removeAllInStreamTopic(Set incomingMessageIds, int streamId, TopicName topic) { final topics = streams[streamId]; if (topics == null) return; - final messageIds = topics[topic]; + final messageIds = topics.get(topic); if (messageIds == null) return; // ([QueueList] doesn't have a `removeAll`) messageIds.removeWhere((id) => incomingMessageIds.contains(id)); if (messageIds.isEmpty) { topics.remove(topic); - if (topics.isEmpty) { + if (topics.size==0) { streams.remove(streamId); } } diff --git a/pubspec.lock b/pubspec.lock index 4ee3317f8d..d7c9cb6fa3 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -291,10 +291,10 @@ packages: dependency: "direct dev" description: name: fake_async - sha256: "6a95e56b2449df2273fd8c45a662d6947ce1ebb7aafe80e550a3f68297f3cacc" + sha256: "5368f224a74523e8d2e7399ea1638b37aecfca824a3cc4dfdf77bf1fa905ac44" url: "https://pub.dev" source: hosted - version: "1.3.2" + version: "1.3.3" ffi: dependency: transitive description: @@ -607,10 +607,10 @@ packages: dependency: "direct main" description: name: intl - sha256: d6f56758b7d3014a48af9701c085700aac781a92a87a62b1333b46d8879661cf + sha256: "3df61194eb431efc39c4ceba583b95633a403f46c9fd341e550ce0bfa50e9aa5" url: "https://pub.dev" source: hosted - version: "0.19.0" + version: "0.20.2" io: dependency: transitive description: @@ -647,10 +647,10 @@ packages: dependency: transitive description: name: leak_tracker - sha256: c35baad643ba394b40aac41080300150a4f08fd0fd6a10378f8f7c6bc161acec + sha256: "6bb818ecbdffe216e81182c2f0714a2e62b593f4a4f13098713ff1685dfb6ab0" url: "https://pub.dev" source: hosted - version: "10.0.8" + version: "10.0.9" leak_tracker_flutter_testing: dependency: transitive description: @@ -1244,10 +1244,10 @@ packages: dependency: transitive description: name: vm_service - sha256: "0968250880a6c5fe7edc067ed0a13d4bae1577fe2771dcf3010d52c4a9d3ca14" + sha256: ddfa8d30d89985b96407efce8acbdd124701f96741f2d981ca860662f1c0dc02 url: "https://pub.dev" source: hosted - version: "14.3.1" + version: "15.0.0" wakelock_plus: dependency: "direct main" description: @@ -1300,10 +1300,10 @@ packages: dependency: transitive description: name: webdriver - sha256: "3d773670966f02a646319410766d3b5e1037efb7f07cc68f844d5e06cd4d61c8" + sha256: "2f3a14ca026957870cfd9c635b83507e0e51d8091568e90129fbf805aba7cade" url: "https://pub.dev" source: hosted - version: "3.0.4" + version: "3.1.0" webkit_inspection_protocol: dependency: transitive description: diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index f22ac7cc8f..d9091364e1 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -1,9 +1,11 @@ import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/algorithms.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/channel.dart'; @@ -394,4 +396,118 @@ void main() { .equals(UserTopicVisibilityPolicy.none); }); }); + group('TopicMap with QueueList', () { + test('initially empty', () { + final topicMap = TopicMap>(); + check(topicMap.size).equals(0); + check(topicMap.keys()).isEmpty(); + check(topicMap.values()).isEmpty(); + check(topicMap.entries).isEmpty(); + check(topicMap.toMap()).deepEquals({}); + }); + + test('set and get values', () { + final topicMap = TopicMap>(); + final topic = TopicName('Topic1'); + final value = QueueList(); + value.addAll([1, 2, 3]); + topicMap.set(topic, value); + + check(topicMap.size).equals(1); + check(topicMap.get(topic)).equals(value); + }); + + test('containsKey and remove', () { + final topicMap = TopicMap>(); + final topic = TopicName('Math'); + final value = QueueList(); + value.addAll([3, 1, 4]); + topicMap.set(topic, value); + + check(topicMap.containsKey(topic)).isTrue(); + // Removing should return true. + check(topicMap.remove(topic)).isTrue(); + check(topicMap.containsKey(topic)).isFalse(); + check(topicMap.size).equals(0); + // Removing a non-existent key returns false. + check(topicMap.remove(topic)).isFalse(); + }); + + test('keys, values, and entries', () { + final topicMap = TopicMap>(); + final topicA = TopicName('A'); + final topicB = TopicName('B'); + final valueA = QueueList(); + valueA.addAll([10, 20]); + final valueB = QueueList(); + valueB.addAll([30,40]); + topicMap.set(topicA, valueA); + topicMap.set(topicB, valueB); + + final keys = topicMap.keys().toList(); + check(keys.length).equals(2); + check(keys).contains(topicA); + check(keys).contains(topicB); + + final values = topicMap.values().toList(); + check(values.length).equals(2); + check(values).contains(valueA); + check(values).contains(valueB); + + final entries = topicMap.entries.toList(); + check(entries.length).equals(2); + for (final entry in entries) { + if (entry.key == topicA) { + check(entry.value).equals(valueA); + } else if (entry.key == topicB) { + check(entry.value).equals(valueB); + } + } + }); + + test('clear and toMap', () { + final topicMap = TopicMap>(); + final topic1 = TopicName('One'); + final topic2 = TopicName('Two'); + final value1 = QueueList(); + final value2 = QueueList(); + value1.add(1); + value2.add(2); + topicMap.set(topic1, value1); + topicMap.set(topic2, value2); + + check(topicMap.size).equals(2); + check(topicMap.toMap()).deepEquals({ + topic1: value1, + topic2: value2, + }); + + topicMap.clear(); + check(topicMap.size).equals(0); + check(topicMap.toMap()).deepEquals({}); + }); + + test('key normalization is case-insensitive', () { + final topicMap = TopicMap>(); + // Using two TopicName instances that differ only by case. + final topicUpper = TopicName('HELLO'); + final topicLower = TopicName('hello'); + final valueLower = QueueList(); + final valueUpper = QueueList(); + valueUpper.add(42); + valueLower.add(32); + topicMap.set(topicLower, valueLower); + topicMap.set(topicUpper, setUnion(topicMap.get(topicLower) as Iterable, valueUpper)); + // Since _munge lowercases the key, using a different case should return the same value. + final list1 = topicMap.get(topicUpper); + final list2 = setUnion(valueUpper, valueLower); + check(list1?.length).equals(list2.length); + for ( int i = 0; i < list1!.length; i++ ) { + check(list1[i]).equals(list2[i]); + } + + check(topicMap.toMap().containsKey(TopicName("HELLO"))).isTrue(); + check(topicMap.toMap().containsKey(TopicName("hello"))).isFalse(); + }); + }); } diff --git a/test/model/unreads_checks.dart b/test/model/unreads_checks.dart index ac4d64846a..e21810e773 100644 --- a/test/model/unreads_checks.dart +++ b/test/model/unreads_checks.dart @@ -3,9 +3,17 @@ import 'package:collection/collection.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/unreads.dart'; +import 'package:zulip/model/channel.dart'; extension UnreadsChecks on Subject { - Subject>>> get streams => has((u) => u.streams, 'streams'); + Subject>>> get streams => has( + (unreads) { + return unreads.streams.map( + (streamId, topicMap) => MapEntry(streamId, topicMap.toMap()), + ); + }, + '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 40e074dcaa..cb542481e1 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -59,6 +59,8 @@ void main() { 'checkMatchesMessages: duplicate messages in test input'); final Map>> expectedStreams = {}; + final earliestCapitalizationByStream = + >{}; final Map> expectedDms = {}; final Set expectedMentions = {}; for (final message in messages) { @@ -67,9 +69,25 @@ void main() { } switch (message) { case StreamMessage(): - final perTopic = expectedStreams[message.streamId] ??= {}; - final messageIds = perTopic[message.topic] ??= QueueList(); - messageIds.add(message.id); + // Convert the message’s topic to lowercase, just like our code. + final streamId = message.streamId; + final realTopic = message.topic.toString(); + final lowerTopic = realTopic.toLowerCase(); + + final perTopic = expectedStreams[streamId] + ??= >{}; + + final earliestMap = earliestCapitalizationByStream[streamId] + ??= {}; + + // If we already have a capitalization for this topic, reuse it; + // otherwise, record this new capitalization as earliest. + final earliestCaps = earliestMap[lowerTopic] ??= realTopic; + + // Insert the message under that earliestCaps key + final topicName = TopicName(earliestCaps); + final qlist = perTopic[topicName] ??= QueueList(); + qlist.add(message.id); case DmMessage(): final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); final messageIds = expectedDms[narrow] ??= QueueList(); @@ -332,13 +350,15 @@ void main() { final stream2 = eg.stream(streamId: 2); for (final (oldStream, newStream, oldTopic, newTopic) in [ (stream1, stream1, 'a', 'a'), + (stream1, stream1, 'a', 'A'), + (stream1, stream1, 'Zebra', 'ZEBRa'), (stream1, stream1, 'a', 'b'), (stream1, stream2, 'a', 'a'), (stream1, stream2, 'a', 'b'), ]) { final description = [ oldStream.streamId == newStream.streamId ? 'same stream' : 'different stream', - oldTopic == newTopic ? 'same topic' : 'different topic', + oldTopic.toLowerCase() == newTopic.toLowerCase() ? 'same topic' : 'different topic', ].join(' / '); test(description, () { final oldMessage = eg.streamMessage(stream: oldStream, topic: oldTopic, flags: []);