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 643111 - allow using volume hardware keys / shortcuts to take screenshots in the overview
allow using volume hardware keys / shortcuts to take screenshots in the overview
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 647707 647741 648093 649887 653028 656348 663075 664379 665321 665719 670159 670345 671369 672124 673642 673740 674433 675125 675751 676182 676244 676876 678251 678651 679413 679789 683238 683611 683749 684065 685173 685272 685318 685815 686466 687068 687341 687443 687587 688046 688177 689288 691594 692057 692132 692140 692357 692441 693465 693908 (view as bug list)
Depends on:
Blocks: 693016
 
 
Reported: 2011-02-23 18:48 UTC by William Jon McCann
Modified: 2014-11-09 17:23 UTC
See Also:
GNOME target: 3.8
GNOME version: ---


Attachments
some video demonstrated a bug (352.11 KB, video/webm)
2012-12-03 09:25 UTC, Sniffy
  Details
keybindings: Add external grab api (8.66 KB, patch)
2012-12-05 08:41 UTC, Florian Müllner
none Details | Review
keybindings: Add accessor function for keysym (1.44 KB, patch)
2012-12-05 08:41 UTC, Florian Müllner
accepted-commit_now Details | Review
shellDBus: Add methods to handle key grabs (5.31 KB, patch)
2012-12-05 08:41 UTC, Florian Müllner
needs-work Details | Review
keybindings: Add external grab API (8.89 KB, patch)
2012-12-14 09:02 UTC, Florian Müllner
reviewed Details | Review
Kill MetaVirtualModifiers in favor of GdkModifierType (23.32 KB, patch)
2012-12-14 11:13 UTC, Florian Müllner
reviewed Details | Review
windowManager: Adjust to removal of Meta.VirtualModifier (1.95 KB, patch)
2012-12-14 11:13 UTC, Florian Müllner
none Details | Review
keybindings: Generalize mechanism to generate dynamic keybinding actions (1.53 KB, patch)
2013-02-01 15:13 UTC, Florian Müllner
committed Details | Review
keybindings: Add external grab API (9.84 KB, patch)
2013-02-01 15:13 UTC, Florian Müllner
accepted-commit_now Details | Review
prefs: Use an unsigned value for META_KEYBINDING_ACTION_NONE (969 bytes, patch)
2013-02-01 15:13 UTC, Florian Müllner
committed Details | Review
shellDBus: Add methods to handle key grabs (2.81 KB, patch)
2013-02-01 15:14 UTC, Florian Müllner
reviewed Details | Review
main: Move KeybindingMode into Shell (24.41 KB, patch)
2013-02-01 15:14 UTC, Florian Müllner
committed Details | Review
keybindings: Add external grab API (12.72 KB, patch)
2013-03-01 03:11 UTC, Florian Müllner
committed Details | Review
shellDBus: Add methods to handle key grabs (6.10 KB, patch)
2013-03-01 03:16 UTC, Florian Müllner
committed Details | Review

Description William Jon McCann 2011-02-23 18:48:26 UTC
It is a bit unexpected that the hardware volume keys don't work while in the overview.

Especially since chat messages and stuff ping at you while you are in there (and while you have a migraine :( )
Comment 1 Aaron Hankin 2011-02-24 07:46:05 UTC
In addition to the volume keys, it would be nice to be able to use the play/pause, next, and stop hardware keys to interact with my music player in overview.
Comment 2 Bastien Nocera 2011-03-07 23:54:22 UTC
I should note that the power key doesn't work in overview mode either.
Comment 3 Jasper St. Pierre (not reading bugmail) 2011-04-14 07:53:37 UTC
*** Bug 647741 has been marked as a duplicate of this bug. ***
Comment 4 Dan Winship 2011-04-14 12:34:50 UTC
Do XGrabKey and XSendEvent work together? If so, the easiest fix would be to have gnome_shell_plugin_xevent_filter() notice when it receives "a key that g-s-d should have dealt with", and re-send it to the root window. Though we still won't get the OSD (pop-up volume-changed window); see bug 607029 for that (though beware that the earlier part of the discussion involves an abandoned plan to move OSDs to the message tray).
Comment 5 Dan Winship 2011-04-18 12:58:05 UTC
*** Bug 648093 has been marked as a duplicate of this bug. ***
Comment 6 Rui Matos 2011-05-10 19:09:38 UTC
*** Bug 649887 has been marked as a duplicate of this bug. ***
Comment 7 Florian Müllner 2011-06-20 17:51:05 UTC
*** Bug 653028 has been marked as a duplicate of this bug. ***
Comment 8 Rui Matos 2011-08-11 15:38:21 UTC
*** Bug 656348 has been marked as a duplicate of this bug. ***
Comment 9 Misha Shnurapet 2011-08-12 00:34:03 UTC
The same applies to the custom keyboard shortcuts like the one to launch a terminal (I use Ctrl+Alt+T).
Comment 10 Rui Matos 2011-10-31 03:40:11 UTC
*** Bug 663075 has been marked as a duplicate of this bug. ***
Comment 11 Severo Raz 2011-10-31 06:56:59 UTC
It is evident that the gnome-shell is aware of keystrokes, for pressing the super key (or whatever key combination one has binded for the opening of the Activities overview) does toggle the overview mode.
Comment 12 Ray Strode [halfline] 2011-11-02 17:25:45 UTC
Wolter, gnome-shell doesn't handle volume.  gnome-settings-daemon is the thing that handles that and it isn't aware of the keystrokes when the overview is up.
Comment 13 Florian Müllner 2011-11-12 21:39:08 UTC
The keyboard shortcuts for taking a screenshots are now located in gnome-settings-daemon as well, so our old hack to make them available in the overview don't work anymore. It's the same issue, so I'm renaming the bug report to cut down on duplicates ...
Comment 14 Jasper St. Pierre (not reading bugmail) 2011-11-19 14:47:26 UTC
*** Bug 664379 has been marked as a duplicate of this bug. ***
Comment 15 Florian Müllner 2011-12-01 19:34:52 UTC
*** Bug 665321 has been marked as a duplicate of this bug. ***
Comment 16 Baptiste Mille-Mathias 2011-12-07 09:41:25 UTC
*** Bug 665719 has been marked as a duplicate of this bug. ***
Comment 17 l300lvl 2011-12-19 04:55:56 UTC
I wasn't fully aware of this problem until I saw this bug. Thanks for pointing it out so I can confirm it, none of my keyboard shortcuts work in overview. This almost seems intentional.
Comment 18 Julien Olivier 2012-02-13 10:05:39 UTC
Why is this bug stille "unconfirmed"? Shouldn't it be changed to "new"?
Comment 19 Milan Bouchet-Valat 2012-02-13 11:17:38 UTC
(In reply to comment #18)
> Why is this bug stille "unconfirmed"? Shouldn't it be changed to "new"?
We don't make a difference between UNCONFIRMED and NEW, so that doesn't matter.
Comment 20 Rui Matos 2012-02-15 18:58:21 UTC
*** Bug 670159 has been marked as a duplicate of this bug. ***
Comment 21 Florian Müllner 2012-02-18 13:32:21 UTC
*** Bug 670345 has been marked as a duplicate of this bug. ***
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-03-05 14:37:56 UTC
*** Bug 671369 has been marked as a duplicate of this bug. ***
Comment 23 Rui Matos 2012-03-15 12:53:32 UTC
*** Bug 672124 has been marked as a duplicate of this bug. ***
Comment 24 Jasper St. Pierre (not reading bugmail) 2012-04-06 14:45:50 UTC
*** Bug 673642 has been marked as a duplicate of this bug. ***
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-04-09 19:20:19 UTC
*** Bug 673740 has been marked as a duplicate of this bug. ***
Comment 26 KTP 2012-04-17 10:37:06 UTC
I cant press prtsc either in overview.  Only way to take screenshot is interactive mode with delay in ovewview.  worked in 3.2 (mint 12)
Comment 27 Florian Müllner 2012-04-19 20:43:50 UTC
*** Bug 674433 has been marked as a duplicate of this bug. ***
Comment 28 Cosimo Cecchi 2012-04-30 12:48:05 UTC
*** Bug 675125 has been marked as a duplicate of this bug. ***
Comment 29 Mike Herwig 2012-05-01 16:24:20 UTC
Also the shortcuts super + l, super + f, super + e neever work if I set them in the shortcuts list.
Comment 30 Julien Olivier 2012-05-02 08:31:40 UTC
I have just noticed today that, if I set up my power button to suspend my computer, then press the power button while the activities overview is displayed, nothing happens. If I leave the overview, then press the power button, it suspends the computer as expected.
Comment 31 Andy Ross 2012-05-02 15:57:29 UTC
Just to pile on, the touchpad toggle key (reports KEY_F21) on the Dell XPS L501x is also affected.  It doesn't work in the overview, which is especially annoying because the hardware toggles an LED when the key is pressed to indicate "disabled", and of course that gets out of sync when the desktop doesn't see the event.

Are there any plans on how to address this?  Really this is my single biggest nit with gnome-shell.  The lack of keyboard navigability in the overview is a huge pain.
Comment 32 Florian Müllner 2012-05-09 14:05:55 UTC
*** Bug 675751 has been marked as a duplicate of this bug. ***
Comment 33 Jasper St. Pierre (not reading bugmail) 2012-05-16 18:11:44 UTC
*** Bug 676182 has been marked as a duplicate of this bug. ***
Comment 34 Florian Müllner 2012-05-17 14:37:31 UTC
*** Bug 676244 has been marked as a duplicate of this bug. ***
Comment 35 tuxor 2012-06-05 22:00:26 UTC
Still the same on Fedora 17 (64 bit) with Gnome 3.4. Seems to be very hard to handle?
Comment 36 Bastien Nocera 2012-06-18 09:28:41 UTC
*** Bug 678251 has been marked as a duplicate of this bug. ***
Comment 37 Cosimo Cecchi 2012-06-18 14:37:08 UTC
*** Bug 676876 has been marked as a duplicate of this bug. ***
Comment 38 Rui Matos 2012-06-22 23:10:14 UTC
*** Bug 678651 has been marked as a duplicate of this bug. ***
Comment 39 Milan Bouchet-Valat 2012-07-05 21:07:11 UTC
*** Bug 679413 has been marked as a duplicate of this bug. ***
Comment 40 Florian Müllner 2012-07-12 10:39:45 UTC
*** Bug 679789 has been marked as a duplicate of this bug. ***
Comment 41 Matthias Clasen 2012-07-31 13:56:46 UTC
A proposal that was discussed at Guadec is to have the shell provide a D-Bus api for grabbing keys, like

gboolean GrabKey (uint keysym, uint modifiers, uint flags)

UngrabKey (uint keysym, uint modifiers)

KeyActivated (uint keysym, uint modifiers)

When calling GrabKey for a certain keysym/modifier combination, the caller
would pass a set of flags like:

GRAB_KEY_FLAG_OVERVIEW     : 1
GRAB_KEY_FLAG_LOCK_SCREEN  : 2
GRAB_KEY_FLAG_LOGIN_SCREEN : 4

to indicate whether they want to handle the key in special situations as well.

The shell establishes a key grab, and sends KeyActivated signals back to the unique name that has added the grab. 

Calling UngrabKey does what its name indicates. If the 'owner' of the grab drops off the bus, the shell will drop the grab as well.

Maybe it should be possible for the owner to change the flags by re-grabbing, but GrabKey should fail if another unique name already owns the key.


gnome-settings-daemon will then have to do something like

if (gnome shell is running) 
   call GrabKey
else
   XGrabKey (...)

and handle the KeyActivated signal.
Comment 42 Jasper St. Pierre (not reading bugmail) 2012-07-31 14:19:16 UTC
Couldn't a fallback process implement the same interface, so we don't branch everywhere?
Comment 43 Florian Müllner 2012-07-31 15:43:30 UTC
(In reply to comment #42)
> Couldn't a fallback process implement the same interface, so we don't branch
> everywhere?

It boils down to one branch in a single method - that certainly sounds saner than gnome-settings-daemon talking to itself over dbus :-)
Comment 44 Jasper St. Pierre (not reading bugmail) 2012-07-31 15:46:44 UTC
But my polymorphism!
Comment 45 l300lvl 2012-08-07 00:07:41 UTC
SO, is it possible that this might actually see a fix before Gnome3 reaches legacy status, not to sound harsh?
Comment 46 Ray Strode [halfline] 2012-08-07 01:04:28 UTC
This bug has been open a long time, but it's a tricky problem. Note the last few comments, though.  Progress is being made and now that some of the details have been worked out, it shouldn't be too long.
Comment 47 Jasper St. Pierre (not reading bugmail) 2012-09-03 06:38:37 UTC
*** Bug 683238 has been marked as a duplicate of this bug. ***
Comment 48 Rui Matos 2012-09-09 14:26:29 UTC
*** Bug 683611 has been marked as a duplicate of this bug. ***
Comment 49 Florian Müllner 2012-09-10 22:01:27 UTC
*** Bug 683749 has been marked as a duplicate of this bug. ***
Comment 50 Florian Müllner 2012-09-15 19:03:12 UTC
*** Bug 684065 has been marked as a duplicate of this bug. ***
Comment 51 Rui Matos 2012-09-18 09:27:10 UTC
*** Bug 647707 has been marked as a duplicate of this bug. ***
Comment 52 Florian Müllner 2012-10-01 06:35:34 UTC
*** Bug 685173 has been marked as a duplicate of this bug. ***
Comment 53 Bastien Nocera 2012-10-02 15:34:35 UTC
*** Bug 685272 has been marked as a duplicate of this bug. ***
Comment 54 Yotam Benshalom 2012-10-04 21:37:44 UTC
This is a serious usability issue, because the shortcuts for changing the keyboard layout are not working too.
Comment 55 Elad Alfassa 2012-10-06 20:31:49 UTC
(In reply to comment #54)
> This is a serious usability issue, because the shortcuts for changing the
> keyboard layout are not working too.
Indeed, because if you go into the overview while using the wrong keyboard layout you have to exit the overview in order to switch layout (this was not the case in 3.4), or use the mouse to do that.

any update on this issue? what are the chances to see this fixed in one of the 3.6 bugfix releases?
Comment 56 Florian Müllner 2012-10-19 13:36:51 UTC
*** Bug 686466 has been marked as a duplicate of this bug. ***
Comment 57 Rui Matos 2012-11-01 13:59:01 UTC
*** Bug 687341 has been marked as a duplicate of this bug. ***
Comment 58 Florian Müllner 2012-11-02 14:02:13 UTC
*** Bug 687443 has been marked as a duplicate of this bug. ***
Comment 59 Reda Lazri 2012-11-04 13:39:02 UTC
Not sure why my report was marked as a duplicate of this one. Alt+Shift used to work in 3.4 I think. Volume keys never worked. Also, Alt+F2 is working in the overview(3.6.1), by my logic, so should Alt+Shift.
Comment 60 Giovanni Campagna 2012-11-04 13:45:07 UTC
No, the duplicate marker is correct. Alt+Shift in 3.4.1 was handled in the X server by XKB, while it's handled by gnome-settings-daemon in 3.6.1, and thus is subject to the same limitations as other keybindings.
Alt-F2 on the other hand is handled by the shell directly, so it's not affect by the overview modal grab.
Comment 61 Rui Matos 2012-11-05 08:22:16 UTC
*** Bug 687587 has been marked as a duplicate of this bug. ***
Comment 62 Florian Müllner 2012-11-10 16:34:21 UTC
*** Bug 688046 has been marked as a duplicate of this bug. ***
Comment 63 Florian Müllner 2012-11-10 16:44:25 UTC
*** Bug 688046 has been marked as a duplicate of this bug. ***
Comment 64 Florian Müllner 2012-11-12 15:56:48 UTC
*** Bug 688177 has been marked as a duplicate of this bug. ***
Comment 65 Giovanni Campagna 2012-11-29 15:32:37 UTC
*** Bug 689288 has been marked as a duplicate of this bug. ***
Comment 66 nikita.vetoshkin 2012-12-02 19:12:04 UTC
Here's another example: I can't change layout to enter my login password in authentication popup e.g. to authorize package installation.
Comment 67 Sergey Khoroshavin 2012-12-02 19:42:23 UTC
And here's one more example: if I open notification area (Super+M), select contact and start typing I also cannot switch layout. And this is really annoying since there is no "?" symbol in Russian layout, so every time I need to ask a question in my native language I have to type a question (hopefully switching to native layout beforehand), then close notification area, switch layout, then open notification area again and put a question mark in the end. This bug makes too many great features of gnome shell unusable for non-english speaker. Is there any simple way to revert to 3.4 behavior? I'm eager to make a temporary patch, but I don't know where to look first, gnome codebase is quite large.
Comment 68 Sniffy 2012-12-03 09:20:26 UTC
Additional numeric keyboard does not working at 3.6.x.
Comment 69 Sniffy 2012-12-03 09:25:32 UTC
Created attachment 230507 [details]
some video demonstrated a bug
Comment 70 Rui Matos 2012-12-03 22:32:10 UTC
(In reply to comment #67)
> This bug makes too many great features of gnome shell unusable for non-english
> speaker. Is there any simple way to revert to 3.4 behavior? I'm eager to make a
> temporary patch, but I don't know where to look first, gnome codebase is quite
> large.

This bug is being properly fixed for 3.8. If you want a workaround for 3.6 you can use the patch in bug 685676.
Comment 71 Sergey Khoroshavin 2012-12-04 00:02:32 UTC
(In reply to comment #70)
> This bug is being properly fixed for 3.8. If you want a workaround for 3.6 you
> can use the patch in bug 685676.

Thank you very much, I've applied this patch on gnome-settings-daemon and it works for me. Looking forward to proper fix in 3.8.
Comment 72 Rui Matos 2012-12-04 09:57:36 UTC
*** Bug 685815 has been marked as a duplicate of this bug. ***
Comment 73 Florian Müllner 2012-12-05 08:41:08 UTC
Created attachment 230732 [details] [review]
keybindings: Add external grab api
Comment 74 Florian Müllner 2012-12-05 08:41:17 UTC
Created attachment 230733 [details] [review]
keybindings: Add accessor function for keysym
Comment 75 Florian Müllner 2012-12-05 08:41:42 UTC
Created attachment 230734 [details] [review]
shellDBus: Add methods to handle key grabs
Comment 76 Florian Müllner 2012-12-05 08:46:28 UTC
Preliminary (so no proper commit messages yet) patches for mutter/gnome-shell; the gsd side needs more clean-up, but I pushed a shell-keygrab branch that should be functional.
Comment 77 Bastien Nocera 2012-12-05 13:50:53 UTC
For gnome-settings-daemon, I'd like to see:
- XI2 support in gnome-shell, I cannot merge without that
- key grab only for a particular GdkDevice would be nice
- the header listing the different modes supported by gnome-shell be a copy/paste of the gnome-shell one (with documentation)
- gnome-shell is the one doing the captures, but g-s-d still does the filtering and acts upon it, correct? What do we have ensuring that only combinations requested by gnome-settings-daemon can filter through to the root window?
- what happens when gnome-shell crashes and restarts?
- should we remove the multi-screen handling in g-s-d if gnome-shell only supports the first screen?
- there's a number of "application launchers" such as the home key, and the calculator key that could/should work in overview (Alt+F2 works and can launch apps from the overview)
- can we move the hacky hacks to capture ugly modifier-only combinations from the keyboard plugin helper to gnome-shell?

Looks good overall, saves us from moving the processing to gnome-shell, though the problem of overlapping OSDs can remain if some are implemented in gnome-shell (layout switcher for example) and some others in g-s-d.
Comment 78 Florian Müllner 2012-12-05 15:16:44 UTC
(In reply to comment #77)
> For gnome-settings-daemon, I'd like to see:
> - XI2 support in gnome-shell, I cannot merge without that

Jasper is working on that. I've already added the device ID to the d-bus API proposed in comment #41, so the idea is that the XI2 port won't affect g-s-d at all (except that the device id may then contain something other than the core pointer of course).


> - key grab only for a particular GdkDevice would be nice

Sure, we can do that - I didn't do that upfront as g-s-d currently hasn't got that either.


> - the header listing the different modes supported by gnome-shell be a
> copy/paste of the gnome-shell one (with documentation)

Is that a hard requirement? We don't have such a header in gnome-shell (e.g. modes are implemented entirely in javascript), and moving that part into C only to copy it elsewhere seems a bit odd.


> - gnome-shell is the one doing the captures, but g-s-d still does the filtering
> and acts upon it, correct?

Yes? It depends a bit on what you mean by filtering. gnome-shell grabs the key combination and handles it by emitting a d-bus signal (passing keysym, modifiers and device id). So g-s-d is responsible for determining what the key combination is supposed to mean and act upon it, but doesn't do any actual XEvent handling.


> What do we have ensuring that only combinations requested by gnome-settings-daemon can filter through to the root window?

Nothing. Should we do that? (And if yes, how?)


> - what happens when gnome-shell crashes and restarts?

g-s-d regrabs the keys. I'm continuously restarting the shell during development, having that part working wasn't optional from the get-go ;-)


> - should we remove the multi-screen handling in g-s-d if gnome-shell only
> supports the first screen?

That's an option. A fancier option would be to expose the screen gnome-shell uses on the bus and keep the old code path in g-s-d for other screens. (that's why I've only #if 0'ed those parts for now instead of removing the code entirely)


> - there's a number of "application launchers" such as the home key, and the
> calculator key that could/should work in overview (Alt+F2 works and can launch
> apps from the overview)

I agree, which is why the current patch does that (mmh, admittedly I should come up with a better alias for the "normal+overview" mode) :)

Few exceptions apart ("lock screen" not working from the lock screen, "window screenshot" only working in normal mode), keybindings either work in normal mode + overview (mostly the aforementioned launchers) or in all modes (e.g. including system modals, lock/login screen etc., like volume/brightness keys, fullscreen screenshots, ...)


> - can we move the hacky hacks to capture ugly modifier-only combinations from
> the keyboard plugin helper to gnome-shell?

Yes. I've looked a bit into porting it to the d-bus API, but I can't see it working properly with any abstraction, e.g. the API would need to be modified to just expose pure X11 on the bus. So I've agreed with Rui that those will need special handling (either using a dedicated d-bus API, or moving it into mutter/gnome-shell). So the current patch just breaks the im-switcher helper completely.


> Looks good overall, saves us from moving the processing to gnome-shell, though
> the problem of overlapping OSDs can remain if some are implemented in
> gnome-shell (layout switcher for example) and some others in g-s-d.

I'm in favor of moving the actual UI of OSDs into the shell, but haven't really thought about an appropriate API yet (e.g. should we use a general approach where gsd requests an OSD with icon "foo", a visible progress bar at fill "bar" etc., or is it better to expose a hardcoded set of OSDs where OSD requests the volume OSD with level "x" or the brightness OSD at level "y")
Comment 79 Bastien Nocera 2012-12-05 15:42:13 UTC
(In reply to comment #78)
> (In reply to comment #77)
> > For gnome-settings-daemon, I'd like to see:
> > - XI2 support in gnome-shell, I cannot merge without that
> 
> Jasper is working on that. I've already added the device ID to the d-bus API
> proposed in comment #41, so the idea is that the XI2 port won't affect g-s-d at
> all (except that the device id may then contain something other than the core
> pointer of course).

I completely missed the lower section of 619af254c4daad4f6476421f2ce2649c37537f6c.

> > - key grab only for a particular GdkDevice would be nice
> 
> Sure, we can do that - I didn't do that upfront as g-s-d currently hasn't got
> that either.

Support for remotes based on context was in my TODO list.

Can you pass the current mode as well? We could then do things like starting playback in the music player when you press the play button, rather than the video player, even if it was the last application used.

> > - the header listing the different modes supported by gnome-shell be a
> > copy/paste of the gnome-shell one (with documentation)
> 
> Is that a hard requirement? We don't have such a header in gnome-shell (e.g.
> modes are implemented entirely in javascript), and moving that part into C only
> to copy it elsewhere seems a bit odd.

It would be good if they were generated from the same place. I'd really like to avoid them getting out of sync.

> > - gnome-shell is the one doing the captures, but g-s-d still does the filtering
> > and acts upon it, correct?
> 
> Yes? It depends a bit on what you mean by filtering. gnome-shell grabs the key
> combination and handles it by emitting a d-bus signal (passing keysym,
> modifiers and device id). So g-s-d is responsible for determining what the key
> combination is supposed to mean and act upon it, but doesn't do any actual
> XEvent handling.

That missed hunk again.

> 
> > What do we have ensuring that only combinations requested by gnome-settings-daemon can filter through to the root window?
> 
> Nothing. Should we do that? (And if yes, how?)

Does the signal have an explicit destination?

> > - what happens when gnome-shell crashes and restarts?
> 
> g-s-d regrabs the keys. I'm continuously restarting the shell during
> development, having that part working wasn't optional from the get-go ;-)

Cool.

> > - should we remove the multi-screen handling in g-s-d if gnome-shell only
> > supports the first screen?
> 
> That's an option. A fancier option would be to expose the screen gnome-shell
> uses on the bus and keep the old code path in g-s-d for other screens. (that's
> why I've only #if 0'ed those parts for now instead of removing the code
> entirely)

I'd remove the fallback code entirely.

> > - there's a number of "application launchers" such as the home key, and the
> > calculator key that could/should work in overview (Alt+F2 works and can launch
> > apps from the overview)
> 
> I agree, which is why the current patch does that (mmh, admittedly I should
> come up with a better alias for the "normal+overview" mode) :)

I must have misread it.

> Few exceptions apart ("lock screen" not working from the lock screen, "window
> screenshot" only working in normal mode), keybindings either work in normal
> mode + overview (mostly the aforementioned launchers) or in all modes (e.g.
> including system modals, lock/login screen etc., like volume/brightness keys,
> fullscreen screenshots, ...)

OK.

> > - can we move the hacky hacks to capture ugly modifier-only combinations from
> > the keyboard plugin helper to gnome-shell?
> 
> Yes. I've looked a bit into porting it to the d-bus API, but I can't see it
> working properly with any abstraction, e.g. the API would need to be modified
> to just expose pure X11 on the bus. So I've agreed with Rui that those will
> need special handling (either using a dedicated d-bus API, or moving it into
> mutter/gnome-shell). So the current patch just breaks the im-switcher helper
> completely.

Ad-hoc API might have to do then.

> > Looks good overall, saves us from moving the processing to gnome-shell, though
> > the problem of overlapping OSDs can remain if some are implemented in
> > gnome-shell (layout switcher for example) and some others in g-s-d.
> 
> I'm in favor of moving the actual UI of OSDs into the shell, but haven't really
> thought about an appropriate API yet (e.g. should we use a general approach
> where gsd requests an OSD with icon "foo", a visible progress bar at fill "bar"
> etc., or is it better to expose a hardcoded set of OSDs where OSD requests the
> volume OSD with level "x" or the brightness OSD at level "y")

Most of them are as simple as the above, though we'd probably want to have "text" as well (bug 675901). There's also the problem of alt-tab like OSDs (for the input source switching, and video output as per bug 600774).
Comment 80 Jasper St. Pierre (not reading bugmail) 2012-12-05 16:48:02 UTC
(In reply to comment #77)
> For gnome-settings-daemon, I'd like to see:
> - XI2 support in gnome-shell, I cannot merge without that

Note that while this will happen, it won't care about multidevice. Owen feels that supporting the multi-pointer or multi-keyboard use cases introduces complexity for no big gain. Note that we have other APIs that do things like pop up modal dialogs, without a device_id argument.

I still need to read the rest of the feedback now.
Comment 81 Jasper St. Pierre (not reading bugmail) 2012-12-05 16:58:03 UTC
(In reply to comment #78)
> Yes. I've looked a bit into porting it to the d-bus API, but I can't see it
> working properly with any abstraction, e.g. the API would need to be modified
> to just expose pure X11 on the bus. So I've agreed with Rui that those will
> need special handling (either using a dedicated d-bus API, or moving it into
> mutter/gnome-shell). So the current patch just breaks the im-switcher helper
> completely.
> 

Is there anything wrong with just passing the binding string over the bus, i.e. "<Ctrl><Shift>"? This also lets us use "Above_Tab" in external bindings.
Comment 82 Bastien Nocera 2012-12-05 16:59:31 UTC
(In reply to comment #80)
> (In reply to comment #77)
> > For gnome-settings-daemon, I'd like to see:
> > - XI2 support in gnome-shell, I cannot merge without that
> 
> Note that while this will happen, it won't care about multidevice. Owen feels
> that supporting the multi-pointer or multi-keyboard use cases introduces
> complexity for no big gain.

We just need to know which device the event came from.
Comment 83 Jasper St. Pierre (not reading bugmail) 2012-12-05 17:02:19 UTC
(In reply to comment #78)
> I'm in favor of moving the actual UI of OSDs into the shell, but haven't really
> thought about an appropriate API yet (e.g. should we use a general approach
> where gsd requests an OSD with icon "foo", a visible progress bar at fill "bar"
> etc., or is it better to expose a hardcoded set of OSDs where OSD requests the
> volume OSD with level "x" or the brightness OSD at level "y")

As I've said elsewhere, I'm really opposed to having the shell becoming the new gnome-settings-daemon. Throwing everything in the compositor process is going to make it slower, and we have to start being a lot more careful.

What's the advantage of having it in the Shell instead of an ARGB32 window? Having it in the overview? Why not just add a "_GNOME_INTERNAL" property on the window to mark it as "should appear in the overview"?


(In reply to comment #82)
> We just need to know which device the event came from.

OK, we should be able to get you that.
Comment 84 Florian Müllner 2012-12-05 17:55:04 UTC
(In reply to comment #83)
> As I've said elsewhere, I'm really opposed to having the shell becoming the new
> gnome-settings-daemon. Throwing everything in the compositor process is going
> to make it slower, and we have to start being a lot more careful.

I share your concern about accumulating random stuff in the shell, but I don't have a problem with any "system ui", including OSDs.


> What's the advantage of having it in the Shell instead of an ARGB32 window?
> Having it in the overview? Why not just add a "_GNOME_INTERNAL" property on the
> window to mark it as "should appear in the overview"?

The advantage of not adding a second extension mechanism. You can call the property _DONT_USE_IT_OR_KITTENS_WILL_DIE all you want, stuff like cairo-dock, maliit etc. will use it anyway.
Comment 85 Florian Müllner 2012-12-14 09:02:58 UTC
Created attachment 231558 [details] [review]
keybindings: Add external grab API

Port to XI2
Comment 86 Jasper St. Pierre (not reading bugmail) 2012-12-14 09:28:17 UTC
Review of attachment 231558 [details] [review]:

::: src/core/keybindings.c
@@ +162,3 @@
+struct _MetaKeyGrab {
+  char *name;
+  MetaKeyCombo *combo;

Any reason this is separately allocated rather than just being inline with the struct?

@@ +510,3 @@
                          &display->key_bindings,
                          &display->n_key_bindings,
+                         prefs, grabs);

Why not pass the hash table in, and use a GHashTableIter, so you don't have to allocate a list?

@@ +4418,3 @@
+  handler->default_func = handle_external_grab;
+
+  g_hash_table_insert (key_handlers, g_strdup ("external-grab"), handler);

What frees this key?
Comment 87 Jasper St. Pierre (not reading bugmail) 2012-12-14 09:28:35 UTC
Review of attachment 230733 [details] [review]:

OK
Comment 88 Jasper St. Pierre (not reading bugmail) 2012-12-14 09:36:29 UTC
Review of attachment 230734 [details] [review]:

Seems a bit bad that all clients listening get to snoop in on another's key grab, but I don't have any other ideas beyond setting up a new object for each client, and that's complicated.

::: js/ui/shellDBus.js
@@ +51,3 @@
+<method name="GrabKey">
+    <arg type="u" direction="in" name="keysym"/>
+    <arg type="u" direction="in" name="modifiers"/>

As said in the bug, I wonder if we should use a modifier string which we parse.

@@ +57,3 @@
+<method name="UngrabKey">
+    <arg type="u" direction="in" name="keysym"/>
+    <arg type="u" direction="in" name="modifiers"/>

I think we should return a unique integer from GrabKey that clients use to unregister. That would prevent the silly case of:

  Client A GrabKey <Super>S
  Client B GrabKey <Super>S
  Client A UngrabKey <Super>S

  Client B is left in the dirt.

@@ +87,3 @@
 </interface>;
 
+function _metaToGdkModifier(mods) {

ow :(

Can we make the two enums the same, and just have a comment here saying why that works?
Comment 89 Florian Müllner 2012-12-14 10:17:12 UTC
(In reply to comment #88)
> Seems a bit bad that all clients listening get to snoop in on another's key
> grab, but I don't have any other ideas beyond setting up a new object for each
> client, and that's complicated.

I agree on the problem, but haven't come up with a nice solution so far.


> ::: js/ui/shellDBus.js
> @@ +51,3 @@
> +<method name="GrabKey">
> +    <arg type="u" direction="in" name="keysym"/>
> +    <arg type="u" direction="in" name="modifiers"/>
> 
> As said in the bug, I wonder if we should use a modifier string which we parse.

I see three options:
 (1) Expose keysym/mods as direct XGrabKey() replacement
 (2) Expose (gtk-compatible) accelerator string
 (3) Expose GSettings schema/path/key (and support string
     settings in addition to string arrays)

(1) is the least intrusive on the g-s-d side and (3) would be my preferred approach for moving the media-keys plugin into mutter/gnome-shell. (2) is a bit intermediate and relies on mutter/gsd using the same accelerator format (which in practice is of course the case). I don't case much about (1) or (2), so as the impact is on g-s-d, I'd leave the decision to Bastien.


> @@ +57,3 @@
> +<method name="UngrabKey">
> +    <arg type="u" direction="in" name="keysym"/>
> +    <arg type="u" direction="in" name="modifiers"/>
> 
> I think we should return a unique integer from GrabKey that clients use to
> unregister.

Sure, we can do that.


> @@ +87,3 @@
>  </interface>;
> 
> +function _metaToGdkModifier(mods) {
> 
> ow :(
> 
> Can we make the two enums the same, and just have a comment here saying why
> that works?

The difference is intentional[0], but if we want to ignore that, I'd prefer to kill MetaVirtualModifier altogether and use either GdkModifierType or ClutterModifierType.

[0] http://git.gnome.org/browse/mutter/tree/src/meta/common.h#n190
Comment 90 Florian Müllner 2012-12-14 10:29:54 UTC
(In reply to comment #86)
> Review of attachment 231558 [details] [review]:
> Any reason this is separately allocated rather than just being inline with the
> struct?

I don't understand the question - do you mean why MetaKeyGrab is a separate struct rather than having "name" in MetaKeyCombo?


> @@ +510,3 @@
> Why not pass the hash table in, and use a GHashTableIter, so you don't have to
> allocate a list?

For symmetry with prefs - of course we could change that as well.


> @@ +4418,3 @@
> +  handler->default_func = handle_external_grab;
> +
> +  g_hash_table_insert (key_handlers, g_strdup ("external-grab"), handler);
> 
> What frees this key?

Nothing. We could allocate the key when the first key combo is grabbed and free it after the last one is released, but I don't think it's really worth it (on a side note, the same thinking has been applied to the "overlay-key" key - we just always allocate that handler as well, independent from whether the setting is actually in use or not)
Comment 91 Bastien Nocera 2012-12-14 10:38:58 UTC
(In reply to comment #89)
> > ::: js/ui/shellDBus.js
> > @@ +51,3 @@
> > +<method name="GrabKey">
> > +    <arg type="u" direction="in" name="keysym"/>
> > +    <arg type="u" direction="in" name="modifiers"/>
> > 
> > As said in the bug, I wonder if we should use a modifier string which we parse.
> 
> I see three options:
>  (1) Expose keysym/mods as direct XGrabKey() replacement
>  (2) Expose (gtk-compatible) accelerator string
>  (3) Expose GSettings schema/path/key (and support string
>      settings in addition to string arrays)
> 
> (1) is the least intrusive on the g-s-d side and (3) would be my preferred
> approach for moving the media-keys plugin into mutter/gnome-shell. (2) is a bit
> intermediate and relies on mutter/gsd using the same accelerator format (which
> in practice is of course the case). I don't case much about (1) or (2), so as
> the impact is on g-s-d, I'd leave the decision to Bastien.

I'd rather 2), as the accelerator format isn't exactly the same :)
That means we could try and support <AboveTab> for example, or modifier-only shortcuts (for things like layout switching, is that useful Rui?).
Comment 92 Florian Müllner 2012-12-14 11:13:05 UTC
Created attachment 231562 [details] [review]
Kill MetaVirtualModifiers in favor of GdkModifierType

We already depend on Gdk and Clutter which each provide their own
modifier enums. There's little benefit in maintaining a third one,
so use GdkModifierType directly instead.


For what it's worth, this is how s/MetaVirtualModifier/GdkModifierType/ looks like :-)
Comment 93 Florian Müllner 2012-12-14 11:13:34 UTC
Created attachment 231563 [details] [review]
windowManager: Adjust to removal of Meta.VirtualModifier
Comment 94 Florian Müllner 2012-12-14 12:24:35 UTC
(In reply to comment #91)
> I'd rather 2), as the accelerator format isn't exactly the same :)

Heh - potentially breaking user settings is now an argument in favor? ;-)


> That means we could try and support <AboveTab> for example,

Yes.


> or modifier-only shortcuts (for things like layout switching, is that useful Rui?).

The current plan is to handle those separately, though it should be possible with some effort to make it work (first mutter would need to gain some custom parsing code for modifier-only accels, and then do a bunch of grab/ungrab/allow hacks before emitting the KeyActivated signal). Rui?
Comment 95 Bastien Nocera 2012-12-14 14:19:40 UTC
(In reply to comment #94)
> (In reply to comment #91)
> > I'd rather 2), as the accelerator format isn't exactly the same :)
> 
> Heh - potentially breaking user settings is now an argument in favor? ;-)

Things that won't work anymore: keycode-only keybindings
Things that will start working: AboveTab

I'm fine with that trade-off.
Comment 96 Rui Matos 2012-12-14 15:44:58 UTC
(In reply to comment #94)
> > or modifier-only shortcuts (for things like layout switching, is that useful Rui?).
> 
> The current plan is to handle those separately, though it should be possible
> with some effort to make it work (first mutter would need to gain some custom
> parsing code for modifier-only accels, and then do a bunch of grab/ungrab/allow
> hacks before emitting the KeyActivated signal). Rui?

I'd like to keep all input source switching in the shell process for the next cycle even for modifiers only keybindings. So, as far as I'm concerned, you don't need to support that in the DBus API.
Comment 97 Florian Müllner 2012-12-14 18:09:08 UTC
(In reply to comment #95)
> I'm fine with that trade-off.

I got that :-)

The mutter/shell parts are ready, the g-s-d side will take the weekend (today is a shitty day :-( )



(In reply to comment #96)
> I'd like to keep all input source switching in the shell process for the next
> cycle even for modifiers only keybindings. So, as far as I'm concerned, you
> don't need to support that in the DBus API.

Cool, you won't hear me complaining :-)

With the DBus API change, the least intrusive approach on the g-s-d side is to change the media-keys plugin to use the GDBusProxy directly, so gsd-keygrab won't be modified at all, e.g. the input source switcher won't be affected.
Comment 98 Jasper St. Pierre (not reading bugmail) 2012-12-14 18:10:43 UTC
Review of attachment 231562 [details] [review]:

So, the thing is that "core" is not supposed to include Gtk or Gdk at all, which was the original reason for MetaVirtualModifier. We first broke that with invisible borders with "GtkBorder", and opened up the floodgates for this. I'd like for Owen to pass this conceptual review. For a less intensive change, you could typedef.
Comment 99 Florian Müllner 2012-12-14 23:57:03 UTC
(In reply to comment #98)
> Review of attachment 231562 [details] [review]:
> 
> So, the thing is that "core" is not supposed to include Gtk or Gdk at all,
> which was the original reason for MetaVirtualModifier.

I'm aware of this, but a quick grep showed that nowadays we are using both Gdk and Clutter symbols all over the place, so I think the change is justified. No strong sentiment though, so I'm fine with leaving the final decision to Owen ...
Comment 100 Jasper St. Pierre (not reading bugmail) 2012-12-23 08:16:22 UTC
(In reply to comment #99)
> I'm aware of this, but a quick grep showed that nowadays we are using both Gdk
> and Clutter symbols all over the place, so I think the change is justified. No
> strong sentiment though, so I'm fine with leaving the final decision to Owen
> ...

Where? I just did the same grep, but couldn't find any Gdk symbols being used in core. Gtk came up with GtkBorder in meta/common.h, but that's it. Clutter shows the existence of a timeline to keep laters working correctly, and the event dispatch in main.c

Is there anything wrong with a simple copy/paste or modification of meta_display_devirtualize_modifier that returns a GdkModifier to make introspection happy?
Comment 101 Florian Müllner 2012-12-23 08:29:15 UTC
(In reply to comment #100)
> Where? I just did the same grep, but couldn't find any Gdk symbols being used
> in core.

There are various gdk_error_trap_(push/pop)s and gdk_display_get_default() in main.c.

In any case the API will be changed to use an accelerator string instead of keysym/modifiers, so the original reason for the patch is gone anyway.
Comment 102 Giovanni Campagna 2013-01-12 14:56:26 UTC
*** Bug 691594 has been marked as a duplicate of this bug. ***
Comment 103 Bastien Nocera 2013-01-12 22:23:26 UTC
*** Bug 685318 has been marked as a duplicate of this bug. ***
Comment 104 Bastien Nocera 2013-01-12 22:49:52 UTC
*** Bug 687068 has been marked as a duplicate of this bug. ***
Comment 105 Giovanni Campagna 2013-01-19 14:43:13 UTC
*** Bug 692057 has been marked as a duplicate of this bug. ***
Comment 106 Giovanni Campagna 2013-01-20 13:33:38 UTC
*** Bug 692132 has been marked as a duplicate of this bug. ***
Comment 107 Giovanni Campagna 2013-01-20 15:13:57 UTC
*** Bug 692140 has been marked as a duplicate of this bug. ***
Comment 108 Rui Matos 2013-01-23 08:58:17 UTC
*** Bug 692357 has been marked as a duplicate of this bug. ***
Comment 109 Giovanni Campagna 2013-01-24 20:57:52 UTC
*** Bug 692441 has been marked as a duplicate of this bug. ***
Comment 110 Cheng-Chia Tseng 2013-02-01 14:23:43 UTC
This issue also influence input method integration. For example, if you switch to non-English input method, and you lock your screen then you cannot switch back to English via the shortcut key you have set up. 

The only way to switch the input method is to click on the input method icon on the top bar of the screen. It is so INCONVENIENT for those non-English users. The introduction of input method integration work is just a mess now. Hope it can be fixed in next release of GNOME.
Comment 111 Florian Müllner 2013-02-01 14:29:45 UTC
Yes, the plan is to fix this with the next version (note that modifier-only IM switching will be fixed separately, e.g. not by the patches in this bug - see bug 689839 for that)
Comment 112 Florian Müllner 2013-02-01 15:12:57 UTC
(In reply to comment #86)
> Review of attachment 231558 [details] [review]:
> 
> ::: src/core/keybindings.c
> @@ +162,3 @@
> +struct _MetaKeyGrab {
> +  char *name;
> +  MetaKeyCombo *combo;
> 
> Any reason this is separately allocated rather than just being inline with the
> struct?

Symmetry with MetaKeyPref (MetaKeyCombo is the parsed accelerator, MetaKeyPref is a keybinding backed by GSettings, and MetaKeyGrab an externally grabbed accelerator).


> @@ +510,3 @@
>                           &display->key_bindings,
>                           &display->n_key_bindings,
> +                         prefs, grabs);
> 
> Why not pass the hash table in, and use a GHashTableIter, so you don't have to
> allocate a list?

Symmetry again. (If we don't care, we don't need to pass the hash table around at all, as it's a static variable in keybindings.c).
I've left it as that for now, it seems like a minor optimization (a much bigger gain would be to add some additional freeze_keybindings()/thaw_keybindings() API to allow g-s-d to batch grabbing)
Comment 113 Florian Müllner 2013-02-01 15:13:22 UTC
Created attachment 234993 [details] [review]
keybindings: Generalize mechanism to generate dynamic keybinding actions
Comment 114 Florian Müllner 2013-02-01 15:13:30 UTC
Created attachment 234994 [details] [review]
keybindings: Add external grab API

During compositor grabs, all global keybindings that don't go
through mutter's keybinding system are blocked. To allow other
processes to make use of it, gnome-shell will expose a simple
grab API on DBus; for this, add API to grab key combos directly
instead of parsing accelerators stored in GSettings.
Comment 115 Florian Müllner 2013-02-01 15:13:37 UTC
Created attachment 234995 [details] [review]
prefs: Use an unsigned value for META_KEYBINDING_ACTION_NONE

Keybinding actions are unsigned, so it is a tad bit odd to use a
negative value for NONE and rely on implicit casting.
Use 0 nstead.
Comment 116 Florian Müllner 2013-02-01 15:14:04 UTC
Created attachment 234996 [details] [review]
shellDBus: Add methods to handle key grabs

Expose Mutter's external grab API on the bus, so gnome-settings-daemon
can refer keygrabbing to the shell.
Comment 117 Florian Müllner 2013-02-01 15:14:12 UTC
Created attachment 234997 [details] [review]
main: Move KeybindingMode into Shell

Having the definition in C instead of Javascript allows sharing
the corresponding header with gnome-settings-daemon.
Comment 118 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:29:02 UTC
Review of attachment 234997 [details] [review]:

I'll trust that you did the mechanical translation correctly, and that it's "KeyBinding" everywhere, not "Keybinding"
Comment 119 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:31:21 UTC
Review of attachment 234993 [details] [review]:

Style nit.

::: src/core/keybindings.c
@@ +546,3 @@
+next_dynamic_keybinding_action () {
+  static guint num_dynamic_bindings = 0;
+  return META_KEYBINDING_ACTION_LAST + ++num_dynamic_bindings;

+ (++num_dynamic_bindings)

the bare "+ ++" looks weird
Comment 120 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:38:24 UTC
Review of attachment 234995 [details] [review]:

"nstead" in commit message
Comment 121 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:48:06 UTC
Review of attachment 234994 [details] [review]:

So, I'm fine with this patch, but why do we have to go through the binding system that's a bit awkward to fit in here? Can't we iterate over the grabs in grab_keys/ungrab_keys, and handle it specially in process_event?

::: src/core/keybindings.c
@@ +852,3 @@
+  guint keysym = 0;
+  guint keycode = 0;
+  guint action = META_KEYBINDING_ACTION_NONE;

Replace with just returning META_KEYBINDING_ACTION_NONE in the branch below.
Comment 122 Jasper St. Pierre (not reading bugmail) 2013-02-01 18:49:56 UTC
Review of attachment 234996 [details] [review]:

::: js/ui/shellDBus.js
@@ +108,3 @@
+        let bindingAction = global.display.grab_accelerator(accelerator);
+        if (bindingAction != Meta.KeyBindingAction.NONE)
+            Main.wm.allowKeybinding('external-grab-' + bindingAction, flags);

Eesh. I wonder if we should we should expose the name for a binding action from mutter.
Comment 123 Florian Müllner 2013-02-02 10:57:19 UTC
(In reply to comment #121)
> So, I'm fine with this patch, but why do we have to go through the binding
> system that's a bit awkward to fit in here? Can't we iterate over the grabs in
> grab_keys/ungrab_keys, and handle it specially in process_event?

What exactly do you think is awkward about using the existing binding system? Not doing that would require changing the keybinding filtering, as that's currently based on MetaKeyBinding ...
Comment 124 Florian Müllner 2013-02-02 10:59:38 UTC
(In reply to comment #122)
> Eesh. I wonder if we should we should expose the name for a binding action from
> mutter.

Yeah, I was considering passing the binding instead of the action, but it's odd to use something else in the signal than in the grab()/ungrab() functions. I'll just add some get_binding_name_for_action() function I guess ...
Comment 125 Jasper St. Pierre (not reading bugmail) 2013-02-02 12:09:12 UTC
(In reply to comment #123)
> What exactly do you think is awkward about using the existing binding system?
> Not doing that would require changing the keybinding filtering, as that's
> currently based on MetaKeyBinding ...

It seems like it's a bit of a square peg in a round hole to have a lot of bindings that have the same handler.
Comment 126 Florian Müllner 2013-02-02 13:12:50 UTC
(In reply to comment #125)
> It seems like it's a bit of a square peg in a round hole to have a lot of
> bindings that have the same handler.

I don't agree with that - the bindings share the same handler, but the result is different for each binding (e.g. the "action" parameter of the emitted signal differs). I don't see a big conceptual difference to existing bindings that share a common handler ({move,switch}-workspace-n, {cycle,switch}-{applications,group,panels,windows}{,-backward}, ...)
Comment 127 Bastien Nocera 2013-02-03 00:35:28 UTC
Review of attachment 234996 [details] [review]:

::: js/ui/shellDBus.js
@@ +68,3 @@
+            function(display, action, deviceid) {
+                this._dbusImpl.emit_signal('AcceleratorActivated',
+                                           GLib.Variant.new('(uu)', [action, deviceid]));

This needs to go only to the application that asked for the grab, not to everywhere. g-s-d uses a well known name, so it crashing wouldn't change the destination.

@@ +105,3 @@
     },
 
+    GrabAccelerator: function(accelerator, flags) {

I would add a GrabAccelerators() helpers to be used on login. 50 odd d-bus calls isn't too great for startup time.
Comment 128 Florian Müllner 2013-02-04 16:04:53 UTC
Comment on attachment 234997 [details] [review]
main: Move KeybindingMode into Shell

I'll update the patches some time post-3.7.5, but let's include the Main.KeybindingMode => Shell.KeyBindingMode API break in the release.

Attachment 234997 [details] pushed as b682c8e - main: Move KeybindingMode into Shell
Comment 129 Jasper St. Pierre (not reading bugmail) 2013-02-07 06:31:59 UTC
Review of attachment 234994 [details] [review]:

OK.
Comment 130 sangu 2013-02-17 05:53:21 UTC
*** Bug 693908 has been marked as a duplicate of this bug. ***
Comment 131 Florian Müllner 2013-03-01 03:11:08 UTC
Created attachment 237674 [details] [review]
keybindings: Add external grab API

- don't allow grabbing the same accelerator multiple times
 - (un)grab a single key in (un)grab_accelerator() rather than
   regrabbing all keybindings
Comment 132 Florian Müllner 2013-03-01 03:16:21 UTC
Created attachment 237676 [details] [review]
shellDBus: Add methods to handle key grabs

(In reply to comment #122)
> Review of attachment 234996 [details] [review]:
> 
> Eesh. I wonder if we should we should expose the name for a binding action from
> mutter.

Done.


(In reply to comment #127)
> Review of attachment 234996 [details] [review]:
> +            function(display, action, deviceid) {
> +                this._dbusImpl.emit_signal('AcceleratorActivated',
> +                                           GLib.Variant.new('(uu)', [action,
> deviceid]));
> 
> This needs to go only to the application that asked for the grab, not to
> everywhere. g-s-d uses a well known name, so it crashing wouldn't change the
> destination.

Done, but not using the well known name - instead, ungrab accelerators automatically if the grabber goes away.


> @@ +105,3 @@
> I would add a GrabAccelerators() helpers to be used on login. 50 odd d-bus
> calls isn't too great for startup time.

Good idea.
Comment 133 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:24:22 UTC
Review of attachment 237674 [details] [review]:

I'm fine with this, but I'd like to land the refactoring so we don't ungrab and regrab all keys on every single modification to keypresses -- that way, a GrabAccelerators won't cause too much harm.
Comment 134 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:26:09 UTC
Review of attachment 237676 [details] [review]:

OK.
Comment 135 Florian Müllner 2013-03-01 04:28:08 UTC
(In reply to comment #133)
> I'm fine with this, but I'd like to land the refactoring so we don't ungrab and
> regrab all keys on every single modification to keypresses -- that way, a
> GrabAccelerators won't cause too much harm.

That's exactly what the patch does for external bindings (and any other bindings are unaffected by GrabAccelerators)
Comment 136 Jasper St. Pierre (not reading bugmail) 2013-03-01 04:31:00 UTC
(In reply to comment #135)
> (In reply to comment #133)
> > I'm fine with this, but I'd like to land the refactoring so we don't ungrab and
> > regrab all keys on every single modification to keypresses -- that way, a
> > GrabAccelerators won't cause too much harm.
> 
> That's exactly what the patch does for external bindings (and any other
> bindings are unaffected by GrabAccelerators)

Ah, didn't notice the meta_change_keygrab in there. Would be nice to do it for other keybindings, but not crucial.
Comment 137 Florian Müllner 2013-03-01 15:55:20 UTC
Attachment 234993 [details] pushed as 4df3e98 - keybindings: Generalize mechanism to generate dynamic keybinding actions
Attachment 234995 [details] pushed as 7f14298 - prefs: Use an unsigned value for META_KEYBINDING_ACTION_NONE
Attachment 237674 [details] pushed as a39cabf - keybindings: Add external grab API


g-s-d differentiates between "repeatable" and "non-repeatable" shortcuts (where the actual difference is that the former trigger on KeyPress and the latter on KeyRelease) which we don't do. It might make sense to support something like that in the future, but as we basically treat all keys as repeatable (and thus won't break keeping volume/brightness pressed), we can leave that as a possible future improvement.

(In reply to comment #136)
> Ah, didn't notice the meta_change_keygrab in there. Would be nice to do it for
> other keybindings, but not crucial.

Yeah, nobody has considered this serious enough in 10 years, so it really doesn't make sense to block on it now :-)
Comment 138 Florian Müllner 2013-03-01 15:59:52 UTC
Attachment 237676 [details] pushed as 81e01b6 - shellDBus: Add methods to handle key grabs

(Before everyone rejoices: the g-s-d patches in bug 693016 need to land before anything here becomes actually useful, and modifier-only input source switching is not in the scope of this bug, that's bug 689839 (which is still scheduled for 3.8 as well though)
Comment 139 Bastien Nocera 2014-11-09 17:23:15 UTC
*** Bug 693465 has been marked as a duplicate of this bug. ***