-
Notifications
You must be signed in to change notification settings - Fork 314
model: Treat topics case-insensitively in Unreads, topic-visibility, and recent-senders #1608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c1c2fe2
fe297da
3c6626e
0972bee
e6932df
59f7531
17c8951
eb15d8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
import 'dart:collection'; | ||
|
||
import 'package:flutter/foundation.dart'; | ||
|
||
import '../api/model/events.dart'; | ||
|
@@ -41,6 +43,8 @@ mixin ChannelStore { | |
/// | ||
/// For policies directly applicable in the UI, see | ||
/// [isTopicVisibleInStream] and [isTopicVisible]. | ||
/// | ||
/// Topics are treated case-insensitively; see [TopicName.isSameAs]. | ||
UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic); | ||
|
||
/// The raw data structure underlying [topicVisibilityPolicy]. | ||
|
@@ -171,13 +175,13 @@ class ChannelStoreImpl with ChannelStore { | |
streams.putIfAbsent(stream.streamId, () => stream); | ||
} | ||
|
||
final topicVisibility = <int, Map<TopicName, UserTopicVisibilityPolicy>>{}; | ||
final topicVisibility = <int, LinkedHashMap<TopicName, UserTopicVisibilityPolicy>>{}; | ||
for (final item in initialSnapshot.userTopics ?? const <UserTopicItem>[]) { | ||
if (_warnInvalidVisibilityPolicy(item.visibilityPolicy)) { | ||
// Not a value we expect. Keep it out of our data structures. // TODO(log) | ||
continue; | ||
} | ||
final forStream = topicVisibility.putIfAbsent(item.streamId, () => {}); | ||
final forStream = topicVisibility.putIfAbsent(item.streamId, () => makeTopicKeyedMap()); | ||
forStream[item.topicName] = item.visibilityPolicy; | ||
} | ||
|
||
|
@@ -204,9 +208,9 @@ class ChannelStoreImpl with ChannelStore { | |
final Map<int, Subscription> subscriptions; | ||
|
||
@override | ||
Map<int, Map<TopicName, UserTopicVisibilityPolicy>> get debugTopicVisibility => topicVisibility; | ||
Map<int, LinkedHashMap<TopicName, UserTopicVisibilityPolicy>> get debugTopicVisibility => topicVisibility; | ||
|
||
final Map<int, Map<TopicName, UserTopicVisibilityPolicy>> topicVisibility; | ||
final Map<int, LinkedHashMap<TopicName, UserTopicVisibilityPolicy>> topicVisibility; | ||
|
||
@override | ||
UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) { | ||
|
@@ -365,8 +369,18 @@ class ChannelStoreImpl with ChannelStore { | |
topicVisibility.remove(event.streamId); | ||
} | ||
} else { | ||
final forStream = topicVisibility.putIfAbsent(event.streamId, () => {}); | ||
final forStream = topicVisibility.putIfAbsent(event.streamId, () => makeTopicKeyedMap()); | ||
forStream[event.topicName] = visibilityPolicy; | ||
} | ||
} | ||
} | ||
|
||
/// Make a case-insensitive, case-preserving [TopicName]-keyed [LinkedHashMap]. | ||
/// | ||
/// The equality function is [TopicName.isSameAs], | ||
/// and the hash code is [String.hashCode] of [TopicName.canonicalize]. | ||
LinkedHashMap<TopicName, V> makeTopicKeyedMap<V>() => LinkedHashMap<TopicName, V>( | ||
equals: (a, b) => a.isSameAs(b), | ||
hashCode: (k) => k.canonicalize().hashCode, | ||
isValidKey: (k) => k is TopicName, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this argument needed? It looks from the docs like this is the default behavior. |
||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,12 @@ | ||
import 'dart:collection'; | ||
|
||
import 'package:collection/collection.dart'; | ||
import 'package:flutter/foundation.dart'; | ||
|
||
import '../api/model/events.dart'; | ||
import '../api/model/model.dart'; | ||
import 'algorithms.dart'; | ||
import 'channel.dart'; | ||
|
||
/// Tracks the latest messages sent by each user, in each stream and topic. | ||
/// | ||
|
@@ -16,7 +19,7 @@ class RecentSenders { | |
|
||
// topicSenders[streamId][topic][senderId] = MessageIdTracker | ||
@visibleForTesting | ||
final Map<int, Map<TopicName, Map<int, MessageIdTracker>>> topicSenders = {}; | ||
final Map<int, LinkedHashMap<TopicName, Map<int, MessageIdTracker>>> topicSenders = {}; | ||
Comment on lines
-19
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this doesn't really express the meaning we care about. It is a LinkedHashMap, true — but so is what you'd get from just I think just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apropos of this: the upstream tree just converted places that were saying LinkedHashMap to say plain Map: |
||
|
||
/// The latest message the given user sent to the given stream, | ||
/// or null if no such message is known. | ||
|
@@ -27,6 +30,8 @@ class RecentSenders { | |
|
||
/// The latest message the given user sent to the given topic, | ||
/// or null if no such message is known. | ||
/// | ||
/// Topics are treated case-insensitively; see [TopicName.isSameAs]. | ||
int? latestMessageIdOfSenderInTopic({ | ||
required int streamId, | ||
required TopicName topic, | ||
|
@@ -53,7 +58,7 @@ class RecentSenders { | |
} | ||
for (final entry in messagesByUserInTopic.entries) { | ||
final (streamId, topic, senderId) = entry.key; | ||
(((topicSenders[streamId] ??= {})[topic] ??= {}) | ||
(((topicSenders[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) | ||
[senderId] ??= MessageIdTracker()).addAll(entry.value); | ||
} | ||
} | ||
|
@@ -64,7 +69,7 @@ class RecentSenders { | |
final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; | ||
((streamSenders[streamId] ??= {}) | ||
[senderId] ??= MessageIdTracker()).add(messageId); | ||
(((topicSenders[streamId] ??= {})[topic] ??= {}) | ||
(((topicSenders[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) | ||
[senderId] ??= MessageIdTracker()).add(messageId); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import 'dart:collection'; | ||
import 'dart:core'; | ||
|
||
import 'package:collection/collection.dart'; | ||
|
@@ -41,14 +42,25 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { | |
required CorePerAccountStore core, | ||
required ChannelStore channelStore, | ||
}) { | ||
final streams = <int, Map<TopicName, QueueList<int>>>{}; | ||
final streams = <int, LinkedHashMap<TopicName, QueueList<int>>>{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Searching this file for "Map<TopicName", it looks like there's one more spot that probably needs |
||
final dms = <DmNarrow, QueueList<int>>{}; | ||
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); | ||
final topics = (streams[streamId] ??= makeTopicKeyedMap()); | ||
if (topics[topic] != null) { | ||
// Older servers differentiate topics case-sensitively, but shouldn't: | ||
// https://github.com/zulip/zulip/pull/31869 | ||
// Our topic-keyed map is case-insensitive. When we've seen this | ||
// topic before, modulo case, aggregate instead of clobbering. | ||
// TODO(server-10) simplify away | ||
topics[topic] = | ||
setUnion(topics[topic]!, unreadChannelSnapshot.unreadMessageIds); | ||
} else { | ||
topics[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds); | ||
Comment on lines
+53
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will look up |
||
} | ||
} | ||
|
||
for (final unreadDmSnapshot in initial.dms) { | ||
|
@@ -88,7 +100,10 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { | |
// int count; | ||
|
||
/// Unread stream messages, as: stream ID → topic → message IDs (sorted). | ||
final Map<int, Map<TopicName, QueueList<int>>> streams; | ||
/// | ||
/// The topic-keyed map is case-insensitive and case-preserving; | ||
/// it comes from [makeTopicKeyedMap]. | ||
final Map<int, LinkedHashMap<TopicName, QueueList<int>>> streams; | ||
|
||
/// Unread DM messages, as: DM narrow → message IDs (sorted). | ||
final Map<DmNarrow, QueueList<int>> dms; | ||
|
@@ -485,14 +500,15 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { | |
} | ||
|
||
void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) { | ||
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId); | ||
((streams[streamId] ??= makeTopicKeyedMap())[topic] ??= QueueList()) | ||
.addLast(messageId); | ||
} | ||
|
||
// [messageIds] must be sorted ascending and without duplicates. | ||
void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, TopicName topic) { | ||
assert(messageIds.isNotEmpty); | ||
assert(isSortedWithoutDuplicates(messageIds)); | ||
final topics = streams[streamId] ??= {}; | ||
final topics = streams[streamId] ??= makeTopicKeyedMap(); | ||
topics.update(topic, | ||
ifAbsent: () => messageIds, | ||
// setUnion dedupes existing and incoming unread IDs, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
|
||
import 'package:checks/checks.dart'; | ||
import 'package:test/scaffolding.dart'; | ||
import 'package:zulip/api/model/events.dart'; | ||
|
@@ -7,7 +6,9 @@ import 'package:zulip/api/model/model.dart'; | |
import 'package:zulip/model/store.dart'; | ||
import 'package:zulip/model/channel.dart'; | ||
|
||
import '../api/model/model_checks.dart'; | ||
import '../example_data.dart' as eg; | ||
import '../stdlib_checks.dart'; | ||
import 'test_store.dart'; | ||
|
||
void main() { | ||
|
@@ -146,7 +147,7 @@ void main() { | |
|
||
test('with nothing for topic', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.muted); | ||
await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.muted); | ||
check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) | ||
.equals(UserTopicVisibilityPolicy.none); | ||
}); | ||
|
@@ -158,9 +159,13 @@ void main() { | |
UserTopicVisibilityPolicy.unmuted, | ||
UserTopicVisibilityPolicy.followed, | ||
]) { | ||
await store.addUserTopic(stream1, 'topic', policy); | ||
await store.setUserTopic(stream1, 'topic', policy); | ||
check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) | ||
.equals(policy); | ||
|
||
// Case-insensitive | ||
check(store.topicVisibilityPolicy(stream1.streamId, eg.t('ToPiC'))) | ||
.equals(policy); | ||
Comment on lines
+166
to
+168
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my ideal version of these tests would have most of the new checks in this file be separate test cases, rather than added to existing tests. This also doesn't need a full matrix with the other variables: for example, this check can be done with any one of the non- These are fine, though. No need to rewrite them — we have higher-priority things to do. |
||
} | ||
}); | ||
}); | ||
|
@@ -193,40 +198,68 @@ void main() { | |
final store = eg.store(); | ||
await store.addStream(stream1); | ||
await store.addSubscription(eg.subscription(stream1)); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isFalse(); | ||
check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isFalse(); | ||
|
||
// Case-insensitive | ||
check(store.isTopicVisibleInStream(stream1.streamId, eg.t('ToPiC'))).isFalse(); | ||
check(store.isTopicVisible (stream1.streamId, eg.t('ToPiC'))).isFalse(); | ||
}); | ||
|
||
test('with policy unmuted', () async { | ||
final store = eg.store(); | ||
await store.addStream(stream1); | ||
await store.addSubscription(eg.subscription(stream1, isMuted: true)); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); | ||
check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); | ||
check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); | ||
|
||
// Case-insensitive | ||
check(store.isTopicVisibleInStream(stream1.streamId, eg.t('tOpIc'))).isTrue(); | ||
check(store.isTopicVisible (stream1.streamId, eg.t('tOpIc'))).isTrue(); | ||
}); | ||
|
||
test('with policy followed', () async { | ||
final store = eg.store(); | ||
await store.addStream(stream1); | ||
await store.addSubscription(eg.subscription(stream1, isMuted: true)); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); | ||
check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); | ||
check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); | ||
|
||
// Case-insensitive | ||
check(store.isTopicVisibleInStream(stream1.streamId, eg.t('TOPIC'))).isTrue(); | ||
check(store.isTopicVisible (stream1.streamId, eg.t('TOPIC'))).isTrue(); | ||
}); | ||
}); | ||
|
||
group('willChangeIfTopicVisible/InStream', () { | ||
UserTopicEvent mkEvent(UserTopicVisibilityPolicy policy) => | ||
eg.userTopicEvent(stream1.streamId, 'topic', policy); | ||
|
||
// For testing case-insensitivity | ||
UserTopicEvent mkEventDifferentlyCased(UserTopicVisibilityPolicy policy) => | ||
eg.userTopicEvent(stream1.streamId, 'ToPiC', policy); | ||
|
||
assert(() { | ||
// (sanity check on mkEvent and mkEventDifferentlyCased) | ||
final event1 = mkEvent(UserTopicVisibilityPolicy.followed); | ||
final event2 = mkEventDifferentlyCased(UserTopicVisibilityPolicy.followed); | ||
return event1.topicName.isSameAs(event2.topicName) | ||
&& event1.topicName.apiName != event2.topicName.apiName; | ||
}()); | ||
|
||
void checkChanges(PerAccountStore store, | ||
UserTopicVisibilityPolicy newPolicy, | ||
VisibilityEffect expectedInStream, VisibilityEffect expectedOverall) { | ||
final event = mkEvent(newPolicy); | ||
check(store.willChangeIfTopicVisibleInStream(event)).equals(expectedInStream); | ||
check(store.willChangeIfTopicVisible (event)).equals(expectedOverall); | ||
|
||
final event2 = mkEventDifferentlyCased(newPolicy); | ||
check(store.willChangeIfTopicVisibleInStream(event2)).equals(expectedInStream); | ||
check(store.willChangeIfTopicVisible (event2)).equals(expectedOverall); | ||
} | ||
|
||
test('stream not muted, policy none -> followed, no change', () async { | ||
|
@@ -340,16 +373,16 @@ void main() { | |
group('events', () { | ||
test('add with new stream', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
compareTopicVisibility(store, [ | ||
eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.muted), | ||
]); | ||
}); | ||
|
||
test('add in existing stream', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); | ||
compareTopicVisibility(store, [ | ||
eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.muted), | ||
eg.userTopicItem(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted), | ||
|
@@ -358,35 +391,43 @@ void main() { | |
|
||
test('update existing policy', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); | ||
compareTopicVisibility(store, [ | ||
eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.unmuted), | ||
]); | ||
|
||
// case-insensitivity | ||
await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.followed); | ||
compareTopicVisibility(store, [ | ||
eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.followed), | ||
]); | ||
}); | ||
|
||
test('remove, with others in stream', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); | ||
compareTopicVisibility(store, [ | ||
eg.userTopicItem(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted), | ||
]); | ||
}); | ||
|
||
test('remove, as last in stream', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
// case-insensitivity | ||
await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.none); | ||
compareTopicVisibility(store, [ | ||
]); | ||
}); | ||
|
||
test('treat unknown enum value as removing', () async { | ||
final store = eg.store(); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unknown); | ||
await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); | ||
// case-insensitivity | ||
await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.unknown); | ||
compareTopicVisibility(store, [ | ||
]); | ||
}); | ||
|
@@ -403,12 +444,39 @@ void main() { | |
])); | ||
check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 1'))) | ||
.equals(UserTopicVisibilityPolicy.muted); | ||
check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 2'))) | ||
// case-insensitivity | ||
check(store.topicVisibilityPolicy(stream.streamId, eg.t('ToPiC 2'))) | ||
.equals(UserTopicVisibilityPolicy.unmuted); | ||
check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 3'))) | ||
.equals(UserTopicVisibilityPolicy.followed); | ||
check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 4'))) | ||
.equals(UserTopicVisibilityPolicy.none); | ||
}); | ||
}); | ||
|
||
group('makeTopicKeyedMap', () { | ||
test('"a" equals "A"', () { | ||
final map = makeTopicKeyedMap<int>() | ||
..[eg.t('a')] = 1 | ||
..[eg.t('A')] = 2; | ||
check(map) | ||
..[eg.t('a')].equals(2) | ||
..[eg.t('A')].equals(2) | ||
..entries.which((it) => it.single | ||
..key.apiName.equals('a') | ||
..value.equals(2)); | ||
}); | ||
|
||
test('"A" equals "a"', () { | ||
final map = makeTopicKeyedMap<int>() | ||
..[eg.t('A')] = 1 | ||
..[eg.t('a')] = 2; | ||
check(map) | ||
..[eg.t('A')].equals(2) | ||
..[eg.t('a')].equals(2) | ||
..entries.which((it) => it.single | ||
..key.apiName.equals('A') | ||
..value.equals(2)); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the one way this is less efficient than a FoldDict approach is that when looking up a key and finding a match (or a match of the hash code), the FoldDict only needs to call
toLowerCase
on the new key because it already has the lower-case form of the key that's in the map; this version needs to calltoLowerCase
twice.It's probably fine. If later we find this is hot in profiling, we can optimize.
(And if we do, we might go farther anyway: turn TopicName into a class instead of an extension type, memoizing its canonicalized form. That would make FoldDict unnecessary too.)