-
Notifications
You must be signed in to change notification settings - Fork 308
Mute muted users (Chris's revision, 1 of 2) #1560
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?
Conversation
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
Like userDisplayName does. And remove a null-check `store.getUser(userId)!` at one of the callers... I think that's *probably* NFC, for the reason given in a comment ("must exist because UserMentionAutocompleteResult"). But it's possible this is actually a small bugfix involving a rare race involving our batch-processing of autocomplete results. Related: zulip#716
We've now centralized on store.userDisplayName and store.senderDisplayName for all the code that's responsible for showing a user's name on the screen, except for a few places we use `User.fullName` for (a) the self-user and (b) to create an @-mention for the compose box. The "(unknown user)" and upcoming "Muted user" placeholders aren't needed for (a) or (b).
(Done by adding an is-muted condition in store.userDisplayName and store.senderDisplayName, with an opt-out param.) If Chris is muted, we'll now show "Muted user" where before we would show "Chris Bobbe", in the following places: - Message-list page: - DM-narrow app bar. - DM recipient headers. - The sender row on messages. This and message content will get more treatment in a separate commit. - Emoji-reaction chips on messages. - Typing indicators ("Muted user is typing…"), but we'll be excluding muted users, coming up in a separate commit. - Voter names in poll messages. - DM items in the Inbox page. (Messages from muted users are automatically marked as read, but they can end up in the inbox if you un-mark them as read.) - @-mention autocomplete, but we'll be excluding muted users, coming up in a separate commit. - Items in the Direct messages ("recent DMs") page. We'll be excluding DMs where everyone is muted, coming up in a separate commit. - User items in custom profile fields. We *don't* do this replacement in the following places, i.e., we'll still show "Chris Bobbe" if Chris is muted: - Sender name in the header of the lightbox page. (This follows web.) - The "hint text" for the compose box in a DM narrow: it will still say "Message @chris Bobbe", not "Message @muted user". (This follows web.) - The user's name at the top of the Profile page. - We won't generate malformed @-mention syntax like @_**Muted user|13313**. Co-authored-by: Sayed Mahmood Sayedi <smahmood.sayedi@gmail.com>
We're free to rename these because they don't correspond to named variables in the Figma. These more general names will be used for an avatar placeholder for muted users, coming up. Co-authored-by: Sayed Mahmood Sayedi <smahmood.sayedi@gmail.com>
…able (Done by adding an is-muted condition in Avatar and AvatarImage, with an opt-out param. Similar to how we handled users' names in a recent commit.) If a user is muted, we'll now show a placeholder where before we would have shown their real avatar, in the following places: - The sender row on messages in the message list. This and message content will get more treatment in a separate commit. - @-mention autocomplete, but we'll be excluding muted users, coming up in a separate commit. - User items in custom profile fields. - 1:1 DM items in the Direct messages ("recent DMs") page. But we'll be excluding those items there, coming up in a separate commit. We *don't* do this replacement in the following places, i.e., we'll still show the real avatar: - The header of the lightbox page. (This follows web.) - The big avatar at the top of the profile page. Co-authored-by: Sayed Mahmood Sayedi <smahmood.sayedi@gmail.com>
Soon we'll want to access some state in the build method of a leafward widget, for muted-users. So, follow the TODO on MessageListPage.ancestorOf.
…erRow No-op because the child Flexible -> GestureDetector -> Row has the default MainAxisSize.max, filling the available space, leaving none that would be controlled by spaceBetween.
For muted-users, coming up. This was ad hoc for mobile, for the "Reveal message" button on a message from a muted sender: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6092-50786&m=dev
For muted-users, coming up. This is consistent with the ad hoc design for muted-users, but also consistent with the component in "Zulip Web UI Kit". (Modulo the TODO for changing icon-to-label gap from 8px to 6px; that's tricky with the Material widget we're working with.)
… type For muted-users, coming up. Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6092-50795&m=dev
Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6089-28385&t=28DdYiTs6fXWR9ua-0 Co-authored-by: Sayed Mahmood Sayedi <smahmood.sayedi@gmail.com>
The screenshots look fine to me otherwise. |
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.
Thanks for the careful implementation! I've read the first N-1 commits:
e4db89d users: Have userDisplayEmail handle unknown users
46dbe1e lightbox: Use senderDisplayName for sender's name
cdb2dd2 compose: Fix error on quote-and-replying message from unknown sender
23413ab users [nfc]: Use userDisplayName at last non-self-user sites in widgets/
1107261 muted-users: Say "Muted user" to replace a user's name, where applicable
fe9f5be theme [nfc]: Rename some variables that aren't named variables in Figma
96bfe26 muted-users: Use placeholder for avatars of muted users, where applicable
794544c msglist [nfc]: Provide MessageListPageState via an inherited widget
cdcb1bc msglist [nfc]: Remove a no-op MainAxisAlignment.spaceBetween in _SenderRow
168c787 button [nfc]: Have ZulipWebUiKitButton support a smaller, ad hoc size
731d714 button [nfc]: Have ZulipWebUiKitButton support an icon
d5553a4 button [nfc]: Have ZulipWebUiKitButton support ad hoc minimal-neutral type
and partway through the last one:
c90fa9d msglist: Hide content of muted messages, with a "Reveal message" button
// Could ask `mention` to omit the |<id> part unless the mention is ambiguous… | ||
// but that would mean a linear scan through all users, and the extra noise | ||
// won't much matter with the already probably-long message link in there too. |
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 think this comment is still relevant for explaining a choice we made about how this logic works.
(The name mention
needs updating, though — already did, to say userMention
.)
String userDisplayName(int userId) { | ||
String userDisplayName(int userId, {bool replaceIfMuted = true}) { | ||
if (replaceIfMuted && isUserMuted(userId)) { | ||
return GlobalLocalizations.zulipLocalizations.mutedUser; | ||
} |
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.
muted-users: Say "Muted user" to replace a user's name, where applicable
(Done by adding an is-muted condition in store.userDisplayName and
store.senderDisplayName, with an opt-out param.)
Cool, I like this structure.
The inventory of where we say "Muted user" and where we don't is also helpful:
If Chris is muted, we'll now show "Muted user" where before we would
show "Chris Bobbe", in the following places:
- Message-list page:
- DM-narrow app bar.
- DM recipient headers.
- The sender row on messages. This and message content will get
more treatment in a separate commit.
- Emoji-reaction chips on messages.
- Typing indicators ("Muted user is typing…"), but we'll be
excluding muted users, coming up in a separate commit.
- Voter names in poll messages.
- DM items in the Inbox page. (Messages from muted users are
automatically marked as read, but they can end up in the inbox if
you un-mark them as read.)
- @-mention autocomplete, but we'll be excluding muted users, coming
up in a separate commit.
- Items in the Direct messages ("recent DMs") page. We'll be
excluding DMs where everyone is muted, coming up in a separate
commit.
- User items in custom profile fields.
We *don't* do this replacement in the following places, i.e., we'll
still show "Chris Bobbe" if Chris is muted:
- Sender name in the header of the lightbox page. (This follows
web.)
- The "hint text" for the compose box in a DM narrow: it will still
say "Message @Chris Bobbe", not "Message @Muted user". (This
follows web.)
- The user's name at the top of the Profile page.
- We won't generate malformed @-mention syntax like
@_**Muted user|13313**.
/// Uses the inefficient [BuildContext.findAncestorStateOfType]; | ||
/// don't call this in a build method. | ||
// If we do find ourselves wanting this in a build method, it won't be hard | ||
// to enable that: we'd just need to add an [InheritedWidget] here. | ||
/// Uses the efficient [BuildContext.dependOnInheritedWidgetOfExactType], | ||
/// so this may be called in a build method. | ||
/// | ||
/// Because this uses [BuildContext.dependOnInheritedWidgetOfExactType], | ||
/// it creates a dependency, and [context] will rebuild when the underlying | ||
/// [State.setState] is called. | ||
static MessageListPageState ancestorOf(BuildContext context) { |
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.
nit: Let's rename to just of
, either in this commit or right after it. The "ancestor of" name was meant to give a bit of pause when one considers calling it, because it suggests the idea of walking up the tree through ancestors, whereas .of
is the usual convention for finding an InheritedWidget.
bool updateShouldNotify(covariant _MessageListPageInheritedWidget oldWidget) { | ||
// Ensure that dependent elements using [MessageListPage.ancestorOf] | ||
// always rebuild when _MessageListPageState.setState is called. | ||
return true; |
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.
This isn't specific to _MessageListPageState.setState
, is it? I believe this means the dependent elements will get rebuilt any time the MessageListPage gets rebuilt. Which could e.g. be because one of its own dependencies was updated.
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.
Maybe the cleanest thing would be to have the InheritedWidget be an InheritedNotifier, specifically for the set of muted messages. That set could be an object like this:
class RevealedMutedMessagesState extends ChangeNotifier {
final Set<int> _revealedMessages = {};
bool isMutedMessageRevealed(int messageId) =>
_revealedMessages.contains(messageId);
void _add(int messageId) {
_revealedMessages.add(messageId);
notifyListeners();
}
// … _remove …
}
Then the bit on _MessageListPageState
would look something like this:
final _revealedMutedMessages = RevealedMutedMessagesState();
@override
void revealMutedMessage(int messageId) {
_revealedMutedMessages._add(messageId);
}
And it wouldn't have an isMutedMessageRevealed
method directly; instead, there'd be a static on MessageListPage that looks like static RevealedMutedMessagesState revealedMutedMessagesOf(BuildContext context)
. A caller that needs isMutedMessageRevealed
starts by calling that static, which creates a dependency; then it calls isMutedMessageRevealed
on that; and the dependency ensures that the caller gets notified if the data changes.
Stacked atop #1530.
Thanks @sm-sayedi for your work on muted-users, and for making sure I had your latest revision of #1429 before you left for vacation! 🙂
With the launch coming up, and since you're on vacation, I've gone ahead with taking #1429 forward with my own changes, as we discussed last week, rather than doing another round of review.
This PR is part of that work; more to come. In particular, this PR doesn't include the part of #1429 that excludes muted users and messages. It only affects how they appear in the UI.
Notable differences from #1429:
store.userDisplayName
andstore.senderDisplayName
, as suggested in the issue and in Distinguish muted users in UI #1429 (review). (With some commits beforehand, to use those methods everywhere it makes sense to use them.)InheritedWidget
strategy to work (Distinguish muted users in UI #1429 (comment) ); I think this is simpler and clearer.ZulipWebUiKitButton
for the "Reveal message" button, rather than making a new ad hoc button.ZulipWebUiKitButton
handles subtle things like a scale animation on press, and making sure its touch target is 44px tall even if the painted button is shorter (24px in this case).DesignVariables
)Related: #296