Implement stream and topic muting #423
Conversation
| switch (subscriptions[streamId]?.isMuted) { | ||
| case false: return true; | ||
| case true: return false; | ||
| case null: return false; // not subscribed; treat like muted |
There was a problem hiding this comment.
The test failures are because I need to make this line more conditional — in some contexts (like the inbox), we want to treat a not-subscribed stream the same as a muted stream, but in others (like the message list), we want to treat it the same as a subscribed and not muted stream.
Marking as draft until I fix that, but in the meantime the rest of the code is ready to review.
|
Very excited to see this!! Now my inbox view will be usable, since it won't have noise from all the stuff I've muted. 😅 Hmm. Take a look at how the "Mark X as read" button works in the all-messages narrow:
(edit: updated the "Stream" row for #423 (comment).) Is that right? That seems like a bug. Looking at my account now, it means I might click a button that says "Mark 28 messages as read", and instead, 5444 messages would get marked as read. That seems misleading. Could maybe resolve by offering a dialog that asks the user if they want to mark everything as read, or just what's not muted / what they're seeing in the list? |
Yeah, that does. I believe the corresponding button in web has the same behavior — but it doesn't say the number of messages. So the fix might just be to not mention the number of unread messages. (After all, do you really need the number? You're already looking at the view where they appear… well unless they're muted, I guess.) In zulip-mobile the corresponding banner has the number, but separately from the button so it doesn't have the same confusion. And the number serves somewhat more of a purpose there, because the banner appears even if you're scrolled up in history (so that it lets you know there's unreads below, e.g. if they came in while you were reading the old history). We probably should do something to serve that purpose, but this button at the bottom of the list doesn't anyway, so it can probably drop the number. |
|
In the "Stream" row of your table, the intended behavior is that the messages shown and the count of unreads disregard any muting of the stream, but don't include muted topics. If you're seeing them include muted topics, that's a bug. |
|
Ah right; I just confirmed that it works as expected. When looking at the web app, I had thought a particular topic was muted, when it was actually in the "default" policy (and the stream was muted). I updated the table. I guess that means the stream-narrow mark-as-read button is also misleading, like the one in "All messages". There could be messages in muted topics that aren't shown in the list, and aren't represented in the unread-count number, but that would be affected by the mark-as-read button. Could do the same fix (removing the number from the mark-as-read button). |
This comment was marked as resolved.
This comment was marked as resolved.
|
Apart from my comments above (and the failing test that you mentioned), this looks good. Thanks! |
|
Thanks for the review! Back from vacation today, and just pushed a revision. PTAL. |
|
LGTM, thanks! The CI failure should resolve after #447; would you review and merge that please? Then please merge this at will. |
This gets us automatic checking for exhaustiveness when we write switch statements on these.
This gives us more space to make this implementation more complex, and to add more features, without overwhelming PerAccountStore and lib/model/store.dart .
…that This invariant should always be maintained by the server, so this mostly serves to validate that our tests don't accidentally break it.
Instead just say "Mark all messages as read". That's the same text used on web in the left-sidebar menus, both for an individual stream and an individual topic.
Generally this file is ordered from general to specific: realm-wide settings first, individual messages last. In between, a stream comes before a subscription, which is a particular user's relationship to a stream.
This type is used in events, too, so it goes best in the "model" library that's shared between those.
…napshot These are also used in events.
These getters are borrowed from muteModel.js in zulip-mobile.
This seems to make more sense conceptually. For its most immediate practical effect, this means UpdateMachine.fromInitialSnapshot doesn't care whether PerAccountStore.fromInitialSnapshot is a generative or a factory constructor, which will give us the flexibility to make it the latter. This change will also probably be helpful for making other decouplings of the polling and registerNotificationToken logic from the data structures of PerAccountStore.
This will let us first construct the StreamStore, as a local of the constructor, and then pass that to the Unreads constructor.
This way setupMessageListPage takes care of putting the data in place, which aligns with the structure of this file's other tests.
|
Thanks for the review! Done. |
In zulip#423 "Implement stream and topic muting", the Unreads class did start doing some filtering based in stream/topic muting, specifically in these methods: - countInStream - countInAllMessagesNarrow So, update the dartdoc so it doesn't claim that this filtering is an entirely separate concern.
In zulip#423 "Implement stream and topic muting", the Unreads class did start doing some filtering based in stream/topic muting, specifically in these methods: - countInStream - countInAllMessagesNarrow So, update the dartdoc so it doesn't claim that this filtering is an entirely separate concern.
In zulip#423 "Implement stream and topic muting", the Unreads class did start doing some filtering based in stream/topic muting, specifically in these methods: - countInStream - countInAllMessagesNarrow So, update the dartdoc so it doesn't claim that this filtering is an entirely separate concern.
Fixes: #346
Fixes: #395
This series also contains some refactoring of PerAccountStore, creating a new class StreamStore and new file
lib/model/stream.dartas a home for the section of the data that's about streams and topics. There's probably a couple of other swathes of data that could benefit from a similar treatment.Also, in order to let the Unreads instance have access to StreamStore:
store [nfc]: The event-poll loop has-a store, not is-a store
This seems to make more sense conceptually.
For its most immediate practical effect, this means
UpdateMachine.fromInitialSnapshot doesn't care whether
PerAccountStore.fromInitialSnapshot is a generative or a
factory constructor, which will give us the flexibility to
make it the latter.
This change will also probably be helpful for making other
decouplings of the polling and registerNotificationToken logic
from the data structures of PerAccountStore.
store [nfc]: Make fromInitialSnapshot a factory constructor
This will let us first construct the StreamStore, as a local of
the constructor, and then pass that to the Unreads constructor.