Created attachment 226842 [details] [review]
userMenu: Hide menu immediately before suspending

The same logic as for commit 1f30670c1d8d8 applies to the case
where we lock the screen before suspending - we don't want the
menu to jump to the opposite screen side to fade out, so remove
the animation altogether.

Review of attachment 226842 [details] [review]:

Yes

Attachment 226842 [details] pushed (to both master and gnome-3-6) as f6458f2 - userMenu: Hide menu immediately before suspending

You guys are quick, I was testing this before acking the patch and found out that the menu still flashes briefly on the other side of the monitor before the lock screen comes down.

Spent a bit trying to understand why but couldn't figure it out. Does it not flash for you even with the patch?

(In reply to comment #4)
> You guys are quick, I was testing this before acking the patch and found out
> that the menu still flashes briefly on the other side of the monitor before the
> lock screen comes down.

Are you insinuating that neither Giovanni nor I actually tested that patch? Uhm ...


> Spent a bit trying to understand why but couldn't figure it out. Does it not
> flash for you even with the patch?

It does. I've looked into it a bit now, but it's still a mystery to me. I'll have to run now, but will try to figure it out later (unless you fix it first ;-)

Created attachment 226855 [details] [review]
userMenu: Fix closing the menu immediately before locking

Wow, that took me a while:

On the lock screen, the user menu moves to the opposite side of
the top bar, so to avoid the popup jumping across the screen while
fading out, we want to hide it with no animation.
We do this from the approriate items' activate handlers by calling
close() on the menu with an appropriate animation parameter.
It turns out that this may or may not work, as the menu itself
connects to the item's activate signal and calls close() requesting
a full animation - and as close() will bail out early if the menu
has already been closed, the signal handler that is run first
determines whether menu closing is animated or not.
Work around this by hiding the menu actor explicitly (and rely on
the handler in PopupMenu to actually close the menu).

Aha. Good catch. But I'm not a fan of the fix. To me, if we request no animation and it's already closing and there's an existing animation, we should cancel the existing animation.

(In reply to comment #7)
> Aha. Good catch. But I'm not a fan of the fix.

Not a big fan either, but it looked like the safest choice.


> To me, if we request no animation and it's already closing and 
> there's an existing animation, we should cancel the existing animation.

I'm not entirely sure (patch coming though) - it's a bit like changing "whatever runs first wins" to "whatever runs last wins (except for no animation, which always wins)". It fixes the issue of course, but I'm not quite convinced ...

Created attachment 226859 [details] [review]
popupMenu: Overwrite ongoing animations when calling close repeatedly

Currently close() is a no-op when the menu has already been closed.
However, repeated calls could pass different animation parameters.
For instance in the user menu, we try to hide the menu immediately
before locking the screen, to avoid the popup jumping across the
screen while fading out - as we do this from the corresponding
item's activate handler, the closing is still animated if the menu's
own handler (which requests a full animation) is run first.
Fix this by changing close() to overwrite ongoing animations before
bailing out early.

Review of attachment 226859 [details] [review]:

::: js/ui/boxpointer.js
@@ +142,3 @@
         this._muteInput();
 
+        Tweener.removeTweens(this);

Won't this prevent a previously passed in onComplete function from being called?

(In reply to comment #10)
> Won't this prevent a previously passed in onComplete function from being
> called?

Yes, but that's what I'd expect when canceling a previous tween. Are you thinking of any particular cases where you expect this to be a problem? I expect the patch to work perfectly fine without that line, it just appeared more correct to me.

Review of attachment 226859 [details] [review]:

(In reply to comment #11)
> (In reply to comment #10)
> > Won't this prevent a previously passed in onComplete function from being
> > called?
> 
> Yes, but that's what I'd expect when canceling a previous tween. Are you
> thinking of any particular cases where you expect this to be a problem? I
> expect the patch to work perfectly fine without that line, it just appeared
> more correct to me.

Ok, I can't think of any right now. Just thought that it could be surprising for future API users but I think you're right. It now works correctly too ;-)

Comment on attachment 226859 [details] [review]
popupMenu: Overwrite ongoing animations when calling close repeatedly

Attachment 226859 [details] pushed as 98b313c - popupMenu: Overwrite ongoing animations when calling close repeatedly