Muted users prep #1530

sm-sayedi · 2025-05-24T03:24:41Z

The first four prep commits of #1429 (decided in 1429 comment), towards #296.

gnprice

Thanks! Generally this looks good. Comments below.

gnprice · 2025-05-28T00:37:11Z

lib/api/model/model.dart

 }

+
+/// An item in the [InitialSnapshot.mutedUsers] or [MutedUsersEvent].


nit: single blank line

gnprice · 2025-05-28T00:42:26Z

lib/model/user.dart

@@ -66,6 +68,24 @@ mixin UserStore on PerAccountStoreBase {
    return getUser(message.senderId)?.fullName
      ?? message.senderFullName;
  }
+
+  /// All the users muted by [selfUser], sorted by [MutedUserItem.id] ascending.
+  List<MutedUserItem> get mutedUsers;


Do we ever use the timestamps on these? I think we don't have a need to use them.

The user store can then just have a Set<int> for the muted users by user ID.

In fact I think the Set can remain private on UserStoreImpl, and UserStore itself just offer the bool isUserMuted method. That keeps things a bit more encapsulated.

gnprice · 2025-05-28T00:46:48Z

lib/model/user.dart

+  ///
+  /// By default, looks for the user id in [UserStore.mutedUsers] unless
+  /// [mutedUsers] is non-null, in which case looks in the latter.
+  bool isUserMuted(int id, {List<MutedUserItem>? mutedUsers});


What's the use case for having this take a mutedUsers argument? Is it about deciding whether a given event will affect a given user? There might be another way to arrange the API for that which might feel cleaner.

Yeah, it's used in MessageListView.handleMutedUsersEvent to remove all the messages of a conversation whose all recipients are just muted by the event. We're removing those messages before the new muted users data is tracked in store, so we can't compare with the store's muted users data.

Cool.

Topic muting has the same problem to solve, and ChannelStore handles it with the willChangeIfTopicVisible and willChangeIfTopicVisibleInStream methods.

Here, the clearest API might be to have this method take an optional MutedUsersEvent, rather than an optional List. That makes it more apparent what the point of the parameter is, and how to properly use it. Relatedly, it allows this file to better encapsulate the knowledge of how to interpret a MutedUsersEvent, allowing the rest of the app to not worry about that.

(I might have a firmer idea of that API when I see how this is used in the subsequent PRs, though.)

gnprice · 2025-05-28T00:48:59Z

lib/model/user.dart

  final Map<int, User> _users;

+  @override
+  final List<MutedUserItem> mutedUsers;
+
  @override
  User? getUser(int userId) => _users[userId];


nit: let's group the mutedUsers data structure with the isUserMuted method that operates on it, and _users with getUser and allUsers (the same way as in the interface/mixin definition UserStore)

gnprice · 2025-05-28T00:49:54Z

lib/model/user.dart

+  bool isUserMuted(int id, {List<MutedUserItem>? mutedUsers}) {
+    return binarySearchByKey(
+      mutedUsers == null ? this.mutedUsers : _sortMutedUsers(mutedUsers), id,
+      (item, id) => item.id.compareTo(id)) >= 0;
+  }
+
+  static List<MutedUserItem> _sortMutedUsers(List<MutedUserItem> mutedUsers) {
+    return mutedUsers..sort((a, b) => a.id.compareTo(b.id));


This logic should get tests. (Doesn't need very many.)

From the start, I was intimidated by using a sorted list (now Set) for storing the muted users' data. In practice, there will probably be very few muted users to keep track of, and binary-searching an item in a small list will probably not gain much. Last night, Chris and I discussed it and decided to go with an unsorted Set. Looking online, I think for most cases looking up in a Set takes linear time anyway.
If using a sorted Set is better, I will be happy to revert to this version. 🙂

Yeah, in fact checking for membership in a Set should take constant time (aka O(1) time), because it's implemented as a hash table. (Sometimes a set gets implemented as another structure like a sorted list or a binary tree, and then checking for membership takes O(log n) time, which is still pretty fast.)

gnprice · 2025-05-28T18:05:42Z

lib/model/user.dart

+  bool allRecipientsMuted(DmNarrow narrow, {List<MutedUserItem>? mutedUsers}) {
+    return !narrow.otherRecipientIds.any((id) => !isUserMuted(id, mutedUsers: mutedUsers));


This returns true for the self-1:1 thread. That seems undesired 🙂

(Noticed that when making a draft-release branch with #1429.)

This method could be left out of this PR, and added in a later commit in the same PR where it ends up getting used. That would probably be helpful for choosing a name and docs that match its desired behavior. (Which is probably more like "should mute this DM conversation", i.e. in terms of the desired effect in the UI, given that this wrinkle means it won't match a mathematically clean description like "all recipients are muted".)

sm-sayedi · 2025-06-04T02:03:03Z

Thanks @gnprice for the review. New revision pushed, PTAL.

So it doesn't look like we just forgot about this field.

Making the Set private was Greg's suggestion in review: zulip#1530 (comment) It required changing some tests to use `isUserMuted` instead, and removing some helper methods on PerAccountStoreTestExtension that weren't used anyway. We can add it back later if needed, or do something else.

Also renamed "user" to "two_person" to make it consistent with other icons of the same purpose. New icons taken from: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-237884&t=dku3J5Fv2dmWo7ht-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-264632&t=dku3J5Fv2dmWo7ht-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-291260&t=B5jAmeUMMG4dlsU7-0

chrisbobbe · 2025-06-09T00:03:18Z

Thanks for the revision! Since you and Greg are both on vacation, and with the launch approaching so soon, I've taken the liberty of adding some "squash" commits—changes I would make if it were my PR, that Greg can squash in if he wants—to help save time in review.

chrisbobbe · 2025-06-10T20:59:34Z

Ah: Alya spotted a glitch with the slashed-eye icon; I should fix that before we merge this: #1560 (comment)

gnprice

Thanks for the revision!

This generally all looks good modulo the icon glitch mentioned above, and squashing the squash commits. Just one comment below that suggests anything to change.

gnprice · 2025-06-11T06:12:36Z

lib/api/model/model.dart

+  // Mobile doesn't use the timestamp; ignore.
+  // final int timestamp;


Cool, this is good.

In #1530 (comment) I actually only meant to suggest removing the timestamps from the model, not the API binding. Removing them from the API binding is fine too, but definitely should have this comment as Chris's commit message says:

chris squash: leave comment about MutedUserItem.timestamp So it doesn't look like we just forgot about this field.

gnprice · 2025-06-11T06:16:09Z

lib/model/user.dart

+  bool isUserMuted(int id, {List<MutedUserItem>? mutedUsers}) {
+    return binarySearchByKey(
+      mutedUsers == null ? this.mutedUsers : _sortMutedUsers(mutedUsers), id,
+      (item, id) => item.id.compareTo(id)) >= 0;
+  }
+
+  static List<MutedUserItem> _sortMutedUsers(List<MutedUserItem> mutedUsers) {
+    return mutedUsers..sort((a, b) => a.id.compareTo(b.id));


Yeah, in fact checking for membership in a Set should take constant time (aka O(1) time), because it's implemented as a hash table. (Sometimes a set gets implemented as another structure like a sorted list or a binary tree, and then checking for membership takes O(log n) time, which is still pretty fast.)

gnprice · 2025-06-11T06:23:27Z

lib/model/user.dart

@@ -91,6 +98,13 @@ class UserStoreImpl extends PerAccountStoreBase with UserStore {
  @override
  Iterable<User> get allUsers => _users.values;

+  final Set<int> _mutedUsers;


chris squash: Make _mutedUsers private; adjust some names/docs Making the Set private was Greg's suggestion in review: https://github.com/zulip/zulip-flutter/pull/1530#discussion_r2110615283 It required changing some tests to use `isUserMuted` instead, and removing some helper methods on PerAccountStoreTestExtension that weren't used anyway. We can add it back later if needed, or do something else.

Yeah, looks good.

If we later feel that seeing the whole Set would be really helpful for testing, we can add a getter like

Iterable<int> debugMutedUsers => _mutedUsers;

and tests can compare using unorderedEquals. The "debug" prefix makes clear that getter isn't something the app should be invoking in normal operation.

gnprice · 2025-06-11T06:32:02Z

lib/model/user.dart

+  ///
+  /// By default, looks for the user id in [UserStore.mutedUsers] unless
+  /// [mutedUsers] is non-null, in which case looks in the latter.
+  bool isUserMuted(int id, {List<MutedUserItem>? mutedUsers});


Cool.

Topic muting has the same problem to solve, and ChannelStore handles it with the willChangeIfTopicVisible and willChangeIfTopicVisibleInStream methods.

Here, the clearest API might be to have this method take an optional MutedUsersEvent, rather than an optional List. That makes it more apparent what the point of the parameter is, and how to properly use it. Relatedly, it allows this file to better encapsulate the knowledge of how to interpret a MutedUsersEvent, allowing the rest of the app to not worry about that.

(I might have a firmer idea of that API when I see how this is used in the subsequent PRs, though.)

sm-sayedi requested a review from gnprice May 24, 2025 03:25

sm-sayedi assigned gnprice May 24, 2025

sm-sayedi added the integration review Added by maintainers when PR may be ready for integration label May 24, 2025

gnprice reviewed May 28, 2025

View reviewed changes

sm-sayedi mentioned this pull request Jun 4, 2025

Distinguish muted users in UI #1429

Closed

sm-sayedi force-pushed the 296-muted-users-prep branch from a2e86f4 to e82dd6d Compare June 4, 2025 01:13

sm-sayedi requested a review from gnprice June 4, 2025 02:04

sm-sayedi and others added 7 commits June 8, 2025 16:11

api: Add InitialSnapshot.mutedUsers

2984999

chris squash: leave comment about MutedUserItem.timestamp

3d84404

So it doesn't look like we just forgot about this field.

api: Add muted_users event

d822c63

user: Add UserStore.mutedUsers with helper methods, updating with events

c7a5cbc

chris squash: _toUserIds can be inlined

4438ff8

chrisbobbe force-pushed the 296-muted-users-prep branch from e82dd6d to 3669b15 Compare June 9, 2025 00:00

chrisbobbe mentioned this pull request Jun 10, 2025

Mute muted users (Chris's revision, 1 of 2) #1560

Open

gnprice reviewed Jun 11, 2025

View reviewed changes

		}


		/// An item in the [InitialSnapshot.mutedUsers] or [MutedUsersEvent].

		bool allRecipientsMuted(DmNarrow narrow, {List<MutedUserItem>? mutedUsers}) {
		return !narrow.otherRecipientIds.any((id) => !isUserMuted(id, mutedUsers: mutedUsers));

		// Mobile doesn't use the timestamp; ignore.
		// final int timestamp;

Muted users prep #1530

Are you sure you want to change the base?

Muted users prep #1530

Uh oh!

Conversation

sm-sayedi commented May 24, 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!

sm-sayedi Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sm-sayedi commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jun 9, 2025

Uh oh!

chrisbobbe commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

sm-sayedi Jun 4, 2025 •

edited

Loading

sm-sayedi commented Jun 4, 2025 •

edited

Loading

chrisbobbe commented Jun 10, 2025 •

edited

Loading