GNOME Bugzilla – Bug 700639
messageTray: Don't allow opening the tray while a notification is active
Last modified: 2013-05-23 16:03:26 UTC
If we press <Super>M while a notification is active, we should just fizzle the request out rather than getting into a weird state where we defer opening the tray but still try to focus it.
Created attachment 244713 [details] [review] messageTray: Don't allow opening the tray while a notification is active
Review of attachment 244713 [details] [review]: The fix is correct, but I believe it would make more sense to hide the notification immediately when you press super+M
(In reply to comment #2) > it would make more sense to hide the notification immediately > when you press super+M I think so too - a notification just pops up, it shouldn't take precedence over explicit user action.
Created attachment 244718 [details] [review] messageTray: Close any open notifications when using <Super>M If we don't do this, then we'll defer the tray opening until the notification is closed naturally, which seems unexpected to the user.
Review of attachment 244718 [details] [review]: ::: js/ui/messageTray.js @@ +2010,3 @@ }, + _toggle: function() { Why the public to private rename? @@ +2014,3 @@ return false; + this._closeNotification(); _closeNotification doesn't handle SHOWING (and changing that would have dangerous side effects probably)
(In reply to comment #5) > Review of attachment 244718 [details] [review]: > > ::: js/ui/messageTray.js > @@ +2010,3 @@ > }, > > + _toggle: function() { > > Why the public to private rename? Because consumers that might want to call .toggle() probably don't want the new "close notification" behavior. > @@ +2014,3 @@ > return false; > > + this._closeNotification(); > > _closeNotification doesn't handle SHOWING (and changing that would have > dangerous side effects probably) And I don't want those dangerous side effects in the toggle method, alone, either. When the notification changes to SHOWN, _updateState() will be called, and the notification will be hidden and the tray eventually shown.
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 244718 [details] [review] [details]: > > > > ::: js/ui/messageTray.js > > @@ +2010,3 @@ > > }, > > > > + _toggle: function() { > > > > Why the public to private rename? > > Because consumers that might want to call .toggle() probably don't want the new > "close notification" behavior. I believe consumers of .toggle() want "whatever <Super>M" does. > > @@ +2014,3 @@ > > return false; > > > > + this._closeNotification(); > > > > _closeNotification doesn't handle SHOWING (and changing that would have > > dangerous side effects probably) > > And I don't want those dangerous side effects in the toggle method, alone, > either. When the notification changes to SHOWN, _updateState() will be called, > and the notification will be hidden and the tray eventually shown. No, if you don't handle SHOWING, then you're not fixing this bug: _updateState() will not go through _toggle() when the notification is shown, and it gives priority to notifications showing/shown vs _traySummoned.
(In reply to comment #7) > I believe consumers of .toggle() want "whatever <Super>M" does. That's toggleAndNavigate() though. Perhaps we should just merge toggle() into toggleAndNavigate().
Created attachment 244837 [details] [review] messageTray: Close any open notifications when using <Super>M If we don't do this, then we'll defer the tray opening until the notification is closed naturally, which seems unexpected to the user. This is the best thing I could come up with. As a notification cannot receive focus unless it is SHOWN, it cannot have a key release, nor should the close button be visible. I tested for a few minutes to see if I could trigger either "by accident", and couldn't find any, thus modifying _closeNotification should be 100% safe. The problem is that _closeNotification sets _notificationClosed back to false right afterwards. So what's going to happen is that _updateState will note that the notification is SHOWING, and not do anything, thus it's like the notification was never closed. As a fix, we'll make the _notificationClosed flag persistent until _hideNotification is called as a result.
Review of attachment 244837 [details] [review]: Still doesn't work: it closes the current notification, but then _updateState() goes on with the next one in the queue.
Created attachment 244849 [details] [review] messageTray: Don't hide the tray for urgent notifications The tray should never close on its own, without the user's input.
Created attachment 244850 [details] [review] messageTray: Remove some unused variables in _updateState
Created attachment 244851 [details] [review] messageTray: Add a synonym for hasNotifications This prevents lines from getting too long.
Created attachment 244852 [details] [review] messageTray: Hide notifications when the tray is summoned If the user summons the tray, it should take priority over a notification showing.
Review of attachment 244849 [details] [review]: Ok
Review of attachment 244850 [details] [review]: Squash this.
Review of attachment 244851 [details] [review]: Ok
Review of attachment 244852 [details] [review]: Just a style fix. ::: js/ui/messageTray.js @@ +2220,3 @@ let notificationLockedOut = !hasNotifications && this._notification; + let notificationMustClose = this._notificationRemoved || notificationLockedOut || (notificationExpired && this._userActiveWhileNotificationShown) || this._notificationClosed || this._traySummoned; + let canShowNotification = notificationsPending && this._trayState == State.HIDDEN && !this._traySummoned; Please wrap these.
Attachment 244849 [details] pushed as d1c54f5 - messageTray: Don't hide the tray for urgent notifications Attachment 244851 [details] pushed as d96726c - messageTray: Add a synonym for hasNotifications Attachment 244852 [details] pushed as d5f95db - messageTray: Hide notifications when the tray is summoned
*** Bug 698883 has been marked as a duplicate of this bug. ***