After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686484 - userMenu: Hide menu immediately before suspending
userMenu: Hide menu immediately before suspending
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-10-19 16:55 UTC by Florian Müllner
Modified: 2012-10-24 07:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
userMenu: Hide menu immediately before suspending (1.01 KB, patch)
2012-10-19 16:55 UTC, Florian Müllner
committed Details | Review
userMenu: Fix closing the menu immediately before locking (2.11 KB, patch)
2012-10-19 21:25 UTC, Florian Müllner
none Details | Review
popupMenu: Overwrite ongoing animations when calling close repeatedly (2.00 KB, patch)
2012-10-19 22:17 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2012-10-19 16:55:23 UTC
See patch.
Comment 1 Florian Müllner 2012-10-19 16:55:27 UTC
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.
Comment 2 Giovanni Campagna 2012-10-19 17:07:31 UTC
Review of attachment 226842 [details] [review]:

Yes
Comment 3 Florian Müllner 2012-10-19 17:11:28 UTC
Attachment 226842 [details] pushed (to both master and gnome-3-6) as f6458f2 - userMenu: Hide menu immediately before suspending
Comment 4 Rui Matos 2012-10-19 17:47:18 UTC
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?
Comment 5 Florian Müllner 2012-10-19 19:44:18 UTC
(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 ;-)
Comment 6 Florian Müllner 2012-10-19 21:25:59 UTC
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).
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-10-19 21:32:35 UTC
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.
Comment 8 Florian Müllner 2012-10-19 22:06:29 UTC
(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 ...
Comment 9 Florian Müllner 2012-10-19 22:17:27 UTC
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.
Comment 10 Rui Matos 2012-10-20 18:31:01 UTC
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?
Comment 11 Florian Müllner 2012-10-22 17:48:54 UTC
(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.
Comment 12 Rui Matos 2012-10-23 23:40:32 UTC
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 13 Florian Müllner 2012-10-24 07:04:50 UTC
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