Skip to content
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

Add setting to only show private messages and mentions in title bar/desktop application unread count. #12677

Closed
wants to merge 2 commits into from

Conversation

@davidtwco
Copy link
Contributor

commented Jun 29, 2019

This PR adds a new setting to the user's notification settings that
will change the behaviour of the unread count in the title bar and
desktop application.

When enabled, the title bar will show the count of unread private messages
and mentions. When disabled, the title bar will act as before, showing
the total number of unread messages.

Testing Plan:
I've added a backend test. As there were no existing tests that checked the title
bar, I wasn't sure where or how best to do a frontend test, so at the time of
submitting this PR, there isn't one.

GIFs or Screenshots:
image

@zulipbot zulipbot added the size: L label Jun 29, 2019

@davidtwco davidtwco force-pushed the davidtwco:mentions-in-tray-icon branch 3 times, most recently from 76221f7 to 791b0f7 Jun 29, 2019

static/js/unread_ui.js Outdated Show resolved Hide resolved
@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

@davidtwco awesome, thanks for implementing this! I posted some small code-level comments above.

I think the main thing I want changed is the string describing it; "unread counts" might be expected to refer to the left sidebar, and the current text is ambiguous about whether it does that. @rishig do you have any ideas for how to thread that needle? I think it'll be easier to explain with a dropdown of options (also convenient for future expansion of options) rather than a checkbox. Maybe something like this???

[Dropdown] title: Unread messages counted in desktop app tray and webapp favicon (tooltip?).`
Total unread messages.
Only unread private messages and mentions.

@davidtwco davidtwco force-pushed the davidtwco:mentions-in-tray-icon branch from 791b0f7 to 984fc4d Jul 4, 2019

@zulipbot zulipbot added size: XL and removed size: L labels Jul 4, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

I've resolved the code-level comments and changed the checkbox to a dropdown as per your suggestion:

image

image

@davidtwco davidtwco force-pushed the davidtwco:mentions-in-tray-icon branch 3 times, most recently from dde53ca to e885731 Jul 4, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@timabbott @rishig is there anything I can do to move this along?

@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

@davidtwco I'm mainly waiting on feedback from @rishig on the wording stuff (also both of us were on vacation for the July 4 holiday weekend here in the USA).

@rishig

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

I think long and even medium term we'll want this to join visual, audible, mobile, and email notifications as a fifth column in the notification-preferences matrix. Besides general coherence, that would allow things like "pinned streams, topics I follow, and mentions are included".

In the short term I guess this is fine, assuming the code is structured in a way where that transition will be easy. Suggested wording:

  • It can go right below "Other notification settings"; it's not a desktop notification in the way that Zulip currently uses the term.

  • Dropdown can be

title: Organization unread count (?)
All unreads
Private messages, mentions, and alerts

Where the (?) is a link to a help article we can write.

@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

This PR is probably 5% of the work of the long-term idea you describe, and I don't expect migrating to be a problem if/when we do that column approach, so we certainly shouldn't block this on that sort of thing.

(Even just adding the logic to calculate things correctly as a 5th column would be a big project at the scale of the left sidebar unreads view project)

@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

@rishig for this:

It can go right below "Other notification settings"; it's not a desktop notification in the way that Zulip currently uses the term.

I think it still makes sense to put this under the "Desktop" heading; I interpret that block as being about "Desktop/Web specific" notification settings, not "desktop notifications" specifically. Which makes sense, because the sound setting isn't a "desktop notification" in our categorization anyway. So that's why I was thinking this belongs in the "Desktop" group.

@rishig

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

cool, sounds good. And yeah, that logic makes sense for putting it under Desktop.

@davidtwco davidtwco force-pushed the davidtwco:mentions-in-tray-icon branch from e885731 to e5eb66e Jul 9, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

I've changed the dropdown as per your wording in this comment, except i have not included "and alerts" because this PR doesn't include those - I wasn't able to find any existing infrastructure to get an unread alert count.

@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@davidtwco this looks great. I pushed an additional (totally untested) code cleanup commit to this PR; can you review and re-test with that change? Once we've confirmed I didn't break anything, we can merge.

notifications: Allow only notifiable in unread count.
This commit adds a new setting to the user's notification settings that
will change the behaviour of the unread count in the title bar and
desktop application.

When enabled, the title bar will show the count of unread private messages
and mentions. When disabled, the title bar will act as before, showing
the total number of unread messages.

Fixes #1736.
notifications: Clean up duplicated code for title unread counts.
Having a single copy of this logic will make it easier to ensure it
stays correct over time.

@davidtwco davidtwco force-pushed the davidtwco:mentions-in-tray-icon branch from 6ffa300 to b60bea0 Jul 10, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@timabbott I've rebased this (so that database migrations aren't conflicting) and fixed any issues.

@zulipbot

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Heads up @davidtwco, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Squashed the fixup commit in and merged, thanks @davidtwco!

@akashnimare FYI -- you can close the similar issues for the desktop app since this is now implemented in the webapp.

@timabbott timabbott closed this Jul 13, 2019

@timabbott

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

As a sidenote, thanks so much for implementing this @davidtwco! It's not often we get to close an issue number below 2000 :).

@davidtwco davidtwco deleted the davidtwco:mentions-in-tray-icon branch Jul 14, 2019

@akashnimare

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Awesome, @davidtwco thanks for working on this.

@ewoutkramer

This comment has been minimized.

Copy link

commented Jul 18, 2019

Maybe a little tweak would be (now you can get the count of private/mentions) to show a red dot in the tray icon when you have private msgs/mentions, this way, even if you show the count for all messages, you know someone explicitly addressed you. Thanks to Slack for showing us how useful that is ;-)

@akashnimare

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@ewoutkramer we already show this on macOS. Gotta find a way to show the same on Windows. Can you attach a screenshot of how it looks on Slack?

image

@ewoutkramer

This comment has been minimized.

Copy link

commented Jul 19, 2019

Sure, this is when I don't have mentions:
image

And it looks like this when I do have a private message/mention:
image

;-) No difference on Windows!

@akashnimare

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@ewoutkramer great and how does it look like in Slack?

@ewoutkramer

This comment has been minimized.

Copy link

commented Jul 22, 2019

This is without any mention or new messages:

image

This is when there are new messages (no count!), but no mention/private msg:

image

And finally, with a private message:

image

BTW: Slack is also an Electron app, wonder whether they experienced the same pain...

@akashnimare akashnimare referenced this pull request Aug 13, 2019
1 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.