*** Bug 629969 has been marked as a duplicate of this bug. ***

Created attachment 172948 [details] [review]
    [Fix: Bug 630767] Increase / add grace period for message tray hiding.     https://bugzilla.gnome.org/show_bug.cgi?id=630767

modified:   messageTray.js

Review of attachment 172948 [details] [review]:

This looks good overall and works as expected! Most of the comments are about naming, commenting, and cleaning up the code a bit more.

The commit message title doesn't need to have the bug number. You also need to have the title and the body of the commit message to be more descriptive about what is actually accomplished with this particular patch. See this e-mail for an explanation of what we'd like to see in the commit messages: http://lists.cairographics.org/archives/cairo/2008-September/015092.html (It's linked to from the WorkingWithPatches wiki page.)

::: js/ui/messageTray.js
@@ +27,3 @@
 
+const INSENSITIVE_MOUSE_AREA = 20;
+const INSENSITIVE_MOUSE_AREA_TIMEOUT = 1.5;

I think MOUSE_LEFT_ACTOR_THRESHOLD is a more suitable name than INSENSITIVE_MOUSE_AREA for the distance we consider too large for the mouse to travel outside an actor without the user meaning to leave the actor. You should add a comment about why we need the variable too.

The HIDE_TIMEOUT is 0.2 seconds right now, so extending it by 1.5 seconds is a lot. I think you can just use LONGER_HIDE_TIMEOUT value which is 0.6 seconds and not add an extra constant. This will result in the 0.8 or 1.2 seconds total timeout if the mouse didn't move far enough in the first 0.2 or 0.6 seconds, which sounds fine.

@@ +1171,3 @@
+            // We record the position of the pointer the moment it leaves the tray. These values are used later to determine
+            // how far the mouse had left after this function ends and thus determine whether or not we should need a longer
+            // timeout if the mouse had not left very far. These values are compared in _onTrayLeftTimeout.

Here is my suggestion for this comment:

// We record the position of the mouse the moment it leaves the tray. These coordinates are used in
// this._onTrayLeftTimeout() to determine if the mouse has moved far enough during the initial timeout
// for us to consider that the user intended to leave the tray and therefore hide the tray. If the mouse
// is still close to its previous position, we extend the timeout once.

@@ +1174,3 @@
+            let [x, y, mods] = global.get_pointer();
+            this._lastSeenMouseInTrayX = x;
+            this._lastSeenMouseInTrayY = y;

At this moment, the mouse is no longer in the tray, so this._trayLeftMouseX and this._trayLeftMouseY would be more accurate names. (Also, similar to this._showNotificationMouseX, this._showNotificationMouseY and this._lastSeenMouseY names.)

We should also unset these variables (set them to -1) when we are removing this._trayLeftTimeoutId in the case when this.actor.hover is true, i.e. the tray is hovered over again.

@@ +1187,3 @@
+        if (this._trayLeftTimeoutId) {
+            Mainloop.source_remove(this._trayLeftTimeoutId);
+            this._trayLeftTimeoutId = 0;

This block is unnecessary because you will reassign this._trayLeftTimeoutId in both cases.

@@ +1190,3 @@
+        }
+        let [x, y, mods] = global.get_pointer();
+        // If the mouse didn't move out of a 20x20 pixel square after a short timeout

The area you end up checking here is 40 x (this.actor.y - this._lastSeenMouseInTrayY + INSENSITIVE_MOUSE_AREA) pixels. I agree that it makes sense to extend the timeout if the mouse moves below this._lastSeenMouseInTrayY by more than INSENSITIVE_MOUSE_AREA, but doesn't reach the tray (e.g. it left at the top of a very tall notification and moved down), which is effectively what you are doing by letting y be any value less than this.actor.y. However, you can just omit this check because if y > this.actor.y the mouse will be hovering over the tray and we won't be here. You can explain this in a comment. Something like:

// We extend the timeout once if the mouse moved no further than MOUSE_LEFT_ACTOR_THRESHOLD
// to either side or up. We don't check how far down the mouse moved because any point above
// the tray, but below the exit coordinate, is close to the tray.

@@ +1197,3 @@
+            // Setting the Y-coordinate to the bottom of the screen will ALWAYS break the above condition, so only setting
+            // the Y-coordinate is needed.
+            this._lastSeenMouseInTrayY = this.actor.y + INSENSITIVE_MOUSE_AREA;

This doesn't set the Y coordinate to the bottom of the screen. The reason this breaks the loop is that this sets the Y coordinate to something greater than this.actor.y and you check that y < this.actor.y in the if statement.

If we only want to extend the callback once, it would make sense to set this._lastSeenMouseInTrayX to -1 here and check that first in the if statement. That would be similar to how we treat this._showNotificationMouseX .

You should also add a comment explaining that we only extend the timeout once.

@@ +1201,3 @@
+                                                             Lang.bind(this, this._onTrayLeftTimeout));
+        } else {
+            Mainloop.source_remove(this._trayLeftTimeoutId);

This line is unnecessary because we return false from this function which will remove the timeout.

Created attachment 173059 [details] [review]
    Description: Increasing the timeout if the mouse hasn't left far from the tray.     Significance: It is annoying to have the tray instantly hide the moment your cursor accidentally leave the tray.

modified:   messageTray.js

(In reply to comment #4)
> Created an attachment (id=173059) [details] [review]
>     Description: Increasing the timeout if the mouse hasn't left far from the
> tray.     Significance: It is annoying to have the tray instantly hide the
> moment your cursor accidentally leave the tray.
> 
> modified:   messageTray.js

I was resting yesterday so I didn't get to update on this. I know that you will be away for this weekend, but I hope that this is perfect anyway to be updated as "reviewed".

Created attachment 173060 [details] [review]
Description: Increase the timeout if the mouse hasn't left far from the tray. Significance: It is annoying to have the tray instantly hide the moment your cursor accidentally leaves the tray. modified: messageTray.js

EDIT: removed trailing whitespaces in patch.

Review of attachment 173060 [details] [review]:

This looks much cleaner! All the comments are very minor.

The commit message should have a distinct subject line and body explaining the reason for the change. Do "git log" to see examples. For example, when you do the commit, you should just put

"Increase the timeout for hiding the tray if the mouse didn't move far from it

We shouldn't hide the tray as quickly if the user might have left it unintentionally,
such as when moving the mouse over to a different tray item or using the scroll bar
in the chat notification."

::: js/ui/messageTray.js
@@ +28,3 @@
 
+// Needed for delayed hiding of tray if cursor is within MOUSE_LEFT_ACTOR_THRESHOLD
+// range of leaving the message tray.

A slight modification to the comment:

// We delay hiding of the tray if the mouse is within MOUSE_LEFT_ACTOR_THRESHOLD
// range from the point where it left the tray.

@@ +1199,3 @@
+            x > this._trayLeftMouseX - MOUSE_LEFT_ACTOR_THRESHOLD &&
+            y < this.actor.y &&
+            this._trayLeftMouseX > -1) {

You should check this._trayLeftMouseX > -1 first because it is a distinct condition under which needs to be true for us to select this clause.

The check y < this.actor.y is not needed because if y >= this.actor.y the tray is being hovered over and we would not be here (the callback would be removed). It would be the right think to extend the timeout even if y >= this.actor.y too.

So it should be
if (this._trayLeftMouseX > -1 &&
    y > this._trayLeftMouseY - MOUSE_LEFT_ACTOR_THRESHOLD &&
    x < this._trayLeftMouseX + MOUSE_LEFT_ACTOR_THRESHOLD &&
    x > this._trayLeftMouseX - MOUSE_LEFT_ACTOR_THRESHOLD)

It's effectively the same, just cleaner.

@@ +1200,3 @@
+            y < this.actor.y &&
+            this._trayLeftMouseX > -1) {
+            this._trayLeftMouseX = -1;

Set this._trayleftMouseY to -1 here too.

@@ +1202,3 @@
+            this._trayLeftMouseX = -1;
+            this._trayLeftTimeoutId = Mainloop.timeout_add(LONGER_HIDE_TIMEOUT*1000,
+                                                             Lang.bind(this, this._onTrayLeftTimeout));

You need spaces aroung '*' sign.

Created attachment 173417 [details] [review]
Increase the timeout or hiding the tray if the mouse didn't move far from it.

We shouldn't hide the tray as quickly if the user might have left it unintentionally, such as when moving the mouse over to a different tray item or using the scroll bar in the chat notification.

Created attachment 173421 [details] [review]
Increase the timeout or hiding the tray if the mouse didn't move far from it.

We shouldn't hide the tray as quickly if the user might have left it
unintentionally, such as when moving the mouse over to a different tray item or
using the scroll bar in the chat notification.

Created attachment 173422 [details] [review]
Increase the timeout or hiding the tray if the mouse didn't move far from it.

We shouldn't hide the tray as quickly if the user might have left it
unintentionally, such as when moving the mouse over to a different tray item or
using the scroll bar in the chat notification.

Created attachment 173564 [details] [review]
Increase the timeout or hiding the tray if the mouse didn't move far from it.

We shouldn't hide the tray as quickly if the user might have left it
unintentionally, such as when moving the mouse over to a different tray item or
using the scroll bar in the chat notification.

Review of attachment 173564 [details] [review]:

Thank you for the patch! I pushed it with the tiniest changes.

1) Changed a typo from "or" to "for" in the commit message subject and removed a period from the end of it. The style is to not use a period in the end of the commit message subject, like you would not do in the e-mail message subject, but to use it in the end of the sentences in the commit message body.

::: js/ui/messageTray.js
@@ +1201,3 @@
+            this._trayLeftMouseX = -1;
+            this._trayLeftTimeoutId = Mainloop.timeout_add(LONGER_HIDE_TIMEOUT * 1000,
+                                                             Lang.bind(this, this._onTrayLeftTimeout));

2) Added this._trayLeftMouseY = -1; as noted in the previous review and aligned the two arguments to timeout_add() .