Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

notification issue fixed #240

Merged
merged 9 commits into from
Mar 29, 2016
Merged

notification issue fixed #240

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
98343a0
notification issue fixed
mebjas Mar 21, 2016
1875377
Some style fixes
mebjas Mar 22, 2016
818299d
persistent = true, as default param in setToolbarHidden() method
mebjas Mar 23, 2016
cbcf23f
funciton name changed to
mebjas Mar 23, 2016
349b472
logic to show/hide toolbar shifted to MatrixChat::render
mebjas Mar 23, 2016
9b5519e
promise -> callback & setToolbarPersistantHidden moved inline
mebjas Mar 23, 2016
8191eaa
unwanted functions removed, prev functions modified
mebjas Mar 23, 2016
8605ca1
comment removed and localStorage update moved up
mebjas Mar 23, 2016
6fc0aae
setToolbarHidden(false) if setEnabled has enable = true
mebjas Mar 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 55 additions & 18 deletions src/Notifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,28 @@ var Notifier = {
return global.Notification.permission == 'granted';
},

isPermissionDefault: function() {
if (!this.supportsDesktopNotifications()) return false;
return global.Notification.permission == 'default';
},

// Function to be used by clients to check weather or not to
// show the toolbar.
shouldShowToolbar: function() {
// Check localStorage for any such meta data
if (global.localStorage) {
if (global.localStorage.getItem('notifications_hidden') === 'true')
return false;
}

// Check if permission is granted by any chance.
if (this.havePermission()) return false;

// means the permission is blocked
if (!this.isPermissionDefault()) return false;
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richvdh Tell me what do you think about showing a message if the user has blocked notif request & not yet attempted to close the toolbar - if yes, I'll change this method to return enums - 'show', 'hide', 'blocked' and the vector will take decision accordingly.

},

setEnabled: function(enable, callback) {
// make sure that we persist the current setting audio_enabled setting
// before changing anything
Expand All @@ -133,38 +155,42 @@ var Notifier = {
}

if(enable) {
if (!this.havePermission()) {
global.Notification.requestPermission(function() {
if (callback) {
callback();
// Case when we do not have the permission as 'granted'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is no longer helpful

if (this.isPermissionDefault()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any need for this test. global.Notification.requestPermission() will do nothing (except give result==granted) if we already have permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well yeah normally it would never lead to this, as if global.Notification.permission != default the toolbar wouldn't even be shown.
But still in SDK shouldn't we have the case where we hide the toolbar in case the global.Notification.permission is already granted or denied

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your answer.

setEnabled is not just used from the toolbar; it is also used from the settings dialog.

But the point is, you do not need to test for isPermissionDefault here - just go straight into requestPermission, which will give the same result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes more sense :)

// Attempt to get permission from user
var self = this;
global.Notification.requestPermission().then(function(result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can rely on our users having browsers which support the promise-returning version of requestPermission. https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission suggests that Chrome doesn't support it at all. You'll need to stick with the callback version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:O it supported on chrome, but yeah we need to have backward compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I shall shift this to callback one? I think that MDN doc is little updated, as this is supported in chrome now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you might as well use the callback flow.

Obviously the MDN doc is outdated, but we don't know which version of Chrome introduced the promise; we don't know if it works in Safari; and using the promise doesn't buy us anything.

if (result === 'denied') {
dis.dispatch({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The denied case doesn't change anything, so I don't think we need to dispatch the notifier_enabled case here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still don't think it is necessary to raise an event here

action: "notifier_enabled",
value: true
value: false
});
self.setToolbarHidden(true, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still don't think it is correct to hide the toolbar here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should atleast change the message or remove the link?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm running out of ways to say this: Leave it alone for now, and make it a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, no actions if denied!

return;
}
if (result === 'default') {
// The permission request was dismissed
return;
}
});
}

if (!global.localStorage) return;
global.localStorage.setItem('notifications_enabled', 'true');
if (callback) callback();
dis.dispatch({
action: "notifier_enabled",
value: true
});

if (this.havePermission) {
dis.dispatch({
action: "notifier_enabled",
value: true
if (!global.localStorage) return;
global.localStorage.setItem('notifications_enabled', 'true');
});
}
}
else {
} else {
if (!global.localStorage) return;
global.localStorage.setItem('notifications_enabled', 'false');
dis.dispatch({
action: "notifier_enabled",
value: false
});
}

this.setToolbarHidden(false);
},

isEnabled: function() {
Expand Down Expand Up @@ -192,12 +218,23 @@ var Notifier = {
return enabled === 'true';
},

setToolbarHidden: function(hidden) {
setToolbarHidden: function(hidden, persistent = true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just fyi: you may know this already but default parameters are a feature of ES6, which means that we can't use them in some browsers. But it's ok to use in react-sdk because babel translates it for us.

this.toolbarHidden = hidden;
dis.dispatch({
action: "notifier_enabled",
value: this.isEnabled()
});

if (persistent) {
this.setToolbarPersistantHidden();
}
},

setToolbarPersistantHidden: function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Persistant/Persistent/, but to be honest you might as well inline this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w/o a function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. it's only two lines, and only used in one place. Having a separate function doesn't do much for code clarity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still needs inlining

// update the info to localStorage
if (global.localStorage) {
global.localStorage.setItem('notifications_hidden', 'true');
}
},

isToolbarHidden: function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isToolbarHidden needs to check the localStorage setting

Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/MatrixChat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ module.exports = React.createClass({
if (MatrixClientPeg.get().isGuest()) {
topBar = <GuestWarningBar />;
}
else if (Notifier.supportsDesktopNotifications() && !Notifier.isEnabled() && !Notifier.isToolbarHidden()) {
else if (Notifier.supportsDesktopNotifications() && Notifier.shouldShowToolbar()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change is necessary. It's clearer to keep isEnabled and isToolbarHidden separate imho

topBar = <MatrixToolbar />;
}
else if (this.state.hasNewVersion) {
Expand Down