GNOME Bugzilla – Bug 590754
Need more flexible and robust handling for keyboard events
Last modified: 2014-12-29 02:43:32 UTC
It's useful for plugin code in some situations to ensure that *all* key events are passed through to the plugin, avoiding mutter-internal keyboard event processing, such as Alt-TAB.
Created attachment 139878 [details] [review] Add meta_display_set_key_event_passthrough
I think this would be fine if you call this once on start up, but I am bit weary of allowing this to be toggled on an off at run time. Say, could this ever be called why mutter has a gbd grab (if so, the user would probably loose the use of their keyboard and have to pull the plug). Also, for stuff like Alt+Tab, you will need to do keyboard grab in the plugin -- is it not possible to just install a custom handler function for the Alt+Tab combination, and allow Mutter to take care of the grabs?
There is need for more generic and robust API for handling keyboard events in Mutter; the following is Owen's comment from http://bugzilla.gnome.org/show_bug.cgi?id=590686#c9 that sums up the main issues: A) When the all_keys_grabbed path in meta_display_process_key_event() is called, it should never be calling process_event() in addition. It should also not be passing these events to the plugin. No good can come of the plugin getting these key events. B) There should be a way of a plugin to trigger a global key grab and sending events *only* to *that* plugin until the keyboard is ungrabbed. (Sort of like meta_display_begin_grab_op(), but passing in a plugin rather than a window/op pair.) C) There should be a way of letting a plugin register additional global key bindings - I don't think plugins should be in the XGrabKey business - even if we get rid of the ungrabbing hanky-panky. --end of quote -- An additional question, in addition to B) do we also want D) a way for plugin to receive all mutter keyboard events exclusively when there is no formal X keyboard grab? (e.g., along the lines of the original patch here). I suspect this would be superfluous, between B), C) and the ability to install custom handlers for the standard meta bindings we can handle pretty much any scenario.
Created attachment 140519 [details] [review] Enforce a policy of single-handling of key events Only process each key event once. If all keys are grabbed, then don't also look for handlers for a key shortcut after processing the grab op. If all keys are grabbed or we find a key shortcut, don't pass the event on to the compositing mananger.
Created attachment 140520 [details] [review] Add a modal mode for plugins mutter_plugin_begin_modal() and mutter_plugin_begin_modal() allow putting a plugin into a "modal" state. This means: - The plugin has the keyboard and mouse grabbed - All keyboard and mouse events go exclusively to the plugin mutter-plugin.[ch]: Add public API compositor.c compositor-private.h: Implement the API mutter-plugin-manager.c: When reloading plugins, make sure none of them are modal at that moment, and if so force-unmodal them. common.h: Add META_GRAB_OP_COMPOSITOR display: When display->grab_op is META_GRAB_OP_COMPOSITOR forward relevant events exclusively to the compositor.
The two attached patches implement steps A and B of my program. Not sure how well the API I've proposed meet other requirements for global grabs, say in mutter-moblin. I made a single API that grabs pointer and keyboard because (as described in the doc comment) things have the potential to go weird if one is grabbed and not the other. [or least it's difficult to analyze.] I'm not sure how much of a problem this is in practice - we could explore relaxing it if there are requirements for keyboard-only grabs. In mutter_compositor_begin_modal_for_plugin()/mutter_compositor_end_modal_for_plugin(), I didn't replicate the practice of removing the passive key grabs from the screen, because (as I discussed in bug 590686), I don't think it has any point. Seems to work fine without.
Tomas: any comment on these patches?
Sorry for the delay; there is problem with having a custom alt_tab handler installed with the first of these patches applied. Basically, the comments in keybindings.c: at lines 2046 and 2054 no longer hold true: /* * This is the case when the Alt key is released; we preserve * the grab, as it is up to the custom implementaiton to free it * (a plugin can catch this in their xevent_filter function). */ /* * We do not want to actually call the handler, we just want to ensure * that if a custom handler is installed, we do not release the grab here. * The handler will get called as normal in the process_event() function. */ I am attaching a patch that fixes this problem, though I am not entirely sure it's the optimal solution.
Created attachment 141320 [details] [review] Patch to fix event processing when custom alt+tab handler is present.
In reply to comment #8) > Sorry for the delay; there is problem with having a custom alt_tab handler > installed with the first of these patches applied. Basically, the comments in > keybindings.c: at lines 2046 and 2054 no longer hold true: > > /* > * This is the case when the Alt key is released; we preserve > * the grab, as it is up to the custom implementaiton to free it > * (a plugin can catch this in their xevent_filter function). > */ > > /* > * We do not want to actually call the handler, we just want to ensure > * that if a custom handler is installed, we do not release the grab > here. > * The handler will get called as normal in the process_event() function. > */ > > I am attaching a patch that fixes this problem, though I am not entirely sure > it's the optimal solution. This is certainly a bit of a messy area. As I read your patch: - It will cause the grab to be released on any key release, not just the key release of the primary modifier. Maybe I'm misreading it since that would seem obvious in testing. - If the plugin registered only a partial set of handlers for the "Alt-Tab" actions, then a mix of the plugins handlers and the default handlers will get called, which seems like it could cause issues. There's also an existing problem which is: - The lookup for display_get_keybinding_action uses the grab mask, while the process_event code will use the mask from the keypress. This means (With normal Metacity/Mutter handling is the action from get_keybinding_action(), so you can't start of in Alt-Tab then switch to Control-Alt-Tab) There are overall things I'm not too happy about with this mechanism - in particular I don't like either listening to the ::keyboard-grabbed notification or listening for KeyRelease as the way to end the grab, but I think I can come up with a patch that gets back to the existing status quo and maybe a bit better.
Created attachment 141697 [details] [review] Fix custom-alt-tabs for single-handling of key events The changes to enforce single handling of all key events were breaking custom-alt-tab keypress handlers, since that code was assuming that key event would get to process_tab_grab(), and then maybe to process_event() and then to the plugin's xevent_filter to detect a key release. We centeralize all of this handling into process_tab_grab() and either - Invoke a custom handler for the key press - Select the current window on modifier release by calling a new pseudo-binding "tab_popup_select" - Cancel the grab on an unbound key by calling a new pseudo-binding "tab_popup_cancel"
The last attached patch is my attempt at coming up with a solution. I haven't tested the modfiied code path at all, since I don't have a plugin that uses the mechanism. There is an incompatibility since to distinguish "select" vs. "cancel", there's a change: "Listen for key release" => install tab_popup_select handler "Listen for notify::keyboard-grabbed" => install tab_popup_cancel handler (You could still listen for notify::keyboard-grabbed, but it's cleaner not to.) With your patch there is actually no way I can see to distinguish the two cases, so this probably a bit better than that.
Yes, looks like a nicer solution in principle; I will get back to you once I get a chance to test it.
Created attachment 141761 [details] [review] Check for NULL rather than assert in meta_screen_tab_popup_destroy() Works well; I am attaching an additional small patch to remove an incorrect (pre-existing) assert in meta_screen_tab_popup_destroy(), otherwise happy for this to be committed.
Comment on attachment 141761 [details] [review] Check for NULL rather than assert in meta_screen_tab_popup_destroy() Looks good to me
Comment on attachment 139878 [details] [review] Add meta_display_set_key_event_passthrough Am I correct assuming the original patch is now obsolete ? If so, we can close this bug.
Marking the original patch as rejected. Element C) above: > C) There should be a way of letting a plugin register additional global key > bindings - I don't think plugins should be in the XGrabKey business - even if > we get rid of the ungrabbing hanky-panky. Is still outstanding - I think I'll leave this open for now since I'm planning to work on something for that soon, though we could also close this bug and file a new one.
(In reply to comment #17) > Marking the original patch as rejected. > > Element C) above: > > > C) There should be a way of letting a plugin register additional global key > > bindings - I don't think plugins should be in the XGrabKey business - even if > > we get rid of the ungrabbing hanky-panky. > > Is still outstanding - I think I'll leave this open for now since I'm planning > to work on something for that soon, though we could also close this bug and > file a new one. I assume 5 years is a tad past "soon". I've written a few solutions in the meantime to try to escape the XGrabKey hell.