Skip to content

Commit 74ed495

Browse files
apoorvapendseapoorvapendse
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. I referred to the FoldDict code in zulip/zulip as @gnprice suggested. Thanks to the folks who wrote it! 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 74ed495

File tree

6 files changed

+234
-32
lines changed

6 files changed

+234
-32
lines changed

‎lib/model/channel.dart‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,3 +361,55 @@ class ChannelStoreImpl with ChannelStore {
361361
}
362362
}
363363
}
364+
365+
class TopicMap<T> {
366+
final Map<String, _KeyValue<T>> _topicMessageMap = {};
367+
368+
int get size => _topicMessageMap.length;
369+
370+
T? get(TopicName key) {
371+
return _topicMessageMap[_munge(key)]?.v;
372+
}
373+
374+
void set(TopicName key, T value) {
375+
_topicMessageMap[_munge(key)] = _KeyValue(k: key, v: value);
376+
}
377+
378+
TopicName? getSomeTopicVersion(TopicName key) {
379+
return _topicMessageMap[_munge(key)]?.k;
380+
}
381+
382+
bool containsKey(TopicName key) => _topicMessageMap.containsKey(_munge(key));
383+
384+
bool remove(TopicName key) => _topicMessageMap.remove(_munge(key)) != null;
385+
386+
Iterable<TopicName> keys() => _topicMessageMap.values.map((kv) => kv.k);
387+
388+
Iterable<T> values() => _topicMessageMap.values.map((kv) => kv.v);
389+
390+
Iterable<MapEntry<TopicName, T>> get entries =>
391+
_topicMessageMap.entries.map((kv) => MapEntry(kv.value.k, kv.value.v));
392+
393+
void clear() => _topicMessageMap.clear();
394+
395+
String _munge(TopicName key) => key.toString().toLowerCase();
396+
}
397+
398+
extension TopicMapExtensions<T> on TopicMap<T> {
399+
// Converts this `TopicMap<T>` into a plain `Map<TopicName, T>`,
400+
// making it easier to do standard deep equality checks in tests.
401+
Map<TopicName, T> toMap() {
402+
final result = <TopicName, T>{};
403+
for (final entry in entries) {
404+
result[entry.key] = entry.value;
405+
}
406+
return result;
407+
}
408+
}
409+
410+
class _KeyValue<T> {
411+
final TopicName k;
412+
final T v;
413+
414+
_KeyValue({required this.k, required this.v});
415+
}

‎lib/model/unreads.dart‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,14 @@ class Unreads extends ChangeNotifier {
4040
required int selfUserId,
4141
required ChannelStore channelStore,
4242
}) {
43-
final streams = <int, Map<TopicName, QueueList<int>>>{};
43+
final streams = <int, TopicMap<QueueList<int>>>{};
4444
final dms = <DmNarrow, QueueList<int>>{};
4545
final mentions = Set.of(initial.mentions);
4646

4747
for (final unreadChannelSnapshot in initial.channels) {
4848
final streamId = unreadChannelSnapshot.streamId;
4949
final topic = unreadChannelSnapshot.topic;
50-
(streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
50+
(streams[streamId] ??= TopicMap<QueueList<int>>()).set(topic,QueueList.from(unreadChannelSnapshot.unreadMessageIds));
5151
}
5252

5353
for (final unreadDmSnapshot in initial.dms) {
@@ -86,7 +86,7 @@ class Unreads extends ChangeNotifier {
8686
// int count;
8787

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

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

188188
int countInTopicNarrow(int streamId, TopicName topic) {
189189
final topics = streams[streamId];
190-
return topics?[topic]?.length ?? 0;
190+
if (topics == null) return 0;
191+
final qlist = topics.get(topic);
192+
return qlist?.length ?? 0;
191193
}
192194

193195
int countInDmNarrow(DmNarrow narrow) => dms[narrow]?.length ?? 0;
@@ -443,26 +445,30 @@ class Unreads extends ChangeNotifier {
443445
// TODO use efficient lookups
444446
bool _slowIsPresentInStreams(int messageId) {
445447
return streams.values.any(
446-
(topics) => topics.values.any(
448+
(topics) => topics.values().any(
447449
(messageIds) => messageIds.contains(messageId),
448450
),
449451
);
450452
}
451453

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

456461
// [messageIds] must be sorted ascending and without duplicates.
457462
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-
);
463+
final topics = streams[streamId] ??= TopicMap<QueueList<int>>();
464+
if (topics.containsKey(topic)) {
465+
// If the topic already exists, update its message ids with setUnion
466+
topics.set(topic, setUnion(topics.get(topic) ?? QueueList<int>(), messageIds));
467+
} else {
468+
// If the topic does not exist, insert the new messageIds list
469+
topics.set(topic, messageIds);
470+
}
471+
466472
}
467473

468474
// TODO use efficient model lookups
@@ -479,7 +485,7 @@ class Unreads extends ChangeNotifier {
479485
for (final topic in newlyEmptyTopics) {
480486
topics.remove(topic);
481487
}
482-
if (topics.isEmpty) {
488+
if (topics.size==0) {
483489
newlyEmptyStreams.add(streamId);
484490
}
485491
}
@@ -491,14 +497,14 @@ class Unreads extends ChangeNotifier {
491497
void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
492498
final topics = streams[streamId];
493499
if (topics == null) return;
494-
final messageIds = topics[topic];
500+
final messageIds = topics.get(topic);
495501
if (messageIds == null) return;
496502

497503
// ([QueueList] doesn't have a `removeAll`)
498504
messageIds.removeWhere((id) => incomingMessageIds.contains(id));
499505
if (messageIds.isEmpty) {
500506
topics.remove(topic);
501-
if (topics.isEmpty) {
507+
if (topics.size==0) {
502508
streams.remove(streamId);
503509
}
504510
}

‎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/channel_test.dart‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11

22
import 'package:checks/checks.dart';
3+
import 'package:collection/collection.dart';
34
import 'package:test/scaffolding.dart';
45
import 'package:zulip/api/model/events.dart';
56
import 'package:zulip/api/model/initial_snapshot.dart';
67
import 'package:zulip/api/model/model.dart';
8+
import 'package:zulip/model/algorithms.dart';
79
import 'package:zulip/model/store.dart';
810
import 'package:zulip/model/channel.dart';
911

@@ -394,4 +396,118 @@ void main() {
394396
.equals(UserTopicVisibilityPolicy.none);
395397
});
396398
});
399+
group('TopicMap with QueueList<int>', () {
400+
test('initially empty', () {
401+
final topicMap = TopicMap<QueueList<int>>();
402+
check(topicMap.size).equals(0);
403+
check(topicMap.keys()).isEmpty();
404+
check(topicMap.values()).isEmpty();
405+
check(topicMap.entries).isEmpty();
406+
check(topicMap.toMap()).deepEquals({});
407+
});
408+
409+
test('set and get values', () {
410+
final topicMap = TopicMap<QueueList<int>>();
411+
final topic = TopicName('Topic1');
412+
final value = QueueList<int>();
413+
value.addAll([1, 2, 3]);
414+
topicMap.set(topic, value);
415+
416+
check(topicMap.size).equals(1);
417+
check(topicMap.get(topic)).equals(value);
418+
});
419+
420+
test('containsKey and remove', () {
421+
final topicMap = TopicMap<QueueList<int>>();
422+
final topic = TopicName('Math');
423+
final value = QueueList<int>();
424+
value.addAll([3, 1, 4]);
425+
topicMap.set(topic, value);
426+
427+
check(topicMap.containsKey(topic)).isTrue();
428+
// Removing should return true.
429+
check(topicMap.remove(topic)).isTrue();
430+
check(topicMap.containsKey(topic)).isFalse();
431+
check(topicMap.size).equals(0);
432+
// Removing a non-existent key returns false.
433+
check(topicMap.remove(topic)).isFalse();
434+
});
435+
436+
test('keys, values, and entries', () {
437+
final topicMap = TopicMap<QueueList<int>>();
438+
final topicA = TopicName('A');
439+
final topicB = TopicName('B');
440+
final valueA = QueueList<int>();
441+
valueA.addAll([10, 20]);
442+
final valueB = QueueList<int>();
443+
valueB.addAll([30,40]);
444+
topicMap.set(topicA, valueA);
445+
topicMap.set(topicB, valueB);
446+
447+
final keys = topicMap.keys().toList();
448+
check(keys.length).equals(2);
449+
check(keys).contains(topicA);
450+
check(keys).contains(topicB);
451+
452+
final values = topicMap.values().toList();
453+
check(values.length).equals(2);
454+
check(values).contains(valueA);
455+
check(values).contains(valueB);
456+
457+
final entries = topicMap.entries.toList();
458+
check(entries.length).equals(2);
459+
for (final entry in entries) {
460+
if (entry.key == topicA) {
461+
check(entry.value).equals(valueA);
462+
} else if (entry.key == topicB) {
463+
check(entry.value).equals(valueB);
464+
}
465+
}
466+
});
467+
468+
test('clear and toMap', () {
469+
final topicMap = TopicMap<QueueList<int>>();
470+
final topic1 = TopicName('One');
471+
final topic2 = TopicName('Two');
472+
final value1 = QueueList<int>();
473+
final value2 = QueueList<int>();
474+
value1.add(1);
475+
value2.add(2);
476+
topicMap.set(topic1, value1);
477+
topicMap.set(topic2, value2);
478+
479+
check(topicMap.size).equals(2);
480+
check(topicMap.toMap()).deepEquals({
481+
topic1: value1,
482+
topic2: value2,
483+
});
484+
485+
topicMap.clear();
486+
check(topicMap.size).equals(0);
487+
check(topicMap.toMap()).deepEquals({});
488+
});
489+
490+
test('key normalization is case-insensitive', () {
491+
final topicMap = TopicMap<QueueList<int>>();
492+
// Using two TopicName instances that differ only by case.
493+
final topicUpper = TopicName('HELLO');
494+
final topicLower = TopicName('hello');
495+
final valueLower = QueueList<int>();
496+
final valueUpper = QueueList<int>();
497+
valueUpper.add(42);
498+
valueLower.add(32);
499+
topicMap.set(topicLower, valueLower);
500+
topicMap.set(topicUpper, setUnion(topicMap.get(topicLower) as Iterable<int>, valueUpper));
501+
// Since _munge lowercases the key, using a different case should return the same value.
502+
final list1 = topicMap.get(topicUpper);
503+
final list2 = setUnion(valueUpper, valueLower);
504+
check(list1?.length).equals(list2.length);
505+
for ( int i = 0; i < list1!.length; i++ ) {
506+
check(list1[i]).equals(list2[i]);
507+
}
508+
509+
check(topicMap.toMap().containsKey(TopicName("HELLO"))).isTrue();
510+
check(topicMap.toMap().containsKey(TopicName("hello"))).isFalse();
511+
});
512+
});
397513
}

‎test/model/unreads_checks.dart‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,17 @@ import 'package:collection/collection.dart';
33
import 'package:zulip/api/model/model.dart';
44
import 'package:zulip/model/narrow.dart';
55
import 'package:zulip/model/unreads.dart';
6+
import 'package:zulip/model/channel.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');

0 commit comments

Comments
 (0)