-
Notifications
You must be signed in to change notification settings - Fork 308
Muted users prep #1530
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?
Muted users prep #1530
Conversation
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! Generally this looks good. Comments below.
lib/api/model/model.dart
Outdated
} | ||
|
||
|
||
/// An item in the [InitialSnapshot.mutedUsers] or [MutedUsersEvent]. |
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: single blank line
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic should get tests. (Doesn't need very many.)
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.
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. 🙂
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.
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.)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".)
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
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. |
Ah: Alya spotted a glitch with the slashed-eye icon; I should fix that before we merge this: #1560 (comment) |
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 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.
// Mobile doesn't use the timestamp; ignore. | ||
// final int timestamp; |
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
@@ -91,6 +98,13 @@ class UserStoreImpl extends PerAccountStoreBase with UserStore { | |||
@override | |||
Iterable<User> get allUsers => _users.values; | |||
|
|||
final Set<int> _mutedUsers; |
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
The first four prep commits of #1429 (decided in 1429 comment), towards #296.