Muted users: part 2 #4699
Muted users: part 2 #4699
Conversation
WesleyAC
force-pushed
the
dev-muted-users
branch
from
fc96f2c
to
bcaab70
Compare
src/webview/static/base.css
Outdated
| .message-muted { | ||
| .message[data-mute-state="hidden"] { |
There was a problem hiding this comment.
Then the rest of these first four commits (i.e. everything in those commits apart from adding the _ plumbing) would be best squashed together. That will simplify the changes quite a bit, because there's a lot of bits like this one that do one thing in an earlier commit and then go a different way in a later commit.
Which is a very natural and useful thing to do while drafting how something should work! Just when it's done, it's quite helpful to clean up the series so it's optimized for the reader to understand.
There was a problem hiding this comment.
Done! I sort of assumed that the whole diff would be larger than I wanted, but looking back at it it's actually pretty simple, which is nice.
There was a problem hiding this comment.
Yeah 😄
There's actually one further squashing I had in mind relative to your new revision, which is:
$ git uvr --shortstat
3dc5af6 webview: Add initial support for hiding muted messages.
4 files changed, 63 insertions(+), 2 deletions(-)
229ddf0 webview [nfc]: Pass GetText into messageAsHtml.
4 files changed, 14 insertions(+), 3 deletions(-)
a1e0775 messageAsHtml: Translate user-muted message.
2 files changed, 5 insertions(+), 5 deletions(-)
to squash those first and third changes (therefore coming after the "pass GetText in" change). The resulting change is
5 files changed, 63 insertions(+), 2 deletions(-)
so actually not any bigger than the version that doesn't translate the text, and it cleans the history up further.
Everything else looks good, so I'll just merge now and make that change on the way.
| const targetType = target.matches('.header') | ||
| ? 'header' | ||
| : target.matches('a') | ||
| ? 'link' | ||
| : 'message'; | ||
| const messageNode = target.closest('.message'); | ||
|
|
||
| if ( | ||
| targetType === 'message' | ||
| && messageNode |
There was a problem hiding this comment.
This is still a bit messy, but it's only extending a way that the existing logic in this function is already messy, and I think it's fine for now.
When I merge this, I may try splitting the conditionals in this function up to follow the style of the click handler above:
- first, see if target matches
.header, if so act on that and return. - then, see if matches
a, if so act on that and return. - then, see if muted message, if so act on that and return.
- finally, act on default case.
because I think that'll make it somewhat clearer, and also the difference in clarity will grow rapidly if we add any more cases here.
src/unread/unreadSelectors.js
Outdated
| export const getUnreadPmsTotal: Selector<number> = createSelector( | ||
| getUnreadPms, | ||
| unreadPms => unreadPms.reduce((total, pm) => total + pm.unread_message_ids.length, 0), | ||
| getMutedUsers, | ||
| (unreadPms, mutedUsers) => | ||
| unreadPms | ||
| .filter(pm => !mutedUsers.has(pm.sender_id)) | ||
| .reduce((total, pm) => total + pm.unread_message_ids.length, 0), |
There was a problem hiding this comment.
This logic isn't necessary -- the server guarantees that messages from muted users will never be unread. See API docs:
https://chat.zulip.org/api/mute-user
There was a problem hiding this comment.
Well, we've got a server bug, then :)
I think even once we fix that, it's prudent to keep this code in case of similar future problems.
There was a problem hiding this comment.
Started a discussion on #backend > muted users on CZO for the backend bug part of this.
There was a problem hiding this comment.
Interesting, and it looks like the outcome of that thread is that this is an existing mobile bug! We're not maintaining the unread state correctly.
Specifically, when we handle EVENT_NEW_MESSAGE (i.e. the Redux action we make from a message event) in each of the unread sub-reducers, we have logic like this:
if (message.sender_id === getOwnUserId(globalState)) {
return state;
}and instead it should be checking something like message.flags.includes('read'). So we should fix that.
Then I don't think it makes sense to also have logic here specific to muted users. The job of this bit of code is to count up part of the unreads data; if we have future bugs where the unreads data is wrong, we should fix the bugs by making the unreads data correct.
(If for example we did have a server bug in the future that gave us incorrect unreads data, the place I'd want to put a workaround like this is at the input end, to make the data correct in Redux. One reason is that that makes sure it's fixed consistently for all the different places we might be using that data.)
Originally discussed here: #4440 (comment) (with a bit of additional rationale) when this question first came up. It's come up a couple more times since then, as we've continued converting a lot of our components to functions with Hooks: #4555 (comment) #4699 (comment) so it's definitely time to have it written down in a proper place.
WesleyAC
force-pushed
the
dev-muted-users
branch
from
75d812e
to
571df77
Compare
|
@gnprice Thanks for the review! I made all the changes, should be good to go. |
This will allow us to translate strings in the UI around messages.
|
Looks good, thanks -- merged! I edited the series slightly from this: 3dc5af6 webview: Add initial support for hiding muted messages. to this: cd28e88 webview [nfc]: Pass GetText into messageAsHtml. by squashing two commits as mentioned at #4699 (comment) , and dropping the unreads change discussed at #4699 (comment) . One thing we'll still need before #4655 is complete is to fix the unread-state bug that we learned about in that latter thread. |
|
Thanks! Followed up with #4710 for the unreads change. |
The continuation of #4678. I moved the CaughtUp fix to #4698, since that's somewhat orthogonal (although this PR doesn't work terribly well without it).
All of the comments by @gnprice in #4678 should be resolved in this.