Skip to content

Commit 054ec0d

Browse files
chrisbobbechrisbobbe
committed
unreads: Treat topics case-insensitively
Fixes: zulip#980
1 parent 4e7c0d0 commit 054ec0d

File tree

2 files changed

+95
-15
lines changed

2 files changed

+95
-15
lines changed

‎lib/model/unreads.dart‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,22 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
4141
required CorePerAccountStore core,
4242
required ChannelStore channelStore,
4343
}) {
44-
final streams = <int, Map<TopicName, QueueList<int>>>{};
44+
final streams = <int, TopicKeyedMap<QueueList<int>>>{};
4545
final dms = <DmNarrow, QueueList<int>>{};
4646
final mentions = Set.of(initial.mentions);
4747

4848
for (final unreadChannelSnapshot in initial.channels) {
4949
final streamId = unreadChannelSnapshot.streamId;
5050
final topic = unreadChannelSnapshot.topic;
51-
(streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
51+
final topics = (streams[streamId] ??= makeTopicKeyedMap());
52+
topics.update(topic,
53+
// Older servers differentiate topics case-sensitively, but shouldn't:
54+
// https://github.com/zulip/zulip/pull/31869
55+
// Our topic-keyed map is case-insensitive. When we've seen this
56+
// topic before, modulo case, aggregate instead of clobbering.
57+
// TODO(server-10) simplify away
58+
(value) => setUnion(value, unreadChannelSnapshot.unreadMessageIds),
59+
ifAbsent: () => QueueList.from(unreadChannelSnapshot.unreadMessageIds));
5260
}
5361

5462
for (final unreadDmSnapshot in initial.dms) {
@@ -88,7 +96,10 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
8896
// int count;
8997

9098
/// Unread stream messages, as: stream ID → topic → message IDs (sorted).
91-
final Map<int, Map<TopicName, QueueList<int>>> streams;
99+
///
100+
/// The topic-keyed map is case-insensitive and case-preserving;
101+
/// it comes from [makeTopicKeyedMap].
102+
final Map<int, TopicKeyedMap<QueueList<int>>> streams;
92103

93104
/// Unread DM messages, as: DM narrow → message IDs (sorted).
94105
final Map<DmNarrow, QueueList<int>> dms;
@@ -405,7 +416,7 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
405416
_slowRemoveAllInDms(messageIdsSet);
406417
}
407418
case UpdateMessageFlagsRemoveEvent():
408-
final newlyUnreadInStreams = <int, Map<TopicName, QueueList<int>>>{};
419+
final newlyUnreadInStreams = <int, TopicKeyedMap<QueueList<int>>>{};
409420
final newlyUnreadInDms = <DmNarrow, QueueList<int>>{};
410421
for (final messageId in event.messages) {
411422
final detail = event.messageDetails![messageId];
@@ -420,7 +431,7 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
420431
}
421432
switch (detail.type) {
422433
case MessageType.stream:
423-
final topics = (newlyUnreadInStreams[detail.streamId!] ??= {});
434+
final topics = (newlyUnreadInStreams[detail.streamId!] ??= makeTopicKeyedMap());
424435
final messageIds = (topics[detail.topic!] ??= QueueList());
425436
messageIds.add(messageId);
426437
case MessageType.direct:
@@ -488,14 +499,15 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
488499
}
489500

490501
void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) {
491-
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId);
502+
((streams[streamId] ??= makeTopicKeyedMap())[topic] ??= QueueList())
503+
.addLast(messageId);
492504
}
493505

494506
// [messageIds] must be sorted ascending and without duplicates.
495507
void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, TopicName topic) {
496508
assert(messageIds.isNotEmpty);
497509
assert(isSortedWithoutDuplicates(messageIds));
498-
final topics = streams[streamId] ??= {};
510+
final topics = streams[streamId] ??= makeTopicKeyedMap();
499511
topics.update(topic,
500512
ifAbsent: () => messageIds,
501513
// setUnion dedupes existing and incoming unread IDs,

‎test/model/unreads_test.dart‎

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@ import 'package:zulip/api/model/events.dart';
55
import 'package:zulip/api/model/initial_snapshot.dart';
66
import 'package:zulip/api/model/model.dart';
77
import 'package:zulip/model/algorithms.dart';
8+
import 'package:zulip/model/channel.dart';
89
import 'package:zulip/model/narrow.dart';
910
import 'package:zulip/model/store.dart';
1011
import 'package:zulip/model/unreads.dart';
1112

1213
import '../example_data.dart' as eg;
14+
import '../stdlib_checks.dart';
1315
import 'test_store.dart';
1416
import 'unreads_checks.dart';
1517

@@ -76,7 +78,7 @@ void main() {
7678
assert(Set.of(messages.map((m) => m.id)).length == messages.length,
7779
'checkMatchesMessages: duplicate messages in test input');
7880

79-
final Map<int, Map<TopicName, QueueList<int>>> expectedStreams = {};
81+
final Map<int, TopicKeyedMap<QueueList<int>>> expectedStreams = {};
8082
final Map<DmNarrow, QueueList<int>> expectedDms = {};
8183
final Set<int> expectedMentions = {};
8284
for (final message in messages) {
@@ -85,7 +87,7 @@ void main() {
8587
}
8688
switch (message) {
8789
case StreamMessage():
88-
final perTopic = expectedStreams[message.streamId] ??= {};
90+
final perTopic = expectedStreams[message.streamId] ??= makeTopicKeyedMap();
8991
final messageIds = perTopic[message.topic] ??= QueueList();
9092
messageIds.add(message.id);
9193
case DmMessage():
@@ -136,6 +138,9 @@ void main() {
136138
eg.unreadChannelMsgs(streamId: stream1.streamId, topic: 'b', unreadMessageIds: [3, 4]),
137139
eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'b', unreadMessageIds: [5, 6]),
138140
eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'c', unreadMessageIds: [7, 8]),
141+
142+
// TODO(server-10) drop this (see implementation)
143+
eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'C', unreadMessageIds: [9, 10]),
139144
],
140145
dms: [
141146
UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [11, 12]),
@@ -157,6 +162,8 @@ void main() {
157162
eg.streamMessage(id: 6, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]),
158163
eg.streamMessage(id: 7, stream: stream2, topic: 'c', flags: []),
159164
eg.streamMessage(id: 8, stream: stream2, topic: 'c', flags: []),
165+
eg.streamMessage(id: 9, stream: stream2, topic: 'C', flags: []),
166+
eg.streamMessage(id: 10, stream: stream2, topic: 'C', flags: []),
160167
eg.dmMessage(id: 11, from: user1, to: [eg.selfUser], flags: []),
161168
eg.dmMessage(id: 12, from: user1, to: [eg.selfUser], flags: []),
162169
eg.dmMessage(id: 13, from: user2, to: [eg.selfUser], flags: []),
@@ -201,10 +208,10 @@ void main() {
201208
await store.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted);
202209
fillWithMessages([
203210
eg.streamMessage(stream: stream, topic: 'a', flags: []),
204-
eg.streamMessage(stream: stream, topic: 'a', flags: []),
205-
eg.streamMessage(stream: stream, topic: 'b', flags: []),
211+
eg.streamMessage(stream: stream, topic: 'A', flags: []),
206212
eg.streamMessage(stream: stream, topic: 'b', flags: []),
207213
eg.streamMessage(stream: stream, topic: 'b', flags: []),
214+
eg.streamMessage(stream: stream, topic: 'B', flags: []),
208215
eg.streamMessage(stream: stream, topic: 'c', flags: []),
209216
]);
210217
check(model.countInChannel (stream.streamId)).equals(5);
@@ -220,9 +227,13 @@ void main() {
220227
test('countInTopicNarrow', () {
221228
final stream = eg.stream();
222229
prepare();
223-
fillWithMessages(List.generate(7, (i) => eg.streamMessage(
224-
stream: stream, topic: 'a', flags: [])));
225-
check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(7);
230+
final messages = [
231+
...List.generate(7, (i) => eg.streamMessage(stream: stream, topic: 'a', flags: [])),
232+
...List.generate(2, (i) => eg.streamMessage(stream: stream, topic: 'A', flags: [])),
233+
];
234+
fillWithMessages(messages);
235+
check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(9);
236+
check(model.countInTopicNarrow(stream.streamId, eg.t('A'))).equals(9);
226237
});
227238

228239
test('countInDmNarrow', () {
@@ -370,6 +381,24 @@ void main() {
370381
});
371382
}
372383
});
384+
385+
test('topics case-insensitive but case-preserving', () {
386+
final stream = eg.stream();
387+
final message1 = eg.streamMessage(stream: stream, topic: 'aaa');
388+
final message2 = eg.streamMessage(stream: stream, topic: 'AaA');
389+
final message3 = eg.streamMessage(stream: stream, topic: 'aAa');
390+
prepare();
391+
fillWithMessages([message1]);
392+
model.handleMessageEvent(eg.messageEvent(message2));
393+
model.handleMessageEvent(eg.messageEvent(message3));
394+
checkNotified(count: 2);
395+
checkMatchesMessages([message1, message2, message3]);
396+
// Redundant with checkMatchesMessages, but for explicitness here:
397+
check(model).streams.values.single
398+
.entries.single
399+
..key.equals(eg.t('aaa'))
400+
..value.length.equals(3);
401+
});
373402
});
374403

375404
group('DM messages', () {
@@ -629,6 +658,39 @@ void main() {
629658
checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic));
630659
});
631660

661+
test('topics case-insensitive but case-preserving', () async {
662+
final message1 = eg.streamMessage(stream: origChannel, topic: 'aaa', flags: []);
663+
final message2 = eg.streamMessage(stream: origChannel, topic: 'aaa', flags: []);
664+
final messages = [message1, message2];
665+
await prepareStore();
666+
fillWithMessages(messages);
667+
668+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
669+
// 'AAA' finds the key 'aaa'
670+
origMessages: copyMessagesWith([message1], newTopic: 'AAA'),
671+
newTopicStr: 'bbb'));
672+
checkNotifiedOnce();
673+
checkMatchesMessages([
674+
...copyMessagesWith([message1], newTopic: 'bbb'),
675+
message2,
676+
]);
677+
678+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
679+
origMessages: [message2],
680+
// 'BBB' finds the key 'bbb'
681+
newTopicStr: 'BBB'));
682+
checkNotifiedOnce();
683+
checkMatchesMessages([
684+
...copyMessagesWith([message1], newTopic: 'bbb'),
685+
...copyMessagesWith([message2], newTopic: 'BBB'),
686+
]);
687+
// Redundant with checkMatchesMessages, but for explicitness here:
688+
check(model).streams.values.single
689+
.entries.single
690+
..key.equals(eg.t('bbb'))
691+
..value.length.equals(2);
692+
});
693+
632694
test('tolerates unreads unknown to the model', () async {
633695
await prepareStore();
634696
fillWithMessages(unreadMessages);
@@ -690,14 +752,20 @@ void main() {
690752
fillWithMessages(messages);
691753

692754
final expectedRemainingMessages = Set.of(messages);
755+
assert(messages.any((m) => m.id == 14));
693756
for (final message in messages) {
694757
final event = switch (message) {
695758
StreamMessage() => DeleteMessageEvent(
696759
id: 0,
697760
messageType: MessageType.stream,
698761
messageIds: [message.id],
699762
streamId: message.streamId,
700-
topic: message.topic,
763+
topic: () {
764+
if (message.id != 14) return message.topic;
765+
final uppercase = message.topic.apiName.toUpperCase();
766+
assert(message.topic.apiName != uppercase);
767+
return eg.t(uppercase); // exercise case-insensitivity of topics
768+
}(),
701769
),
702770
DmMessage() => DeleteMessageEvent(
703771
id: 0,

0 commit comments

Comments
 (0)