Support unmuting topics in muted streams #5727
Conversation
gnprice
added
P1 high-priority
api migrations
server release goal
src/topics/topicSelectors.js
Outdated
| // If we're looking at a stream the user isn't subscribed to, then | ||
| // they won't see unreads from it even if they somehow have | ||
| // individual topics set to unmuted. So effectively it's all muted. | ||
| const streamMuted = !subscription?.in_home_view ?? true; |
There was a problem hiding this comment.
I think the right side of the ?? is unreachable: it wants the left side to be undefined or null, but since the left is the result of a !, it'll in fact be a boolean.
Did you mean to write this as !!subscription && !subscription.in_home_view, like you wrote for a similar value in the next commit?
There was a problem hiding this comment.
(…and, coming back from reading later commits, now I see this point stops applying in the last commit.)
There was a problem hiding this comment.
Good catch, thanks. I'll fix that (though as you say, it ends up getting replaced in a later commit anyway.)
| // Not in the API docs yet, but the API has been agreed on: | ||
| // https://chat.zulip.org/#narrow/stream/378-api-design/topic/muted.20topics/near/1501574 | ||
| // https://chat.zulip.org/#narrow/stream/378-api-design/topic/muted.20topics/near/1528885 | ||
| // TODO(server): delete this comment once documented | ||
| Unmuted = 2, |
There was a problem hiding this comment.
I do see this in the API doc: https://zulip.com/api/update-user-topic
There was a problem hiding this comment.
Hmm true. It doesn't appear in the other places this enum is used, though:
https://zulip.com/api/register-queue
https://zulip.com/api/get-events#user_topic
I guess I should raise this in #api documentation.
There was a problem hiding this comment.
https://chat.zulip.org/#narrow/stream/378-api-design/topic/muted.20topics/near/1557615
(Mentioned in #api design instead because there was a previous message there to follow up on.)
| @@ -27,15 +28,22 @@ export const getTopicsForNarrow: Selector<$ReadOnlyArray<string>, Narrow> = crea | |||
| export const getTopicsForStream: Selector<?$ReadOnlyArray<TopicExtended>, number> = createSelector( | |||
There was a problem hiding this comment.
This selector getTopicsForStream gives the data for TopicListScreen. Is that the screen meant by this part of #5362's description, under "ideally":
In muted streams, sorting unmuted topics at the top in the stream topic view.
?
It looks like the web app does that kind of sorting in the left sidebar:
unless I click "more topics":
There was a problem hiding this comment.
Hmm, yeah, I'd missed that line in #5362.
It's a bit awkward here, because we don't have the "more topics" distinction — really this screen is always in "more topics" mode.
In particular, in this screen (like in "more topics" in web) we show all the topics that have messages, even when already read. So if we just sorted the unmuted topics to the top, then you'd quickly end up having the top clogged with a bunch of old topics that you unmuted back when they were live; effectively it'd degrade to only showing you the unmuted topics.
Short of reworking this screen more deeply, I think the right answer here is:
- When you want just the unmuted topics that have unreads (the same list that web puts at the top in the left sidebar), you can find that in the inbox view.
- When you want all the topics (just like "more topics" mode on web), that's this screen.
@alya, thoughts on those UI choices?
This avoids clobbering the state of topics that have newer types of policies.
Just like we do on web. This will also set us up to show the distinction when some topics are *un*muted soon.
The concept of muting or unmuting a stream can only exist when the user is subscribed in the first place; without a subscription, there isn't even a place in the data model for the muted/unmuted state to live. For a topic, the data model doesn't rule it out but it wouldn't mean anything: you don't get unreads or notifications when you're not subscribed.
These are a bit hard to follow already, and would get more so with the introduction of unmuted topics in muted streams. Also rename a local variable to be hopefully a bit clearer about what it does.
For an upcoming change to this logic, we'll want the call site to have its hands on the Subscription object.
With the upcoming feature of unmuted topics in muted streams, the question "is this topic muted?" will split into several different questions that can have different answers. To prepare for that, stop having a function asking that less-specific question. For several call sites, we can cleanly write a name and jsdoc for a function that covers all of them. In the topic action sheet, the question we'll end up wanting to ask is different from those, and reflects a UI choice we're making specifically about the action sheet. So just put an explicit switch statement there.
gnprice
force-pushed
the
pr-unmute-topic
branch
from
b4a41be to
d02040d
Compare
|
Merging, with that If we want to make further changes to the topic-list screen (per #5727 (comment) above), that can be a followup; I think this version is definitely enough for us to feel good about the feature being fully implemented when we're advertising it in the release notes. |
Fixes: #5362
Fixes: #5691
Fixes: #5726