model: Treat topics case-insensitively in Unreads, topic-visibility, and recent-senders #1608

chrisbobbe · 2025-06-18T22:47:24Z

Fixes: #980

… tests And fix a test that was breaking an invariant in its setup.

…ages And change some callers slightly, to adapt. Since fillWithMessages is implemented as handling a sequence of new-message events, require the messages to be sorted without duplicates, for realism. This doesn't change what the model's state looks like for any of the tests, because messages in the input with the "read" flag are ignored by model.handleMessageEvent, and we're not changing the order of the unread messages. Also, the previous commit started checking the invariant that the model's message-ID lists are sorted.

…etup

…c maps

Fixes: zulip#980

The event's doc says: https://zulip.com/api/get-events#user_topic > Event sent to a user's clients when the user mutes/unmutes a > topic, or otherwise modifies their personal per-topic > configuration. The new name is still accurate for "adding" a configuration (i.e. setting it to something not-"none"), but now also covers updating a configuration (moving between not-"none" configurations) and removing one (resetting to "none).

gnprice

Thanks for taking care of this! Generally this all looks good; mostly small comments below.

(I just saw you'd marked it for maintainer review first, ah well — already wrote this review, so posting it.)

gnprice · 2025-06-19T01:26:49Z

test/model/unreads_test.dart

        eg.streamMessage(id: 7, stream: stream2, topic: 'c', flags: []),
        eg.streamMessage(id: 8, stream: stream2, topic: 'c', flags: []),
-        eg.dmMessage(id: 9,  from: user1, to: [eg.selfUser], flags: []),
-        eg.dmMessage(id: 10, from: user1, to: [eg.selfUser], flags: []),


nit:

unreads test: Make room for two more message IDs (7, 8) in a test's setup

9, 10, right?

gnprice · 2025-06-19T01:32:09Z

lib/model/channel.dart

+LinkedHashMap<TopicName, V> makeTopicKeyedMap<V>() => LinkedHashMap<TopicName, V>(
+  equals: (a, b) => a.isSameAs(b),
+  hashCode: (k) => k.canonicalize().hashCode,


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 call toLowerCase 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.)

gnprice · 2025-06-19T01:32:52Z

lib/model/channel.dart

+LinkedHashMap<TopicName, V> makeTopicKeyedMap<V>() => LinkedHashMap<TopicName, V>(
+  equals: (a, b) => a.isSameAs(b),
+  hashCode: (k) => k.canonicalize().hashCode,
+  isValidKey: (k) => k is TopicName,


Is this argument needed? It looks from the docs like this is the default behavior.

gnprice · 2025-06-19T02:05:01Z

lib/model/recent_senders.dart

-  final Map<int, Map<TopicName, Map<int, MessageIdTracker>>> topicSenders = {};
+  final Map<int, LinkedHashMap<TopicName, Map<int, MessageIdTracker>>> topicSenders = {};


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 {}. If these were some other (reasonable) Map implementation instead, that'd be fine too as long as they used the particular equals and hashCode etc. that makeTopicKeyedMap passes.

I think just Map here would be fine (and better than saying LinkedHashMap). It could also be a type alias, like TopicKeyedMap<Map<int, …>> — that'd express the desired meaning, even though it wouldn't be enforced.

Apropos of this: the upstream tree just converted places that were saying LinkedHashMap to say plain Map:
flutter/flutter#170713
which apparently will be needed as part of a future lint change there.

gnprice · 2025-06-19T02:25:29Z

test/model/recent_senders_checks.dart

+extension MessageIdTrackerChecks on Subject<MessageIdTracker> {
+  Subject<QueueList<int>> get ids => has((x) => x.ids, 'ids');


nit: let's put this inside recent_senders_test.dart — for checks-extensions that are unlikely to be used outside a given test file, I've preferred doing it that way

I'm not totally satisfied with that pattern, because sometimes things are used in two or three files, and sometimes they start out used in just one and later are used in another, so that we end up having one test file import another which feels a little unclean.

But the trade-off is that when this file is present, it makes it harder to type a command like flutter test test/model/recent_senders_test.dart — the tab-completion has to stop at the …/recent_senders_ to ask if the next character should be a t or a c 🙂

gnprice · 2025-06-19T02:29:12Z

test/model/recent_senders_test.dart

+      check(model.topicSenders).values.single.entries.single
+        ..key.equals(eg.t('thing'))
+        ..value.entries.single.which((it) => it
+          ..key.equals(userX.userId)
+          ..value.isA<MessageIdTracker>().ids.length.equals(2));


This felt a bit verbose to me, so I tried simplifying it by using deepEquals.

Not sure whether this is an improvement — the need for a Condition in there does dampen things:

check(model.topicSenders).values.single.deepEquals( {eg.t('thing'): {userX.userId: (Subject<Object?> it) => it.isA<MessageIdTracker>().ids.length.equals(2)}});

Maybe better if a bit more expanded in formatting:

check(model.topicSenders).values.single.deepEquals( {eg.t('thing'): {userX.userId: (Subject<Object?> it) => it.isA<MessageIdTracker>().ids.length.equals(2)}});

gnprice · 2025-06-19T02:37:30Z

lib/model/unreads.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>>>{};


Searching this file for "Map<TopicName", it looks like there's one more spot that probably needs makeTopicKeyedMap: in handleUpdateMessageFlagsEvent when marking unread.

gnprice · 2025-06-19T02:42:34Z

lib/model/unreads.dart

+      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);


This will look up topic in the map two or three times. Since this is in a potentially hot path (processing the unreads data from the initial snapshot, so often 50k message IDs across perhaps 1000s of conversations), let's avoid that: topics.update(topic, …).

gnprice · 2025-06-19T02:52:52Z

test/model/test_store.dart

-  Future<void> addUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async {
+  Future<void> setUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async {


test [nfc]: s/addUserTopic/setUserTopic/ in PerAccountStoreTestExtension The event's doc says: https://zulip.com/api/get-events#user_topic > Event sent to a user's clients when the user mutes/unmutes a > topic, or otherwise modifies their personal per-topic > configuration. The new name is still accurate for "adding" a configuration (i.e. setting it to something not-"none"), but now also covers updating a configuration (moving between not-"none" configurations) and removing one (resetting to "none).

Ah, good thought.

gnprice · 2025-06-19T03:06:10Z

test/model/channel_test.dart

+          // Case-insensitive
+          check(store.topicVisibilityPolicy(stream1.streamId, eg.t('ToPiC')))
+            .equals(policy);


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-none policies, rather than all three.

These are fine, though. No need to rewrite them — we have higher-priority things to do.

chrisbobbe added 8 commits June 17, 2025 14:24

unreads test: Add/use a checkInvariants function like in message-list…

c1c2fe2

… tests And fix a test that was breaking an invariant in its setup.

unreads test: Make room for two more message IDs (7, 8) in a test's s…

3c6626e

…etup

model: Add makeTopicKeyedMap for case-insensitive-but-preserving topi…

0972bee

…c maps

recent_senders: Treat topics case-insensitively

e6932df

unreads: Treat topics case-insensitively

59f7531

Fixes: zulip#980

model: Treat topics case-insensitively in topic-visibility

eb15d8b

chrisbobbe requested review from gnprice and rajveermalviya June 18, 2025 22:47

chrisbobbe assigned rajveermalviya Jun 18, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jun 18, 2025

gnprice reviewed Jun 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

model: Treat topics case-insensitively in Unreads, topic-visibility, and recent-senders #1608

model: Treat topics case-insensitively in Unreads, topic-visibility, and recent-senders #1608

Uh oh!

chrisbobbe commented Jun 18, 2025

Uh oh!

gnprice left a comment

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 21, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

gnprice Jun 19, 2025

Uh oh!

Uh oh!

		final Map<int, Map<TopicName, Map<int, MessageIdTracker>>> topicSenders = {};
		final Map<int, LinkedHashMap<TopicName, Map<int, MessageIdTracker>>> topicSenders = {};

		extension MessageIdTrackerChecks on Subject<MessageIdTracker> {
		Subject<QueueList<int>> get ids => has((x) => x.ids, 'ids');

		Future<void> addUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async {
		Future<void> setUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async {

model: Treat topics case-insensitively in Unreads, topic-visibility, and recent-senders #1608

Are you sure you want to change the base?

model: Treat topics case-insensitively in Unreads, topic-visibility, and recent-senders #1608

Uh oh!

Conversation

chrisbobbe commented Jun 18, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!