Skip to content

Commit 07f29ad

Browse files
committed
unreads: Fix topic case-sensitivity.
Maps canoncalized topic names to a variant of the actual topic name. This is achieved by introducing a TopicMap class similar to FoldDict in web. See https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Expected.20behaviour.20for.20case.20insensitivity.20for.20unreads.20.23F980/near/2076806 Fixes #980.
1 parent 449f326 commit 07f29ad

File tree

5 files changed

+104
-33
lines changed

5 files changed

+104
-33
lines changed

lib/model/topicmap.dart

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
2+
import '../api/model/model.dart';
3+
4+
class TopicMap<T> {
5+
final Map<String, _KeyValue<T>> _topicMessageMap = {};
6+
7+
int get size => _topicMessageMap.length;
8+
9+
T? get(TopicName key) {
10+
return _topicMessageMap[_munge(key)]?.v;
11+
}
12+
13+
void set(TopicName key, T value) {
14+
_topicMessageMap[_munge(key)] = _KeyValue(k: key, v: value);
15+
}
16+
17+
TopicName? getSomeTopicVersion(TopicName key) {
18+
return _topicMessageMap[_munge(key)]?.k;
19+
}
20+
21+
bool has(TopicName key) => _topicMessageMap.containsKey(_munge(key));
22+
23+
bool delete(TopicName key) => _topicMessageMap.remove(_munge(key)) != null;
24+
25+
Iterable<TopicName> keys() => _topicMessageMap.values.map((kv) => kv.k);
26+
27+
Iterable<T> values() => _topicMessageMap.values.map((kv) => kv.v);
28+
29+
Iterable<MapEntry<TopicName, T>> get entries =>
30+
_topicMessageMap.entries.map((kv) => MapEntry(kv.value.k, kv.value.v));
31+
32+
void clear() => _topicMessageMap.clear();
33+
34+
35+
String _munge(TopicName key) => key.toString().toLowerCase();
36+
}
37+
38+
extension TopicMapExtensions<T> on TopicMap<T> {
39+
// Converts this `TopicMap<T>` into a plain `Map<TopicName, T>`,
40+
// making it easier to do standard deep equality checks in tests.
41+
Map<TopicName, T> toMap() {
42+
final result = <TopicName, T>{};
43+
for (final entry in entries) {
44+
result[entry.key] = entry.value;
45+
}
46+
return result;
47+
}
48+
}
49+
50+
class _KeyValue<T> {
51+
final TopicName k;
52+
final T v;
53+
54+
_KeyValue({required this.k, required this.v});
55+
}

lib/model/unreads.dart

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import '../log.dart';
1010
import 'algorithms.dart';
1111
import 'narrow.dart';
1212
import 'channel.dart';
13+
import 'topicmap.dart';
1314

1415
/// The view-model for unread messages.
1516
///
@@ -40,14 +41,14 @@ class Unreads extends ChangeNotifier {
4041
required int selfUserId,
4142
required ChannelStore channelStore,
4243
}) {
43-
final streams = <int, Map<TopicName, QueueList<int>>>{};
44+
final streams = <int, TopicMap<QueueList<int>>>{};
4445
final dms = <DmNarrow, QueueList<int>>{};
4546
final mentions = Set.of(initial.mentions);
4647

4748
for (final unreadChannelSnapshot in initial.channels) {
4849
final streamId = unreadChannelSnapshot.streamId;
4950
final topic = unreadChannelSnapshot.topic;
50-
(streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
51+
(streams[streamId] ??= TopicMap<QueueList<int>>()).set(topic,QueueList.from(unreadChannelSnapshot.unreadMessageIds));
5152
}
5253

5354
for (final unreadDmSnapshot in initial.dms) {
@@ -86,7 +87,7 @@ class Unreads extends ChangeNotifier {
8687
// int count;
8788

8889
/// Unread stream messages, as: stream ID → topic → message IDs (sorted).
89-
final Map<int, Map<TopicName, QueueList<int>>> streams;
90+
final Map<int, TopicMap<QueueList<int>>> streams;
9091

9192
/// Unread DM messages, as: DM narrow → message IDs (sorted).
9293
final Map<DmNarrow, QueueList<int>> dms;
@@ -187,7 +188,9 @@ class Unreads extends ChangeNotifier {
187188

188189
int countInTopicNarrow(int streamId, TopicName topic) {
189190
final topics = streams[streamId];
190-
return topics?[topic]?.length ?? 0;
191+
if (topics == null) return 0;
192+
final qlist = topics.get(topic);
193+
return qlist?.length ?? 0;
191194
}
192195

193196
int countInDmNarrow(DmNarrow narrow) => dms[narrow]?.length ?? 0;
@@ -443,26 +446,30 @@ class Unreads extends ChangeNotifier {
443446
// TODO use efficient lookups
444447
bool _slowIsPresentInStreams(int messageId) {
445448
return streams.values.any(
446-
(topics) => topics.values.any(
449+
(topics) => topics.values().any(
447450
(messageIds) => messageIds.contains(messageId),
448451
),
449452
);
450453
}
451454

452455
void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) {
453-
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId);
456+
if ((streams[streamId] ??= TopicMap<QueueList<int>>()).get(topic) == null) {
457+
streams[streamId]?.set(topic, QueueList<int>());
458+
}
459+
(streams[streamId] ??= TopicMap<QueueList<int>>()).get(topic)?.addLast(messageId);
454460
}
455461

456462
// [messageIds] must be sorted ascending and without duplicates.
457463
void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, TopicName topic) {
458-
final topics = streams[streamId] ??= {};
459-
topics.update(topic,
460-
ifAbsent: () => messageIds,
461-
// setUnion dedupes existing and incoming unread IDs,
462-
// so we tolerate zulip/zulip#22164, fixed in 6.0
463-
// TODO(server-6) remove 6.0 comment
464-
(existing) => setUnion(existing, messageIds),
465-
);
464+
final topics = streams[streamId] ??= TopicMap<QueueList<int>>();
465+
if (topics.has(topic)) {
466+
// If the topic already exists, update its message ids with setUnion
467+
topics.set(topic, setUnion(topics.get(topic) ?? QueueList<int>(), messageIds));
468+
} else {
469+
// If the topic does not exist, insert the new messageIds list
470+
topics.set(topic, messageIds);
471+
}
472+
466473
}
467474

468475
// TODO use efficient model lookups
@@ -477,9 +484,9 @@ class Unreads extends ChangeNotifier {
477484
}
478485
}
479486
for (final topic in newlyEmptyTopics) {
480-
topics.remove(topic);
487+
topics.delete(topic);
481488
}
482-
if (topics.isEmpty) {
489+
if (topics.size==0) {
483490
newlyEmptyStreams.add(streamId);
484491
}
485492
}
@@ -491,14 +498,14 @@ class Unreads extends ChangeNotifier {
491498
void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
492499
final topics = streams[streamId];
493500
if (topics == null) return;
494-
final messageIds = topics[topic];
501+
final messageIds = topics.get(topic);
495502
if (messageIds == null) return;
496503

497504
// ([QueueList] doesn't have a `removeAll`)
498505
messageIds.removeWhere((id) => incomingMessageIds.contains(id));
499506
if (messageIds.isEmpty) {
500-
topics.remove(topic);
501-
if (topics.isEmpty) {
507+
topics.delete(topic);
508+
if (topics.size==0) {
502509
streams.remove(streamId);
503510
}
504511
}

pubspec.lock

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -291,10 +291,10 @@ packages:
291291
dependency: "direct dev"
292292
description:
293293
name: fake_async
294-
sha256: "6a95e56b2449df2273fd8c45a662d6947ce1ebb7aafe80e550a3f68297f3cacc"
294+
sha256: "5368f224a74523e8d2e7399ea1638b37aecfca824a3cc4dfdf77bf1fa905ac44"
295295
url: "https://pub.dev"
296296
source: hosted
297-
version: "1.3.2"
297+
version: "1.3.3"
298298
ffi:
299299
dependency: transitive
300300
description:
@@ -607,10 +607,10 @@ packages:
607607
dependency: "direct main"
608608
description:
609609
name: intl
610-
sha256: d6f56758b7d3014a48af9701c085700aac781a92a87a62b1333b46d8879661cf
610+
sha256: "3df61194eb431efc39c4ceba583b95633a403f46c9fd341e550ce0bfa50e9aa5"
611611
url: "https://pub.dev"
612612
source: hosted
613-
version: "0.19.0"
613+
version: "0.20.2"
614614
io:
615615
dependency: transitive
616616
description:
@@ -647,10 +647,10 @@ packages:
647647
dependency: transitive
648648
description:
649649
name: leak_tracker
650-
sha256: c35baad643ba394b40aac41080300150a4f08fd0fd6a10378f8f7c6bc161acec
650+
sha256: "6bb818ecbdffe216e81182c2f0714a2e62b593f4a4f13098713ff1685dfb6ab0"
651651
url: "https://pub.dev"
652652
source: hosted
653-
version: "10.0.8"
653+
version: "10.0.9"
654654
leak_tracker_flutter_testing:
655655
dependency: transitive
656656
description:
@@ -1244,10 +1244,10 @@ packages:
12441244
dependency: transitive
12451245
description:
12461246
name: vm_service
1247-
sha256: "0968250880a6c5fe7edc067ed0a13d4bae1577fe2771dcf3010d52c4a9d3ca14"
1247+
sha256: ddfa8d30d89985b96407efce8acbdd124701f96741f2d981ca860662f1c0dc02
12481248
url: "https://pub.dev"
12491249
source: hosted
1250-
version: "14.3.1"
1250+
version: "15.0.0"
12511251
wakelock_plus:
12521252
dependency: "direct main"
12531253
description:
@@ -1300,10 +1300,10 @@ packages:
13001300
dependency: transitive
13011301
description:
13021302
name: webdriver
1303-
sha256: "3d773670966f02a646319410766d3b5e1037efb7f07cc68f844d5e06cd4d61c8"
1303+
sha256: "2f3a14ca026957870cfd9c635b83507e0e51d8091568e90129fbf805aba7cade"
13041304
url: "https://pub.dev"
13051305
source: hosted
1306-
version: "3.0.4"
1306+
version: "3.1.0"
13071307
webkit_inspection_protocol:
13081308
dependency: transitive
13091309
description:

test/model/unreads_checks.dart

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,18 @@ import 'package:checks/checks.dart';
22
import 'package:collection/collection.dart';
33
import 'package:zulip/api/model/model.dart';
44
import 'package:zulip/model/narrow.dart';
5+
import 'package:zulip/model/topicmap.dart';
56
import 'package:zulip/model/unreads.dart';
67

78
extension UnreadsChecks on Subject<Unreads> {
8-
Subject<Map<int, Map<TopicName, QueueList<int>>>> get streams => has((u) => u.streams, 'streams');
9+
Subject<Map<int, Map<TopicName, QueueList<int>>>> get streams => has(
10+
(unreads) {
11+
return unreads.streams.map(
12+
(streamId, topicMap) => MapEntry(streamId, topicMap.toMap()),
13+
);
14+
},
15+
'streams',
16+
);
917
Subject<Map<DmNarrow, QueueList<int>>> get dms => has((u) => u.dms, 'dms');
1018
Subject<Set<int>> get mentions => has((u) => u.mentions, 'mentions');
1119
Subject<bool> get oldUnreadsMissing => has((u) => u.oldUnreadsMissing, 'oldUnreadsMissing');

test/model/unreads_test.dart

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,10 @@ void main() {
6767
}
6868
switch (message) {
6969
case StreamMessage():
70-
final perTopic = expectedStreams[message.streamId] ??= {};
71-
final messageIds = perTopic[message.topic] ??= QueueList();
72-
messageIds.add(message.id);
70+
final perTopic = expectedStreams[message.streamId]
71+
??= <TopicName, QueueList<int>>{};
72+
final qlist = perTopic[message.topic] ??= QueueList<int>();
73+
qlist.add(message.id);
7374
case DmMessage():
7475
final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId);
7576
final messageIds = expectedDms[narrow] ??= QueueList();

0 commit comments

Comments
 (0)