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 629368 - alt-tab doesn't always close correctly
alt-tab doesn't always close correctly
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-09-11 16:24 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2010-09-11 20:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[appSwitcher] Close the switcher on Meta_L/R release (1.07 KB, patch)
2010-09-11 16:35 UTC, drago01
reviewed Details | Review
[appSwitcher] Close the switcher on Meta_L/R release (1.07 KB, patch)
2010-09-11 16:38 UTC, drago01
committed Details | Review
[appSwitcher] Check the modifier state rather than assuming Meta (1.08 KB, patch)
2010-09-11 19:10 UTC, drago01
needs-work Details | Review
[appSwitcher] Check the modifier state rather than assuming Meta (1.04 KB, patch)
2010-09-11 19:38 UTC, drago01
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2010-09-11 16:24:42 UTC
Repro:

   1. Alt-tab-shift
   2. Release tab, then alt, then shift.
Comment 1 drago01 2010-09-11 16:35:02 UTC
Created attachment 170031 [details] [review]
[appSwitcher] Close the switcher on Meta_L/R release

The keyReleaseEvent handler does not take into account that a user might
release Alt while still holding shift (the keycode because Meta_L/R in that case).

The result is that the switcher stays open which is unexpected.
Comment 2 drago01 2010-09-11 16:38:57 UTC
Created attachment 170033 [details] [review]
[appSwitcher] Close the switcher on Meta_L/R release

*) Don't remove the blank line ...
Comment 3 Florian Müllner 2010-09-11 16:54:17 UTC
Review of attachment 170031 [details] [review]:

The code looks good, just the commit message needs fixing:

the keycode because ... => the keycode becomes

(also that line does not fit in a standard terminal window, but maybe I'm just being anal here)
Comment 4 drago01 2010-09-11 17:02:31 UTC
Attachment 170033 [details] pushed as 246d9f1 - [appSwitcher] Close the switcher on Meta_L/R release
Comment 5 Dan Winship 2010-09-11 18:53:51 UTC
This is not strictly correct; the fact that Shift+Alt gives Meta (and not some other modifier) is just a fact about your keyboard layout, not an innate truth.

What we really want to test is "is the Alt modifier no longer set after the keyrelease". You can't do that with Shell.get_event_state(event), because the event's state still reflects the alt key being down, but if you call Shell.get_pointer() to get a new modifier state, it should be correct (I think).
Comment 6 drago01 2010-09-11 19:10:32 UTC
Created attachment 170038 [details] [review]
[appSwitcher] Check the modifier state rather than assuming Meta

It isn't guaranteed that Alt+Shift equals to Meta, so we have to check
the modifier instead.
Comment 7 Dan Winship 2010-09-11 19:14:11 UTC
Comment on attachment 170038 [details] [review]
[appSwitcher] Check the modifier state rather than assuming Meta

>+        if (keysym == Clutter.Alt_L || keysym == Clutter.Alt_R || state == 0)
>             this._finish();

don't even look at keysym. The dialog shouldn't go away if the user presses and releases the opposite alt key from the one they started with.
Comment 8 drago01 2010-09-11 19:38:12 UTC
Created attachment 170039 [details] [review]
[appSwitcher] Check the modifier state rather than assuming Meta

It isn't guaranteed that Alt+Shift equals to Meta, so we have to check
the modifier instead.
Comment 9 Dan Winship 2010-09-11 20:32:41 UTC
Comment on attachment 170039 [details] [review]
[appSwitcher] Check the modifier state rather than assuming Meta

(assuming you tested that it works)
Comment 10 drago01 2010-09-11 20:35:51 UTC
(In reply to comment #9)
> (From update of attachment 170039 [details] [review])
> (assuming you tested that it works)

I did ...
Comment 11 drago01 2010-09-11 20:42:54 UTC
Attachment 170039 [details] pushed as 907dd43 - [appSwitcher] Check the modifier state rather than assuming Meta