Remove "badge" setting and rely on browser choice for desktop notifcations #28

lpoujol · 2016-02-13T22:35:44Z

Remove the badge setting and rely on browser choice for desktop notifications.

But the user still have to activate the desktop notifications via the lounge settings page. Making lounge ask the permission to the browser.

The status on whether or not it's activated is also displayed on the settings page.

Some screens to show how it looks :

"Deactivated" state :

Activated :

Blocked by web browser

astorije · 2016-02-16T01:32:35Z

Can @xPaw, @JocelynDelalande, @YaManicKill or @wizardfrag self-assign this one please? :-)

AlMcKinlay · 2016-02-19T08:20:30Z

I shall take a look at this tonight or tomorrow.

1 question before I actually download it and try it:

If notifications are disabled, and then the user wants notifications again, how do they get it? I don't think the app can ask for notifications again, can it? And if it can't, what happens when the user tells the browser not to disable notifications again?

I'll have a play around and see how it works.

AlMcKinlay · 2016-02-19T08:21:05Z

Oh, also, I'm not sure about there not being a setting to disable notifications after they have been enabled. That seems a little awkward for users.

lpoujol · 2016-02-19T09:16:26Z

The app can't ask again if the browser blocks notifications. If you re-enable them, notifications will pop again without the need to touch shout settings. But, if you set it as "always ask", shout will have to ask again.

There's is no settings because in the end it's the browser who have the last word on wether notifications are displayed or not. Having a checkbox on the settings page for notifications doesn't really mean anything as long as browser settings are set differently.

AlMcKinlay · 2016-02-19T09:24:42Z

The app can't ask again if the browser blocks notifications. If you re-enable them, notifications will pop again without the need to touch shout settings. But, if you set it as "always ask", shout will have to ask again.

Alright, good. I'll double check that works with different browsers as well, but I believe you.

There's is no settings because in the end it's the browser who have the last word on wether notifications are displayed or not. Having a checkbox on the settings page for notifications doesn't really mean anything as long as browser settings are set differently.

Well, that's not really true. If we have a "disable notifications" we (in the code) will not send a notification, even when they are enabled in the browser.

I don't think it's right for the user to have to change browser settings to stop us notifying them again.

AlMcKinlay · 2016-02-19T20:47:21Z

client/index.html

@@ -243,8 +243,8 @@ <h1 class="title">Settings</h1>
 							</div>
 							<div class="col-sm-12">
 								<label class="opt">
-								<input id="badge" type="checkbox" name="badge">
-								Enable badge
+									Desktop notification status : <span id="desktopNotificationsStatus">Unknown</span>


There's extra indentation here.

AlMcKinlay · 2016-02-19T20:51:12Z

Right, so in general I quite like it. I'm just not a fan of the user having to go to the browser settings to disable notifications.

I'm not sure I want another setting for that though.

I'll give a 👍 and see what others think. I'm happy for this to go in.

xPaw · 2016-02-19T22:42:26Z

I don't like that you completely removed disabling notifications in settings. When we will add per-channel settings, we will need to have an ability to disable notifications.

Having a badge to say that notifications are not enabled by the browser is a good idea though.

astorije · 2016-02-21T00:03:44Z

So, after reading the initial proposition and @YaManicKill's and @xPaw's concerns, here is my proposal. First of all, some definitions to be sure we are all on the same page:

On the browser side of things, notification states are:

Not enabled: The user was never asked for this website, or refused to accept or answer by clicking the × button. This state is only possible when the user set their browser to always ask for approval (default).
Allowed: User has allowed this website to display notifications (either he allowed for all or already allowed for this website before).
Blocked: User has disallowed this website to display notifications (either he disallowed for all or already disallowed for this website before).

On The Lounge, you should be able to enable or disable notifications, as long as they are allowed (state (2)). As @YaManicKill and @xPaw have mentioned, first because in the future we will want to be able to override this setting per channel, but also because disabling notifications within the browser is quite a reach in the browser settings. The opposite is also true: when the user has blocked the notifications, it's a quite a dig to re-enable... but that's their choice and there is nothing we can do about it :-)

My proposal is to keep the current checkbox but to enhance it:

An empty checkbox (along with the text Enable notifications) means The Lounge will not trigger notifications at all, whatever states (1) through (3) the browser is in.
A disabled checkbox means that we are in state (3).
A checked checkbox means we are in state (2) and The Lounge triggers notifications.
When the user checks the checkbox, either:
- Browser is in state (1), so the confirmation popup above will appear again:
  - If the user clicks on Allow, the checkbox gets checked and notifications will show up in the future (third bullet point of this list).
  - If the user clicks on Block, the checkbox goes back to unchecked and gets disabled (second bullet point of this list).
  - If the user closes without answering, the checkbox goes back to unchecked (first bullet point of this list).
- If browser is in state (2), the checkbox simply gets checked and notifications will show up (third bullet point of this list).
When the user unchecks the checkbox, the notifications stop getting triggered within The Lounge, but permission at the browser-level is left unchanged.

This allows for what was discussed earlier:

Notifications can be disabled within the app even after they were enabled and allowed.
Notifications can be enabled per channel (future) independently of the browser permission.
The user knows notifications are blocked with the disabled checkbox. This can be enough feedback for this PR, or a mention after the checkbox text (Blocked by the browser).

When I look at how things are done at the moment (before your PR), I see two crucial differences:

When notifications are blocked by the user, we can still check or uncheck the notifications. This is not good because it's misleading: there is no way a notification can get through. Disabling the checkbox would help with that, as well as adding the mention.
When the checkbox is checked for the first time, the user gets asked. If he doesn't answer (closes the permission popup), the checkbox stays checked although notifications cannot go through. Unchecking the checkbox when this happens would help.

Sorry, this was a very long comment for something very simple, but I wanted to give all details. How does everyone feel about that?

astorije · 2016-02-26T06:50:08Z

@lpoujol, ping?

maxpoulin64 · 2016-02-28T00:37:30Z

I agree with the others on this one. Notification settings are not convenient to find in browsers, especially when The Lounge is running as a web application where there is no browser (especially mobile where pinning the app to the home screen opens it fullscreen):

We need to be able to toggle them on/off.

Additionally, another problem for the moment is that notifications needs to be disabled on mobile Chrome because Notification was never implemented. It exists, it will grant the permission, but invoking new Notification() throws an error which makes the app crash and reload.

astorije · 2016-02-28T01:10:17Z

Additionally, another problem for the moment is that notifications needs to be disabled on mobile Chrome because Notification was never implemented. It exists, it will grant the permission, but invoking new Notification() throws an error which makes the app crash and reload.

Didn't know that.
@maxpoulin64, this PR has been open for a while, would you be OK with my suggestion in terms of usage and see to fix this afterwards?

maxpoulin64 · 2016-02-28T02:53:47Z

@astorije Yes, I fully agree with you. Your solution above has a state where we don't try to send notifications, this is the only thing that's needed. I just didn't want to rely on the browser to block our notifications.

AlMcKinlay · 2016-02-29T09:25:59Z

I agree with @astorije and @maxpoulin64. @lpoujol would you be able to make these changes and rebase the PR? It would be good to get these changes in :-)

astorije · 2016-03-02T04:56:44Z

I am removing the second review needed label until this PR goes further. Can't wait to hear from you, @lpoujol :-)

lpoujol · 2016-03-03T17:20:05Z

I brought back the setting (also renaming it from badge to desktopNotifications in the code)

I check the browser state on display of the settings page.
It will ask permission for desktop notifications if the setting is activated but not granted by the browser.
It will turn off the setting if it's blocked by the browser, disable the checkbox and display a warning below.

But there is an edge case that I can't really cover. It's when the browser choice for desktop notifications (Block, Ask, Allow) change while the settings page is displayed :/. There will be a Permission API on browsers, but it's still experimental :/
A work around would be to not disable the checkbox. Checking the box would trigger a new check.

astorije · 2016-03-04T05:34:30Z

client/index.html

-								<input id="badge" type="checkbox" name="badge">
-								Enable badge
+								<input id="desktopNotifications" type="checkbox" name="desktopNotifications">
+								Enable desktop notifications<br />


<br> instead of <br />

astorije · 2016-03-04T05:57:57Z

@lpoujol, looking good! :-)

I noticed an issue though, with the permission pop-up which never stops asking once clicked on the checkbox once, even after changing tab:

Also, when decision to allow or block is not taken, the checkbox should never be checked, which can be the case at the moment.

But there is an edge case that I can't really cover. It's when the browser choice for desktop notifications (Block, Ask, Allow) change while the settings page is displayed :/.

I am not too bothered by this edge case. We could also check if denied when the settings are open and the app is being focused (ie. clicking on the browser tab), but not a big deal to me. The situation above is a bit more worrying IMO.
Another solution could be a "Check now" link/button as part of the warning to trigger a new check. None of these are ideal which is why I'd leave it like what you have now.

A work around would be to not disable the checkbox. Checking the box would trigger a new check.

Meh, it would be weird to be able to be able to check a checkbox that automatically goes back to unchecked because blocked by the browser I think.

lpoujol · 2016-03-05T19:03:06Z

Last commit fixes the issue with the permission popup (Didn't had on Firefox because it won't execute the callback if no decision is taken) .
(I'll squash all commits if everyone is happy with that ;) )

astorije · 2016-03-05T20:22:07Z

@lpoujol, works great, awesome, I really like it!!

👍

astorije · 2016-03-05T20:24:37Z

Taking ownership of this one, to let @YaManicKill breathe a bit :-)

astorije · 2016-03-06T05:35:26Z

By the way, yes, please do squash @lpoujol :-)

maxpoulin64 · 2016-03-06T05:41:12Z

All good to me, 👍, ok to merge after squash.

lpoujol · 2016-03-06T09:59:21Z

Squashed !

xPaw · 2016-03-06T10:00:45Z

client/index.html

-								Enable badge
+								<input id="desktopNotifications" type="checkbox" name="desktopNotifications">
+								Enable desktop notifications<br>
+								<div class="error" id="warnDisabledDesktopNotifications">Warning : Desktop notifications are blocked by your web browser</div>


Nit: can you remove space after warning here, maybe even bold the warining?

👍 on the space, that's a difference with French :P
For the warning, I don't think so at the moment, because none of the similar-looking error do that. I'd keep this for a later time.

Also checks the browser status and display a warning message if it blocks desktop notifications

astorije · 2016-03-06T19:45:50Z

👍 and merging, thanks all for the multiple reviews and thanks a lot @lpoujol for this and for sticking with us for 22 days!! :-)

Remove "badge" setting and rely on browser choice for desktop notifcations

astorije mentioned this pull request Feb 16, 2016

update default settings to be more usable than technical #49

Closed

AlMcKinlay self-assigned this Feb 19, 2016

AlMcKinlay reviewed Feb 19, 2016
View reviewed changes

AlMcKinlay added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed labels Feb 19, 2016

astorije mentioned this pull request Feb 22, 2016

[Discussion] Towards better settings (and per-channel settings) #85

Open

astorije assigned AlMcKinlay and unassigned AlMcKinlay Feb 25, 2016

astorije removed the second review needed label Mar 2, 2016

astorije reviewed Mar 4, 2016
View reviewed changes

astorije added the second review needed label Mar 5, 2016

astorije assigned astorije and unassigned AlMcKinlay Mar 5, 2016

astorije removed the second review needed label Mar 6, 2016

xPaw reviewed Mar 6, 2016
View reviewed changes

Rename badge setting to desktopNotifications

bf9e500

Also checks the browser status and display a warning message if it blocks desktop notifications

astorije added a commit that referenced this pull request Mar 6, 2016

Merge pull request #28 from lpoujol/desktopNotificationSetting

c4d628d

Remove "badge" setting and rely on browser choice for desktop notifcations

astorije merged commit c4d628d into thelounge:master Mar 6, 2016

astorije added this to the 1.4.0 milestone Oct 8, 2016

brunnre8 pushed a commit to brunnre8/thelounge that referenced this pull request Apr 6, 2021

Fix thelounge#28

58b8d91

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "badge" setting and rely on browser choice for desktop notifcations #28

Remove "badge" setting and rely on browser choice for desktop notifcations #28

lpoujol commented Feb 13, 2016

astorije commented Feb 16, 2016

AlMcKinlay commented Feb 19, 2016

AlMcKinlay commented Feb 19, 2016

lpoujol commented Feb 19, 2016

AlMcKinlay commented Feb 19, 2016

AlMcKinlay Feb 19, 2016

AlMcKinlay commented Feb 19, 2016

xPaw commented Feb 19, 2016

astorije commented Feb 21, 2016

astorije commented Feb 26, 2016

maxpoulin64 commented Feb 28, 2016

astorije commented Feb 28, 2016

maxpoulin64 commented Feb 28, 2016

AlMcKinlay commented Feb 29, 2016

astorije commented Mar 2, 2016

lpoujol commented Mar 3, 2016

astorije Mar 4, 2016

lpoujol Mar 5, 2016

astorije commented Mar 4, 2016

lpoujol commented Mar 5, 2016

astorije commented Mar 5, 2016

astorije commented Mar 5, 2016

astorije commented Mar 6, 2016

maxpoulin64 commented Mar 6, 2016

lpoujol commented Mar 6, 2016

xPaw Mar 6, 2016

astorije Mar 6, 2016

astorije commented Mar 6, 2016

Remove "badge" setting and rely on browser choice for desktop notifcations #28

Remove "badge" setting and rely on browser choice for desktop notifcations #28

Conversation

lpoujol commented Feb 13, 2016

astorije commented Feb 16, 2016

AlMcKinlay commented Feb 19, 2016

AlMcKinlay commented Feb 19, 2016

lpoujol commented Feb 19, 2016

AlMcKinlay commented Feb 19, 2016

AlMcKinlay Feb 19, 2016

Choose a reason for hiding this comment

AlMcKinlay commented Feb 19, 2016

xPaw commented Feb 19, 2016

astorije commented Feb 21, 2016

astorije commented Feb 26, 2016

maxpoulin64 commented Feb 28, 2016

astorije commented Feb 28, 2016

maxpoulin64 commented Feb 28, 2016

AlMcKinlay commented Feb 29, 2016

astorije commented Mar 2, 2016

lpoujol commented Mar 3, 2016

astorije Mar 4, 2016

Choose a reason for hiding this comment

lpoujol Mar 5, 2016

Choose a reason for hiding this comment

astorije commented Mar 4, 2016

lpoujol commented Mar 5, 2016

astorije commented Mar 5, 2016

astorije commented Mar 5, 2016

astorije commented Mar 6, 2016

maxpoulin64 commented Mar 6, 2016

lpoujol commented Mar 6, 2016

xPaw Mar 6, 2016

Choose a reason for hiding this comment

astorije Mar 6, 2016

Choose a reason for hiding this comment

astorije commented Mar 6, 2016