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

chrisbobbe · 2025-06-10T20:51:35Z

Stacked on #1560.

Many thanks again to @sm-sayedi for your work on this! As with #1560, this is meant to take #1429 across the finish line in time for the upcoming app launch. Thanks again for making sure I had your latest revision of #1429 before you left for vacation. 🙂

In these new commits:

In "Direct messages" (the recent-DMs page), we exclude conversations where everyone is muted.
In "Combined feed", "Starred", and "Mentions", we exclude messages in those conversations.
In the typing-status text ("Chris Bobbe is typing…"), we exclude muted users.

Fixes: #296

chrisbobbe · 2025-06-12T19:39:32Z

Revision pushed; this one also excludes muted users from the new new-DM UI, which landed after my last revision.

gnprice

Thanks! I've read so far the first three commits:
8af150c recent-dms: Exclude DM conversations where all other recipients are muted
06937e2 msglist [nfc]: Place _allMessagesVisible right after _messageVisible
8339c64 msglist [nfc]: s/VisibilityEffect/UserTopicVisibilityEffect/

and the next one except for its tests:
898a2ad msglist: In combined/mentions/starred, exclude DMs if all recipients muted

Generally all looks good; a few small comments below.

That leaves four more commits that I haven't read (though I've skimmed them):
619eb24 autocomplete: Exclude muted users from user-mention autocomplete
87df780 msglist [nfc]: Represent nobody-is-typing as null, not empty list
c40cb89 msglist: Exclude muted users from typing-status text
0acc63b new-dm: Exclude muted users

gnprice · 2025-06-12T23:39:45Z

lib/model/channel.dart

-      (false, true) => VisibilityEffect.unmuted,
-      (true, false) => VisibilityEffect.muted,
-      _             => VisibilityEffect.none,
+      (false, true) => UserTopicVisibilityEffect.unmuted,
+      (true, false) => UserTopicVisibilityEffect.muted,
+      _             => UserTopicVisibilityEffect.none,


This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it.

Yeah, agreed.

Some of the verbosity (like in these lines) will at least get resolved by the upcoming Dart feature of "dot shorthands", expected later this year:
https://github.com/dart-lang/language/blob/main/working/3616%20-%20enum%20value%20shorthand/proposal-simple-lrhn.md

I believe with that feature we'll be able to say just .unmuted, .muted, .none here, without the name of the enum type.

gnprice · 2025-06-12T23:44:02Z

lib/model/message_list.dart

+            !store.shouldMuteDmConversation(DmNarrow(
+              allRecipientIds: allRecipientIds, selfUserId: store.selfUserId)),


Let's make another DmNarrow constructor for this:

Suggested change

!store.shouldMuteDmConversation(DmNarrow(

allRecipientIds: allRecipientIds, selfUserId: store.selfUserId)),

!store.shouldMuteDmConversation(DmNarrow.ofConversation(

message.conversation, selfUserId: store.selfUserId)),

Compare the comment at the top of that class:

// Zulip has many ways of representing a DM conversation; for example code // handling many of them, see zulip-mobile:src/utils/recipient.js . // Please add more constructors and getters here to handle any of those // as we turn out to need them. class DmNarrow extends Narrow implements SendableNarrow {

It'd be bad if this logic did something inefficient like copy the list of recipient IDs, because we're doing this for potentially a large fraction of the messages in this feed (if a user uses lots of DMs). I believe this in fact doesn't make such a copy — and a constructor on DmNarrow is a good place to centralize coming to that conclusion.

Huh…it looks like DmNarrow.ofMessage, though, does copy the list of recipient IDs:

factory DmNarrow.ofMessage(MessageBase<DmConversation> message, { required int selfUserId, }) { return DmNarrow( allRecipientIds: List.unmodifiable(message.conversation.allRecipientIds), selfUserId: selfUserId, ); }

Hmm indeed.

It looks like we don't currently call that one in any place that's as performance-sensitive as this. But it's a bit of a pitfall lying around.

gnprice · 2025-06-12T23:48:04Z

lib/model/message_list.dart

+    switch (narrow) {
+      case CombinedFeedNarrow():
+      case ChannelNarrow():
      case MentionsNarrow():
      case StarredMessagesNarrow():
+        return false;
+
+      case TopicNarrow():
+      case DmNarrow():
        return true;


nit: let's keep these cases in the same order as in _messageVisible, for ease of comparing the logic. It's important that the two align (that's this method's job); and it's potentially a bit subtle to confirm that, so it's good to make it as easy as possible.

It's fine for this to have two separate groups of cases that return the same thing.

gnprice · 2025-06-12T23:48:46Z

lib/model/message_list.dart

+  MutedUsersVisibilityEffect _mutedUsersEventCanAffectVisibility(MutedUsersEvent event) {
+    switch(narrow) {
      case CombinedFeedNarrow():


similarly, put the cases in the same order as in _messageVisible

gnprice · 2025-06-12T23:52:10Z

lib/model/user.dart

+    final union = setUnion(sortedOld, sortedNew);
+
+    final willMuteSome = sortedOld.length < union.length;
+    final willUnmuteSome = sortedNew.length < union.length;


Neat, yeah, this logic works.

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

chrisbobbe · 2025-06-13T02:15:18Z

Thanks! Revision pushed.

gnprice · 2025-06-13T02:49:32Z

Thanks! Those changes look good.

There's a CI failure in build_runner.

(And there are parts of the branch I still need to read, per #1561 (review) .)

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

chrisbobbe · 2025-06-13T02:51:57Z

I just rebased atop current main (after the dependency-upgrades PR), in case that fixes CI. 🤔

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

gnprice

OK, and read those remaining 4 commits. Just small comments below.

gnprice · 2025-06-19T01:00:11Z

lib/model/autocomplete.dart

    // TODO(#236) test email too, not just name
    if (!user.isActive) return false;
+    if (store.isUserMuted(user.userId)) return false;

    return _testName(user, cache);


nit:

Suggested change

// TODO(#236) test email too, not just name

if (!user.isActive) return false;

if (store.isUserMuted(user.userId)) return false;

return _testName(user, cache);

if (!user.isActive) return false;

if (store.isUserMuted(user.userId)) return false;

// TODO(#236) test email too, not just name

return _testName(user, cache);

The comment was already a bit separated from what it applied to, so perhaps should have been moved down there sooner; but this would widen the separation, so let's fix it.

(Can stay in the same commit, no need to make a separate prep commit.)

gnprice · 2025-06-19T01:06:41Z

lib/widgets/message_list.dart

    final typistIds = model!.typistIdsInNarrow(narrow);
-    if (typistIds.isEmpty) return const SizedBox();
-    final text = switch (typistIds.length) {
+    if (typistIds == null) return const SizedBox();


msglist [nfc]: Represent nobody-is-typing as null, not empty list Just to skip the bit of extra work of creating new lists.

Hmm, this feels like the API gets a bit more complicated from the caller's perspective. What if typistIds is empty, and why is there a different possibility which is that it's null?

(In reality I think it's never empty after this change — _removeTypist removes the sub-maps when empty. But that's not obvious from the outside.)

gnprice · 2025-06-19T01:07:45Z

lib/model/typing_status.dart

-  Iterable<int> typistIdsInNarrow(SendableNarrow narrow) =>
-    _timerMapsByNarrow[narrow]?.keys ?? [];
+  Iterable<int>? typistIdsInNarrow(SendableNarrow narrow) =>
+    _timerMapsByNarrow[narrow]?.keys;


An alternate way of not creating new lists each time:

Iterable<int> typistIdsInNarrow(SendableNarrow narrow) => _timerMapsByNarrow[narrow]?.keys ?? const [];

gnprice · 2025-06-19T01:08:30Z

lib/widgets/message_list.dart

+    final filteredTypistIds = typistIds
+      .whereNot((userId) => store.isUserMuted(userId));


nit: can use a tear-off, which is shorter and I think here a bit easier to read:

Suggested change

final filteredTypistIds = typistIds

.whereNot((userId) => store.isUserMuted(userId));

final filteredTypistIds = typistIds.whereNot(store.isUserMuted);

gnprice · 2025-06-19T01:11:56Z

test/widgets/message_list_test.dart

          expected: 'Third User and Fourth User are typing…');
+        await store.setMutedUsers([eg.thirdUser.userId]);
+        await tester.pump();
+        check(tester.widget<Text>(finder)).data.equals('Fourth User is typing…');


Let's put this in a separate test case. This one (or this pair of them) is pretty long and a bit unfocused already.

The new test case can be for just a topic narrow; I don't think it needs to exercise both kinds of conversations.

gnprice · 2025-06-19T01:12:55Z

lib/widgets/new_dm_sheet.dart

+    final sansMuted = store.allUsers
+      .whereNot((User user) => store.isUserMuted(user.userId));


nit: omit boring types

Suggested change

final sansMuted = store.allUsers

.whereNot((User user) => store.isUserMuted(user.userId));

final sansMuted = store.allUsers

.whereNot((user) => store.isUserMuted(user.userId));

…uted Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

This is solely for a better order.

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

…muted In the future, this should apply to the ReactionsNarrow from the Web app too, once we have it. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

Just to skip the bit of extra work of creating new lists.

Following web: zulip#1429 (review)

This completes the planned work for muting muted users (hooray!). Fixes: zulip#296

gnprice · 2025-07-03T04:02:13Z

(Just rebased after merging #1560.)

chrisbobbe requested a review from gnprice June 10, 2025 20:51

chrisbobbe assigned gnprice Jun 10, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 10, 2025

chrisbobbe mentioned this pull request Jun 12, 2025

Distinguish muted users in UI #1429

Closed

chrisbobbe force-pushed the pr-chris-muted-users-2 branch from 9c82301 to 0acc63b Compare June 12, 2025 19:39

gnprice reviewed Jun 13, 2025

View reviewed changes

chrisbobbe force-pushed the pr-chris-muted-users-2 branch from 0acc63b to ceb4e0f Compare June 13, 2025 02:15

chrisbobbe force-pushed the pr-chris-muted-users-2 branch from ceb4e0f to f3f4df2 Compare June 13, 2025 02:51

gnprice force-pushed the pr-chris-muted-users-2 branch from f3f4df2 to 5438a65 Compare June 13, 2025 06:26

gnprice mentioned this pull request Jun 17, 2025

Mute muted users #296

Open

gnprice reviewed Jun 19, 2025

View reviewed changes

chrisbobbe and others added 8 commits July 2, 2025 21:00

recent-dms: Exclude DM conversations where all other recipients are m…

e77bd17

…uted Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

msglist [nfc]: Place _allMessagesVisible right after _messageVisible

6214c61

This is solely for a better order.

msglist: In combined/mentions/starred, exclude DMs if all recipients …

1bc9a51

…muted In the future, this should apply to the ReactionsNarrow from the Web app too, once we have it. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

autocomplete: Exclude muted users from user-mention autocomplete

9c1cc16

Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

msglist [nfc]: Represent nobody-is-typing as null, not empty list

3bffc93

Just to skip the bit of extra work of creating new lists.

msglist: Exclude muted users from typing-status text

fa81750

Following web: zulip#1429 (review)

new-dm: Exclude muted users

79986a1

This completes the planned work for muting muted users (hooray!). Fixes: zulip#296

gnprice force-pushed the pr-chris-muted-users-2 branch from 5438a65 to 79986a1 Compare July 3, 2025 04:01

		!store.shouldMuteDmConversation(DmNarrow(
		allRecipientIds: allRecipientIds, selfUserId: store.selfUserId)),

		final filteredTypistIds = typistIds
		.whereNot((userId) => store.isUserMuted(userId));

	final filteredTypistIds = typistIds
	.whereNot((userId) => store.isUserMuted(userId));
	final filteredTypistIds = typistIds.whereNot(store.isUserMuted);

		final sansMuted = store.allUsers
		.whereNot((User user) => store.isUserMuted(user.userId));

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

chrisbobbe commented Jun 10, 2025

Uh oh!

chrisbobbe commented Jun 12, 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!

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 Jun 13, 2025

Uh oh!

gnprice commented Jun 13, 2025

Uh oh!

chrisbobbe commented Jun 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jul 3, 2025

Uh oh!

Uh oh!

chrisbobbe commented Jun 12, 2025 •

edited

Loading

chrisbobbe commented Jun 13, 2025 •

edited

Loading