Mute muted users (Chris's revision, 1 of 2) #1560
Conversation
chrisbobbe
added
the
integration review
|
"Reveal message" button and "Muted user" placeholder in the message list, then "Hide muted message again" option in the action sheet:
|
|
The slashed eye icon seems to be missing half its iris. It doesn't have that effect on web.
|
|
The screenshots look fine to me otherwise. |
There was a problem hiding this comment.
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.
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.
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**.
lib/widgets/message_list.dart
Outdated
| /// 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.
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.
There was a problem hiding this comment.
I think this is moot with the InheritedNotifier / RevealedMutedMessagesState approach: #1560 (comment)
Leafward widgets won't need MessageListPageState in their build methods (they'll use RevealedMutedMessagesState), so we don't need to provide MessageListPageState via an inherited widget.
lib/widgets/message_list.dart
Outdated
| 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.
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.
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.
chrisbobbe
force-pushed
the
pr-chris-muted-users-1
branch
from
c90fa9d to
9b3cb35
Compare
|
Oops, just noticed in the 0.0.31 beta that I dropped a piece of #1429 🙂 we should disable gestures on hidden messages in the message list (tap sender-row to go to profile; long-press to bring up message action sheet). Fixed. |
chrisbobbe
force-pushed
the
pr-chris-muted-users-1
branch
from
9b3cb35 to
f99e6d8
Compare
|
Done; revision pushed! This is on Sayed's revision of the prep PR from today; please see updated screenshots: #1530 (comment) |
|
I don't think I've seen this CI failure before. I'll trigger it to rerun: (edit: it worked!) |
There was a problem hiding this comment.
Thanks for the revision! The revision's changes all look good, modulo one comment below.
From my review above at #1560 (review) , I guess I still need to read the remainder of the last commit:
f99e6d8 msglist: Hide content of muted messages, with a "Reveal message" button
lib/widgets/message_list.dart
Outdated
| final revealedMutedMessages = MessageListPage.revealedMutedMessagesOf(context); | ||
| final showAsMuted = store.isUserMuted(message.senderId) | ||
| && message is Message // i.e., not an outbox message | ||
| && !revealedMutedMessages.isMutedMessageRevealed((message as Message).id); |
There was a problem hiding this comment.
Usually isUserMuted will be false, so we can optimize this by saving the revealedMutedMessagesOf lookup (and the resulting dependency tracking) in that case.
chrisbobbe
force-pushed
the
pr-chris-muted-users-1
branch
from
f99e6d8 to
ba57e41
Compare
|
Thanks! Revision pushed. |
|
Thanks! That looks good. So what remains is for me to read the remainder of the last commit: |
chrisbobbe
force-pushed
the
pr-chris-muted-users-1
branch
from
ba57e41 to
cd93194
Compare
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.)
- The new-DM sheet, but we'll be excluding muted users, coming up in
a separate commit.
- @-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 <[email protected]>
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 <[email protected]>
…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 <[email protected]>
…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 <[email protected]>
gnprice
force-pushed
the
pr-chris-muted-users-1
branch
from
cd93194 to
9cd2972
Compare
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.userDisplayNameandstore.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.)InheritedWidgetstrategy to work (Distinguish muted users in UI #1429 (comment) ); I think this is simpler and clearer.ZulipWebUiKitButtonfor the "Reveal message" button, rather than making a new ad hoc button.ZulipWebUiKitButtonhandles 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