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

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

Fixes: #980

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

    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.

Yeah, makes sense.

Worth noting some helpful feedback from Greg in review, about the additions to test/model/channel_test.dart: zulip#1608 (comment) > 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 · 2025-07-07T18:06:54Z

Thanks for the review! Revision pushed.

… 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.

…setup

…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).

Worth noting some helpful feedback from Greg in review, about the additions to test/model/channel_test.dart: zulip#1608 (comment) > 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.

gnprice · 2025-07-08T19:03:47Z

Thanks! Looks good; merging.

…nsions I've long had a recurring small annoyance that the "foo_checks.dart" file interferes with tab-completion for "foo_test.dart". For example as mentioned in this comment: zulip#1608 (comment) So here's an approach to solve that, beginning with the "checks" files in test/widgets/. For updating the imports, I used a bit of Perl: $ perl -i -0pe ' s,import .\K(?!dialog)\w*_checks.dart,checks.dart,g ' test/widgets/*_test.dart

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

chrisbobbe force-pushed the pr-topic-case-insensitivity branch from eb15d8b to 8c29baf Compare July 7, 2025 18:06

gnprice added the integration review Added by maintainers when PR may be ready for integration label Jul 8, 2025

chrisbobbe added 8 commits July 8, 2025 12:01

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

c4e03ff

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

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

a518e4f

…setup

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

c1b3734

…c maps

recent_senders: Treat topics case-insensitively

ca17ee3

unreads: Treat topics case-insensitively

7382bdf

Fixes: zulip#980

gnprice force-pushed the pr-topic-case-insensitivity branch from 8c29baf to a81e43a Compare July 8, 2025 19:03

gnprice merged commit a81e43a into zulip:main Jul 8, 2025

gnprice mentioned this pull request Jul 22, 2025

Split avatar, presence, status widgets out from content.dart #1735

Merged

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 7, 2025

Uh oh!

gnprice commented Jul 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants