GNOME Bugzilla – Bug 617209
It is frustrating when the notification expands because the mouse was sitting idely where it appears
Last modified: 2010-09-10 19:55:00 UTC
I rarely use my mouse so it often sits idely somewhere on my screen. And more than once already, it was sitting just where the notification appears (probably because I had moved it on a previous notification to expand it). If a notification appears and the mouse is waiting over it, then the notification expands, without any action from my part (and thus without me showing any interest for it). That makes the notification very disruptive, at least much more than it should be, as if notifications were expanded by default. One solution could be to only expand the notification if the mouse is over it AND the mouse is moving towards it. But as Marina said on IRC: <marina> bochecha: definitely happens now; though I'm not sure an alternate behavior of having the user to explicitly move outside the notification and move back to it would be any better IMHO, if you have your hand on your mouse, you're probably already moving it and thus this solution would not change much. On the other hand, if you don't have your hand on your mouse, it is a much bigger disruption to have to move your hand, take your mouse and move it so that the notification finally disappears and you can go back to your work.
There's probably a third (or even a fourth?) possibility that would be better than the current one or what I suggested, so I'm adding the ui-review keyword as requested by Marina on IRC.
Created attachment 164669 [details] [review] Stop notifications from expanding when pointer is left in tray Simply stores the position of the pointer when the notification is first displayed and then compares when the 'should I expand the notification' logic runs. If it hasn't moved, don't expand the notification, the idea being that if you aren't moving the mouse, you're busy with something and don't need the notification to expand. I don't think there's any way to decide whether someone wants the notification to expand based on mouse movement.
(In reply to comment #2) > Created an attachment (id=164669) [details] [review] > Stop notifications from expanding when pointer is left in tray > > Simply stores the position of the pointer when the notification is first > displayed and then compares when the 'should I expand the notification' logic > runs. If it hasn't moved, don't expand the notification, the idea being that > if you aren't moving the mouse, you're busy with something and don't need the > notification to expand. I don't think there's any way to decide whether > someone wants the notification to expand based on mouse movement. This does not really make much sense ... "mouse has moved" isn't really a good heuristic as you might move the mouse by a small amount without the intention to do anything, or you might move the mouse and still being "busy with something".
I described the behavior we thought of with Jon in the "design update" bug: https://bugzilla.gnome.org/show_bug.cgi?id=617224#c17 4) If a notification popped up under the pointer, only expand it if the user moves out of it and then moves back in. Use a longer timeout before popping it down in that case. (bug https://bugzilla.gnome.org/show_bug.cgi?id=617209 ) I was going to check out the patch here and write a new patch that works in the above way. If someone else wants to work on this, it's best to do it on top of the patches in the "design update" bug. There are such caveats as making sure that the urgent notifications that get expanded by default, but don't grab focus initially, do so when the notification is re-entered.
(In reply to comment #4) > I described the behavior we thought of with Jon in the "design update" bug: > > https://bugzilla.gnome.org/show_bug.cgi?id=617224#c17 > > 4) If a notification popped up under the pointer, only expand it if the user > moves out of it and then moves back in. Yeah that makes more sense than simply comparing whether the pointer moved at all.
Review of attachment 164669 [details] [review]: Marking as needs-work based on above comments.
Created attachment 168151 [details] [review] Don't expand notifications that pop up over the pointer Expanding notifications automatically just because they popped up where the pointer is positioned distracts the user. It can also lead to a behavior that is surprising to the user by, for example, making the user's input switch to the notification's text entry box. It is possible to expand a notification that popped up over the pointer by mousing away from it and then mousing back in immediately.
Comment on attachment 168151 [details] [review] Don't expand notifications that pop up over the pointer >+ if (this._firstSeenMouseX > 0) { I don't like "firstSeenMouseX/Y" as names. It's not clear what timeframe "first" refers to. (It's clearly not the first mouse position *ever*, etc.) But anyway, I'm not sure we even need it anyway; all we want to know is that the hover state changed because the notification was moving rather than because the mouse was moving. So really we could just test "if (this._notificationState == State.SHOWING)" in _onTrayHoverChanged. (Theoretically this is not entirely right, since the tray and mouse could *both* be moving. But it would require superhuman reflexes on the part of the user to demonstrate the bug. :-) >+ // automatically. Instead, the user is able to expand the notification by mousing away from it and then >+ // mousing back in. Because this is an expected action, we set the boolean flag that indicates that a longer >+ // timeout should be used before popping down the notification. Do we need a longer timeout? In the "normal" case, the pointer might be all the way across the screen, and we're assuming the user will have enough time to move the mouse over to the notification then. It seems like doing a quick out-in bounce ought to take less time than that. If we *are* keeping the longer timeout, then it would be nice to integrate it with the existing "longer timeout" code (in _notificationTimeout, where it automatically extends the timeout if the mouse has moved towards the notification since the last timeout) rather than having two different systems for longer timeouts.
Oh, wait, my bad, this is the hide-tray-on-leave timeout, not the hide-tray-on-idle timeout. Um... if "useLongerHideOnExitTimeout" doesn't make things look horrible, then maybe that would help?
(In reply to comment #8) > (From update of attachment 168151 [details] [review]) > >+ if (this._firstSeenMouseX > 0) { > > I don't like "firstSeenMouseX/Y" as names. It's not clear what timeframe > "first" refers to. (It's clearly not the first mouse position *ever*, etc.) > > But anyway, I'm not sure we even need it anyway; all we want to know is that > the hover state changed because the notification was moving rather than because > the mouse was moving. So really we could just test "if (this._notificationState > == State.SHOWING)" in _onTrayHoverChanged. (Theoretically this is not entirely > right, since the tray and mouse could *both* be moving. But it would require > superhuman reflexes on the part of the user to demonstrate the bug. :-) This is actually what I originally did, but it doesn't work because _onTrayHoverChanged() doesn't get called when the notification appears under the mouse, but only gets called when the user starts moving the mouse again. (This means the notifications don't expand now if you leave the mouse in the tray area while they show up, but only expand when you start moving the mouse.) This is why we need to know if the notification originally appeared under the mouse when _onTrayHoverChanged() is called. We could do this calculation as soon as the notification is done showing up, but then we'd be making it for all notifications, not just the ones the user actually moused to or happened to have the mouse over. So it seems to make sense to keep this calculation in _onTrayHoverChanged() . We could call the variables this._showNotificationMouseX and this._showNotificationMouseY and add a comment explaining why we need these. Does that sound good? (In reply to comment #9) > Oh, wait, my bad, this is the hide-tray-on-leave timeout, not the > hide-tray-on-idle timeout. Um... if "useLongerHideOnExitTimeout" doesn't make > things look horrible, then maybe that would help? How about useLongerTrayLeftTimeout, since the function is called _onTrayLeftTimeout() and the id variable is called this._trayLeftTimeoutId ?
Created attachment 169193 [details] [review] Don't expand notifications that pop up over the pointer Expanding notifications automatically just because they popped up where the pointer is positioned distracts the user. It can also lead to a behavior that is surprising to the user by, for example, making the user's input switch to the notification's text entry box. It is possible to expand a notification that popped up over the pointer by mousing away from it and then mousing back in immediately. Updated with the changes explained in the previous comment. Added more comments. Added a check for the case when _onTrayHoverChanged() gets called multiple times while this.actor.hover is true.
Created attachment 169194 [details] [review] Make sure we auto-expand urgent notifications with long banners We only add the banner body if the banner text doesn't fully fit in the banner mode after the notification has been allocated. Therefore we should wait until the notification's content is finalized to call expandNotification() for urgent notifications.
Created attachment 169195 [details] [review] Make sure we auto-expand urgent notifications with long banners We only add the banner body if the banner text doesn't fully fit in the banner mode after the notification has been allocated. Therefore we should wait until the notification's content is finalized to call expandNotification() for urgent notifications.
Created attachment 169197 [details] [review] Unset the initial notification timeout if the user mouses away from the message tray We should always remove the notification if the user has interacted with it and then moved the mouse away from it. This applies even if the initial timeout for which we show the notification by default has not passed yet.
Comment on attachment 169195 [details] [review] Make sure we auto-expand urgent notifications with long banners >+ // We don't show the banner if none of it fits or if >+ // the notification is urgent and the banner doesn't fully >+ // fit. We expand urgent notifications by default, and we >+ // don't want the incomplete banner text to be flickering. >+ if (bannerX1 > availWidth || (this.urgent && !bannerFits)) { This shouldn't be triggered by the "urgent" flag. We might someday change the rules about what notifications are expanded by default, and we don't want to have to change those rules in two different places.
Created attachment 169395 [details] [review] Make sure we auto-expand urgent notifications with long banners We only add the banner body if the banner text doesn't fully fit in the banner mode after the notification has been allocated. Therefore we should wait until the notification's content is finalized to call expandNotification() for urgent notifications.
Comment on attachment 169395 [details] [review] Make sure we auto-expand urgent notifications with long banners The content-finalized signal does not appear to be necessary; expandNotification() sets up a signal handler to re-call itself if the height of the notification changes, so the notification does get fully expanded after the bannerbody is added anyway. Was urgent-notification-expansion previously broken? It doesn't seem to work in master. At any rate, this is probably not the fault of these changes, but the combination of sliding and fading in at the same time does not work well IMHO; the fade-in ends up obscuring the slide-in, so it looks like the notification just appears there. I'm not really sure why we need to have the notifications both sliding and fading anyway? >+ _expandNotification: function(autoExpanding) { >+ let poppedOut = this._notification.popOut(!autoExpanding); >+ if (this._notification && (autoExpanding || poppedOut)) { the second line checks if this._notification is non-null, but the first line has already dereferenced it. (I'm not sure it's actually possible for this._notification to be null at that point, so you might want to just remove the check.)
Created attachment 169461 [details] [review] Fix various details of how notifications are shown This patch ensures the following notifications behavior: - Urgent notifications that have long title or banner text are auto-expanded correctly. - Single-line notifications that have _expandNotification() called (e.g. because the user mouses over to them), are treated as expanded, which means they get fully expanded if they are updated with more content and the user can escape them. - The position of expanded notifications is updated when they are updated. - Notification banner is shown again on the first line if it can fully fit there after a notification is updated, even if it was previously hidden because the notification was expanded and the old banner did not fully fit. - New notifications are immediately hidden if the user mouses away from them. - If a new notification is updated while it is shown, we extend the time it will be shown. - If a new notification is updated while it is hiding, we stop hiding it and show it again. - If a summary notification is updated while it is hiding, we let it finish hiding and show a new notification with the updated information. Implementation details: - Single-line notifications now have 4px bottom padding instead of 8px, which means that their height matches the tray height, they are fully shown in the banner mode, and don't pop out by 4px when the notification is expanded. - Notification keeps a flag that indicates whether it is expanded, updates its expanded look when it is updated, and emits an 'expanded' signal indicating that its layout has possibly changed. The message tray connects to this 'expanded' signal when it is showing a notification in the expanded state and updates the position of the notification accordingly when this signal is received so that the notification is fully shown. This is better than connecting to 'notify::height' signal on the notification bin, since it results in fewer callbacks.
*** Bug 625502 has been marked as a duplicate of this bug. ***
(In reply to comment #17) > (From update of attachment 169395 [details] [review]) > > Was urgent-notification-expansion previously broken? It doesn't seem to work in > master. It was broken for the notifications that had a banner that didn't fit and didn't have any other content or buttons. The call to popOut() would return false because we haven't added the banner body by then yet, so we would never connect to the 'notify::height' signal on the notification bin and expand the notification when the banner body was added. > The content-finalized signal does not appear to be necessary; > expandNotification() sets up a signal handler to re-call itself if the height > of the notification changes, so the notification does get fully expanded after > the bannerbody is added anyway. We were not connecting to 'notify::height' when popOut() was returning false, but a simple change would have been to connect to it in either case. > At any rate, this is probably not the fault of these changes, but the > combination of sliding and fading in at the same time does not work well IMHO; > the fade-in ends up obscuring the slide-in, so it looks like the notification > just appears there. > > I'm not really sure why we need to have the notifications both sliding and > fading anyway? This is how we had it before. The plan for the animation of the summary notifications is to have them fade in and only slightly move, as described in bug 624900. Maxim was going to work on that. Let's wait till it's implemented to see if the fade in for new notifications fits in better with the overall feel too. I think it is there to make the notifications appearing and disappearing feel less abrupt. > > >+ _expandNotification: function(autoExpanding) { > >+ let poppedOut = this._notification.popOut(!autoExpanding); > >+ if (this._notification && (autoExpanding || poppedOut)) { > > the second line checks if this._notification is non-null, but the first line > has already dereferenced it. (I'm not sure it's actually possible for > this._notification to be null at that point, so you might want to just remove > the check.) I had a separate check for it, but must have lost it during a rearrangement. We no longer call _expandNotification() from a callback with the new patch, so I removed this check there.
Comment on attachment 169461 [details] [review] Fix various details of how notifications are shown >- Single-line notifications now have 4px bottom padding instead of 8px, which > means that their height matches the tray height, they are fully shown in the > banner mode, and don't pop out by 4px when the notification is expanded. But we don't actually want to have single-line notifications have less padding than multi-line ones, do we? It seems like the fix should be that the open out to the height of the notification, not the height of the tray. (Or else the tray should be 4px taller.) >+ this.connect('hide-summary-notification-complete', >+ Lang.bind(this, >+ function() { >+ this._onNotify(source, notification); >+ })); You don't need a signal for this; it's entirely internal to MessageTray, you can just call the appropriate code directly from the appropriate place. Eg, just set a flag like "this._summaryNotificationReshowAfterHide = true", and check that from _hideSummaryNotificationCompleted.
Comment on attachment 169461 [details] [review] Fix various details of how notifications are shown Commited with suggested changes.
(In reply to comment #21) > (From update of attachment 169461 [details] [review]) > >- Single-line notifications now have 4px bottom padding instead of 8px, which > > means that their height matches the tray height, they are fully shown in the > > banner mode, and don't pop out by 4px when the notification is expanded. > > But we don't actually want to have single-line notifications have less padding > than multi-line ones, do we? It seems like the fix should be that the open out > to the height of the notification, not the height of the tray. (Or else the > tray should be 4px taller.) We actually do want single-line notifications to have less padding than multi-line ones. We don't want the single-line notifications to be taller than the tray height or make the tray 4px taller. > > >+ this.connect('hide-summary-notification-complete', > >+ Lang.bind(this, > >+ function() { > >+ this._onNotify(source, notification); > >+ })); > > You don't need a signal for this; it's entirely internal to MessageTray, you > can just call the appropriate code directly from the appropriate place. Eg, > just set a flag like "this._summaryNotificationReshowAfterHide = true", and > check that from _hideSummaryNotificationCompleted. Done.