Skip to content

Commit d4be4d7

Browse files
chrisbobbechrisbobbe
committed
unreads: Treat topics case-insensitively
Fixes: zulip#980
1 parent fbbf986 commit d4be4d7

File tree

2 files changed

+99
-13
lines changed

2 files changed

+99
-13
lines changed

‎lib/model/unreads.dart‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import 'dart:collection';
12
import 'dart:core';
23

34
import 'package:collection/collection.dart';
@@ -41,14 +42,25 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
4142
required CorePerAccountStore core,
4243
required ChannelStore channelStore,
4344
}) {
44-
final streams = <int, Map<TopicName, QueueList<int>>>{};
45+
final streams = <int, LinkedHashMap<TopicName, QueueList<int>>>{};
4546
final dms = <DmNarrow, QueueList<int>>{};
4647
final mentions = Set.of(initial.mentions);
4748

4849
for (final unreadChannelSnapshot in initial.channels) {
4950
final streamId = unreadChannelSnapshot.streamId;
5051
final topic = unreadChannelSnapshot.topic;
51-
(streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
52+
final topics = (streams[streamId] ??= makeTopicKeyedMap());
53+
if (topics[topic] != null) {
54+
// Older servers differentiate topics case-sensitively, but shouldn't:
55+
// https://github.com/zulip/zulip/pull/31869
56+
// Our topic-keyed map is case-insensitive. When we've seen this
57+
// topic before, modulo case, aggregate instead of clobbering.
58+
// TODO(server-10) simplify away
59+
topics[topic] =
60+
setUnion(topics[topic]!, unreadChannelSnapshot.unreadMessageIds);
61+
} else {
62+
topics[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
63+
}
5264
}
5365

5466
for (final unreadDmSnapshot in initial.dms) {
@@ -88,7 +100,10 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
88100
// int count;
89101

90102
/// Unread stream messages, as: stream ID → topic → message IDs (sorted).
91-
final Map<int, Map<TopicName, QueueList<int>>> streams;
103+
///
104+
/// The topic-keyed map is case-insensitive and case-preserving;
105+
/// it comes from [makeTopicKeyedMap].
106+
final Map<int, LinkedHashMap<TopicName, QueueList<int>>> streams;
92107

93108
/// Unread DM messages, as: DM narrow → message IDs (sorted).
94109
final Map<DmNarrow, QueueList<int>> dms;
@@ -485,14 +500,15 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
485500
}
486501

487502
void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) {
488-
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId);
503+
((streams[streamId] ??= makeTopicKeyedMap())[topic] ??= QueueList())
504+
.addLast(messageId);
489505
}
490506

491507
// [messageIds] must be sorted ascending and without duplicates.
492508
void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, TopicName topic) {
493509
assert(messageIds.isNotEmpty);
494510
assert(isSortedWithoutDuplicates(messageIds));
495-
final topics = streams[streamId] ??= {};
511+
final topics = streams[streamId] ??= makeTopicKeyedMap();
496512
topics.update(topic,
497513
ifAbsent: () => messageIds,
498514
// setUnion dedupes existing and incoming unread IDs,

‎test/model/unreads_test.dart‎

Lines changed: 78 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,19 @@
1+
import 'dart:collection';
2+
13
import 'package:checks/checks.dart';
24
import 'package:collection/collection.dart';
35
import 'package:test/scaffolding.dart';
46
import 'package:zulip/api/model/events.dart';
57
import 'package:zulip/api/model/initial_snapshot.dart';
68
import 'package:zulip/api/model/model.dart';
79
import 'package:zulip/model/algorithms.dart';
10+
import 'package:zulip/model/channel.dart';
811
import 'package:zulip/model/narrow.dart';
912
import 'package:zulip/model/store.dart';
1013
import 'package:zulip/model/unreads.dart';
1114

1215
import '../example_data.dart' as eg;
16+
import '../stdlib_checks.dart';
1317
import 'test_store.dart';
1418
import 'unreads_checks.dart';
1519

@@ -76,7 +80,7 @@ void main() {
7680
assert(Set.of(messages.map((m) => m.id)).length == messages.length,
7781
'checkMatchesMessages: duplicate messages in test input');
7882

79-
final Map<int, Map<TopicName, QueueList<int>>> expectedStreams = {};
83+
final Map<int, LinkedHashMap<TopicName, QueueList<int>>> expectedStreams = {};
8084
final Map<DmNarrow, QueueList<int>> expectedDms = {};
8185
final Set<int> expectedMentions = {};
8286
for (final message in messages) {
@@ -85,7 +89,7 @@ void main() {
8589
}
8690
switch (message) {
8791
case StreamMessage():
88-
final perTopic = expectedStreams[message.streamId] ??= {};
92+
final perTopic = expectedStreams[message.streamId] ??= makeTopicKeyedMap();
8993
final messageIds = perTopic[message.topic] ??= QueueList();
9094
messageIds.add(message.id);
9195
case DmMessage():
@@ -136,6 +140,9 @@ void main() {
136140
eg.unreadChannelMsgs(streamId: stream1.streamId, topic: 'b', unreadMessageIds: [3, 4]),
137141
eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'b', unreadMessageIds: [5, 6]),
138142
eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'c', unreadMessageIds: [7, 8]),
143+
144+
// TODO(server-10) drop this (see implementation)
145+
eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'C', unreadMessageIds: [9, 10]),
139146
],
140147
dms: [
141148
UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [11, 12]),
@@ -157,6 +164,8 @@ void main() {
157164
eg.streamMessage(id: 6, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]),
158165
eg.streamMessage(id: 7, stream: stream2, topic: 'c', flags: []),
159166
eg.streamMessage(id: 8, stream: stream2, topic: 'c', flags: []),
167+
eg.streamMessage(id: 9, stream: stream2, topic: 'C', flags: []),
168+
eg.streamMessage(id: 10, stream: stream2, topic: 'C', flags: []),
160169
eg.dmMessage(id: 11, from: user1, to: [eg.selfUser], flags: []),
161170
eg.dmMessage(id: 12, from: user1, to: [eg.selfUser], flags: []),
162171
eg.dmMessage(id: 13, from: user2, to: [eg.selfUser], flags: []),
@@ -201,10 +210,10 @@ void main() {
201210
await store.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted);
202211
fillWithMessages([
203212
eg.streamMessage(stream: stream, topic: 'a', flags: []),
204-
eg.streamMessage(stream: stream, topic: 'a', flags: []),
205-
eg.streamMessage(stream: stream, topic: 'b', flags: []),
213+
eg.streamMessage(stream: stream, topic: 'A', flags: []),
206214
eg.streamMessage(stream: stream, topic: 'b', flags: []),
207215
eg.streamMessage(stream: stream, topic: 'b', flags: []),
216+
eg.streamMessage(stream: stream, topic: 'B', flags: []),
208217
eg.streamMessage(stream: stream, topic: 'c', flags: []),
209218
]);
210219
check(model.countInChannel (stream.streamId)).equals(5);
@@ -220,9 +229,13 @@ void main() {
220229
test('countInTopicNarrow', () {
221230
final stream = eg.stream();
222231
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);
232+
final messages = [
233+
...List.generate(7, (i) => eg.streamMessage(stream: stream, topic: 'a', flags: [])),
234+
...List.generate(2, (i) => eg.streamMessage(stream: stream, topic: 'A', flags: [])),
235+
];
236+
fillWithMessages(messages);
237+
check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(9);
238+
check(model.countInTopicNarrow(stream.streamId, eg.t('A'))).equals(9);
226239
});
227240

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

375406
group('DM messages', () {
@@ -629,6 +660,39 @@ void main() {
629660
checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic));
630661
});
631662

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

692756
final expectedRemainingMessages = Set.of(messages);
757+
assert(messages.any((m) => m.id == 14));
693758
for (final message in messages) {
694759
final event = switch (message) {
695760
StreamMessage() => DeleteMessageEvent(
696761
id: 0,
697762
messageType: MessageType.stream,
698763
messageIds: [message.id],
699764
streamId: message.streamId,
700-
topic: message.topic,
765+
topic: () {
766+
if (message.id != 14) return message.topic;
767+
final uppercase = message.topic.apiName.toUpperCase();
768+
assert(message.topic.apiName != uppercase);
769+
return eg.t(uppercase); // exercise case-insensitivity of topics
770+
}(),
701771
),
702772
DmMessage() => DeleteMessageEvent(
703773
id: 0,

0 commit comments

Comments
 (0)