Muted users prep #1530
Muted users prep #1530
Conversation
sm-sayedi
added
the
integration review
lib/api/model/model.dart
Outdated
| } | ||
|
|
||
|
|
||
| /// An item in the [InitialSnapshot.mutedUsers] or [MutedUsersEvent]. |
lib/model/user.dart
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/model/user.dart
Outdated
| /// | ||
| /// 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}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/model/user.dart
Outdated
| final Map<int, User> _users; | ||
|
|
||
| @override | ||
| final List<MutedUserItem> mutedUsers; | ||
|
|
||
| @override | ||
| User? getUser(int userId) => _users[userId]; |
There was a problem hiding this comment.
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)
lib/model/user.dart
Outdated
| 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)); |
There was a problem hiding this comment.
This logic should get tests. (Doesn't need very many.)
There was a problem hiding this comment.
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. 🙂
lib/model/user.dart
Outdated
| bool allRecipientsMuted(DmNarrow narrow, {List<MutedUserItem>? mutedUsers}) { | ||
| return !narrow.otherRecipientIds.any((id) => !isUserMuted(id, mutedUsers: mutedUsers)); |
There was a problem hiding this comment.
This returns true for the self-1:1 thread. That seems undesired 🙂
(Noticed that when making a draft-release branch with #1429.)
There was a problem hiding this comment.
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
force-pushed
the
296-muted-users-prep
branch
from
a2e86f4 to
e82dd6d
Compare
|
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
force-pushed
the
296-muted-users-prep
branch
from
e82dd6d to
3669b15
Compare
|
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. |
The first four prep commits of #1429 (decided in 1429 comment), towards #296.