GNOME Bugzilla – Bug 688202
Introduce a more fine-grained approach to keybindings
Last modified: 2012-11-17 01:12:55 UTC
This is an alternative to the patch set in bug 613543 to get rid of the infamous global key-press HACK in main.js. On top of that, it introduces the concept of a "keybinding mode", which allows to selectively enable keybindings for different elements (overview, login screen, message tray, ...).
Created attachment 228808 [details] [review] Add hook for selectively processing keybindings during compositor grabs Currently keybindings are blocked while the compositor holds a grab; if we want a keybinding to be available anyway, we use captured ClutterEvents to determine the KeyBindingAction the event would have triggered and run our own handlers (ugh). Instead, provide a hook to selectively allow normal processing of bindings during compositor grabs.
Created attachment 228809 [details] [review] keybindings: Add MetaKeyBinding for overlay-key As the overlay key works differently from normal keybindings, it requires special treatment. However, by adding a rudimentary MetaKeyBinding for it, we will be able to confine the special handling to mutter and treat it like any other keybinding in the shell.
Created attachment 228810 [details] [review] windowManager: Implement keybinding_filter hook We are currently using a hack to allow a select set of keybindings in the overview. Implement the new MetaPlugin keybinding_filter hook, which provides a cleaner way to achieve the same.
Created attachment 228811 [details] [review] main: Use Params for optional parameters to pushModal() As the number of optional parameters grows, it gets more convenient to use Params instead of passing in a lot of undefined/null/0 values.
Created attachment 228812 [details] [review] grabHelper: Support optional parameters to pushModal()
Created attachment 228813 [details] [review] main: Add optional keybindingMode parameter to pushModal() For now we just use it to mark "special" modes, but it will shortly be used to filter out keybindings selectively.
Created attachment 228814 [details] [review] windowManager: Add a generic mechanism for filtering keybindings Currently we hardcode the set of keybindings that are available in the overview; add a generic mechanism to specify in which KeybindingModes a keybinding should be available.
Created attachment 228815 [details] [review] windowManager: Use generic filter mechanism for all keybindings With the new flexible system in place, there's no point explicitly hardcoding some build-in keybindings; just use the generic mechanism for everything.
Created attachment 228816 [details] [review] windowManager: Replace sessionMode.allowKeybindingsWhenModal The original condition the property was based on was added to make the a11y switcher available in the login screen, though it did never work properly - after popping up the switcher, additional tab key presses were ignored. Use a fancier check based on the current KeybindingMode instead, which allows for finer control and fixes that issue.
Created attachment 228817 [details] [review] Add screenshield and unlock dialog to ctrl-alt-tab As we now allow the ctrl-alt-tab popup on the lock screen, it should be possible to navigate back from the top bar, so add the corresponding elements to the switcher.
Created attachment 228818 [details] [review] messageTray: Make toggle-message-tray available when up It makes so much sense that the setting even has 'toggle' in its name.
Created attachment 228819 [details] [review] recorder: Make toggle-recorder action available in the overview There's not really a good reason to not allow starting a recording in the overview ...
Created attachment 228820 [details] [review] viewSelector: Make toggle-application-view binding available in overview
Review of attachment 228813 [details] [review]: What "special" modes are there? That implies that we special-case some modes, but it doesn't look like we do that in this patch. ::: js/ui/main.js @@ +41,3 @@ const DEFAULT_BACKGROUND_COLOR = Clutter.Color.from_pixel(0x2e3436ff); +const KeybindingMode = { Move this out of Main; I think it belongs in GrabHelper (I eventually want to remove pushModal() entirely and port everything to GrabHelper), but for now this is fine.
Review of attachment 228808 [details] [review]: This is surprisingly nifty.
Review of attachment 228809 [details] [review]: ::: src/core/prefs.c @@ +1716,3 @@ (GDestroyNotify)meta_key_pref_free); + + name = "overlay-key"; Why the "name" here and the literal in the other hunk?
Review of attachment 228810 [details] [review]: Minor style nit; otherwise I like the approach. ::: src/shell-wm.c @@ +122,3 @@ + G_SIGNAL_RUN_LAST, + 0, + g_signal_accumulator_true_handled, NULL, NULL, Seems to be some whitespace issues here.
Review of attachment 228811 [details] [review]: .
Review of attachment 228812 [details] [review]: Fine with me.
Review of attachment 228814 [details] [review]: ::: js/ui/windowManager.js @@ +146,3 @@ }, + setCustomKeybindingHandler: function(name, flags, handler) { The return! @@ +473,3 @@ + let flag; + if (Main.keybindingMode == Main.KeybindingMode.OVERVIEW) + flag = Main.KeybindingModeFlags.OVERVIEW; ouuuuuch. Would you mind doing a lookup table for this, or making the modes themselves flags?
Review of attachment 228815 [details] [review]: ::: js/ui/windowManager.js @@ +106,3 @@ this._workspaceSwitcherPopup = null; + this.setCustomKeybindingHandler('switch-to-workspace-left', + Main.KeybindingModeFlags.OVERVIEW, This can be done at any time, not just in the overview, no?
Review of attachment 228816 [details] [review]: ::: js/ui/main.js @@ +572,3 @@ + if (keybindingMode != params.keybindingMode) + perModeModalCounts[params.keybindingMode] = 0; I don't understand this logic at all; if we push a keybinding mode that's not on the top of the stack, but exists, it gets reset to 0?
Review of attachment 228817 [details] [review]: Sure.
Review of attachment 228818 [details] [review]: Sure.
Review of attachment 228820 [details] [review]: Sure.
Review of attachment 228819 [details] [review]: OK.
Review of attachment 228815 [details] [review]: ::: js/ui/main.js @@ +99,3 @@ + wm.allowKeybinding('overlay-key', + sessionMode.hasOverview ? KeybindingModeFlags.OVERVIEW + : KeybindingModeFlags.NONE); If the session mode doesn't have an overview, does it hurt to allow the keybinding in OVERVIEW ?
(In reply to comment #16) > Review of attachment 228809 [details] [review]: > + name = "overlay-key"; > > Why the "name" here and the literal in the other hunk? It's basically a left-over from a previous patch that I dropped at some point. I've removed the variable locally now.
(In reply to comment #17) > @@ +122,3 @@ > + G_SIGNAL_RUN_LAST, > + 0, > + g_signal_accumulator_true_handled, NULL, NULL, > > Seems to be some whitespace issues here. But they are consistent with the surrounding code!!11!! Fixed locally.
(In reply to comment #14) > What "special" modes are there? That implies that we special-case some modes, > but it doesn't look like we do that in this patch. Not sure what you mean. Assigning a name (well, technically a number) to some modes while keeping an "anonymous" pushModal() for others is special-casing, no? Feel free to suggest something else though, I'm not clinging to this commit message :-) > ::: js/ui/main.js > @@ +41,3 @@ > const DEFAULT_BACKGROUND_COLOR = Clutter.Color.from_pixel(0x2e3436ff); > > +const KeybindingMode = { > > Move this out of Main; I think it belongs in GrabHelper Mmmh, I was considering putting it into windowManager, but went with Main as it's a more common include (and particularly users of pushModal() already have it). I think this reasoning will apply to GrabHelper once it becomes a replacement for pushModal(), but for now it's the last place I'd expect it. Are you OK with leaving it in Main and make moving it part of the GrabHelper patches?
(In reply to comment #30) > (In reply to comment #14) > > What "special" modes are there? That implies that we special-case some modes, > > but it doesn't look like we do that in this patch. > > Not sure what you mean. Assigning a name (well, technically a number) to some > modes while keeping an "anonymous" pushModal() for others is special-casing, > no? Feel free to suggest something else though, I'm not clinging to this commit > message :-) main: Add optional keybindingMode parameter to pushModal() For now we just use it to mark keybindings with their supposed use but don't use them to filter keypresses out; we'll start doing this shortly. Something like that. > > ::: js/ui/main.js > > @@ +41,3 @@ > > const DEFAULT_BACKGROUND_COLOR = Clutter.Color.from_pixel(0x2e3436ff); > > > > +const KeybindingMode = { > > > > Move this out of Main; I think it belongs in GrabHelper > > Mmmh, I was considering putting it into windowManager, but went with Main as > it's a more common include (and particularly users of pushModal() already have > it). I think this reasoning will apply to GrabHelper once it becomes a > replacement for pushModal(), but for now it's the last place I'd expect it. > > Are you OK with leaving it in Main and make moving it part of the GrabHelper > patches? A few minutes after I hit "Submit" I thought windowManager would be the best place, but I guess it's just not worth it. I just hate that we have a bunch of unrelated garbage in "Main" that really should be moved out to separate files (workspace management should be in windowManager.js, deferred work should be in a utility file somewhere, etc.)
(In reply to comment #21) > @@ +106,3 @@ > this._workspaceSwitcherPopup = null; > + this.setCustomKeybindingHandler('switch-to-workspace-left', > + Main.KeybindingModeFlags.OVERVIEW, > > This can be done at any time, not just in the overview, no? No, this works exactly like it works now - it works in the overview, but not e.g. with a system modal dialog open. Or in case that you are wondering about "normal" mode - the filter_keybinding() hook is only relevant when the compositor has a grab, otherwise keybindings are handled normally (exactly like now). Running the hook in any case would be a trivial change, we'd just have to add a huge list of allowKeybinding(KeybindingMode.NORMAL). Given that so far we don't have a single use case, I tend to prefer the current way and change it if we ever happen to need it (there's also a simple workaround using "if (Main.modalCount == 0) return;", though I'd rather change the hook if that becomes widespread)
Created attachment 228919 [details] [review] main: Add optional keybindingMode parameter to pushModal() For now we just use it to assign an identifier to modal modes in which we want to allow some keybindings, but we don't use it for any actual filtering; we'll start doing this shortly. Better?
Review of attachment 228919 [details] [review]: The only thing I'd do is add a comment to the keybinding modes about KeybindingMode.NONE and what it's used for.
Created attachment 228922 [details] [review] windowManager: Add a generic mechanism for filtering keybindings (In reply to comment #20) > ouuuuuch. Would you mind doing a lookup table for this, or making the modes > themselves flags? For now I left them separate and tweaked the values to allow mapping from mode to flags algorithmically - it's a bit of a hack, I'll change the enum to allow using it directly as flags if you prefer that. (In reply to comment #27) > + wm.allowKeybinding('overlay-key', > + sessionMode.hasOverview ? KeybindingModeFlags.OVERVIEW > + : KeybindingModeFlags.NONE); > > If the session mode doesn't have an overview, does it hurt to allow the > keybinding in OVERVIEW ? Yeah, looks like that's a left-over workaround for some buggy behavior that got fixed at some point.
Created attachment 228923 [details] [review] windowManager: Use generic filter mechanism for all keybindings With the new flexible system in place, there's no point explicitly hardcoding some build-in keybindings; just use the generic mechanism for everything.
Review of attachment 228922 [details] [review]: I think having one enum instead of two would be better, yeah. Otherwise, we're going to see bugs with: Main.wm.addKeybinding("blah", Main.KeybindingMode.OVERVIEW, handleBlah);
Created attachment 228925 [details] [review] main: Add optional keybindingMode parameter to pushModal() Added some additional comments
Created attachment 228926 [details] [review] windowManager: Add a generic mechanism for filtering keybindings (re-attaching)
Created attachment 228927 [details] [review] windowManager: Use generic filter mechanism for all keybindings Dropped KeybindingModeFlags in favor of KeybindingMode itself. ^^^___ That's actually about the previous patch, this one's re-attached (except for the obvious s/KeybindingModeFlags/KeybindingMode/ changes, I won't re-attach the a-c-n patches that are affected by that as well)
Created attachment 228928 [details] [review] windowManager: Replace sessionMode.allowKeybindingsWhenModal (In reply to comment #22) > + if (keybindingMode != params.keybindingMode) > + perModeModalCounts[params.keybindingMode] = 0; > > I don't understand this logic at all; if we push a keybinding mode that's not > on the top of the stack, but exists, it gets reset to 0? Yeah, it's tricky - the idea is "pushModals() on top of a special mode" The patch used to be a lot simpler, but I found some breakage in the login screen yesterday and quickly came up with this. Results that I squashed a couple of bugs since then, so ... TADAAA ... the patch becomes super simple.
Review of attachment 228925 [details] [review]: ::: js/ui/main.js @@ +42,3 @@ +const KeybindingMode = { + NONE: 0, /* used when not modal or to filter out all bindings */ I still don't like this comment. If I'm adding a keybinding, what does passing "KeybindingMode.NONE" do? @@ +555,1 @@ + keybindingMode = params.keybindingMode; Shouldn't we only modify the keybinding mode if it's not NONE?
Review of attachment 228926 [details] [review]: Sure.
Review of attachment 228927 [details] [review]: "built-in" in commit message, otherwise fine.
Review of attachment 228928 [details] [review]: I'm not sure if this will work, but whatever.
(In reply to comment #42) > +const KeybindingMode = { > + NONE: 0, /* used when not modal or to filter out all bindings */ > > I still don't like this comment. If I'm adding a keybinding, what does passing > "KeybindingMode.NONE" do? Note the 2nd comment in the pushModal() docs: * - keybindingMode: used to set the current Main.KeybindingMode to filter * global keybindings; the default of NONE will filter * out all keybindings during compositor grabs > @@ +555,1 @@ > + keybindingMode = params.keybindingMode; > > Shouldn't we only modify the keybinding mode if it's not NONE? Nope, that's actually what was broken about the a11y switcher in the login screen: the first ctrl-alt-tab popped up the switcher, and as we kept processing keybindings, consecutive tab presses didn't transfer control to the switcher, but kept focusing the first element. The current code is convenient, but we could split NONE into NONE (used when we are not modal, e.g. in normal mode => all keybindings are processed normally) and BLOCK (filter out all keybindings when modal).
(In reply to comment #46) > The current code is convenient, but we could split NONE into NONE (used when we > are not modal, e.g. in normal mode => all keybindings are processed normally) It's probably worth stressing again that filterKeybinding() is not called in normal mode, so the value of keybindingMode does not actually matter in that case.
Created attachment 229184 [details] [review] Add compositor hook to process keybindings selectively Use the hook regardless of compositor grabs
Created attachment 229185 [details] [review] keybindings: Add MetaKeyBinding for overlay-key (reattaching)
Created attachment 229186 [details] [review] keybindings: Add is_builtin() method
Created attachment 229187 [details] [review] windowManager: Implement keybinding_filter hook Update for mutter changes
Created attachment 229188 [details] [review] main: Use Params for optional parameters to pushModal() (reattaching)
Created attachment 229189 [details] [review] grabHelper: Support optional parameters to pushModal() (reattaching)
Created attachment 229190 [details] [review] main: Add optional keybindingMode parameter to pushModal() Add additional NORMAL mode
Created attachment 229191 [details] [review] windowManager: Add a generic mechanism for filtering keybindings reattaching with minor changes due to mutter changes
Created attachment 229192 [details] [review] windowManager: Use generic filter mechanism for all keybindings Pass KeybindingMode.NORMAL as necessary
Created attachment 229193 [details] [review] windowManager: Replace sessionMode.allowKeybindingsWhenModal (reattaching)
Created attachment 229194 [details] [review] Add screenshield and unlock dialog to ctrl-alt-tab (reattaching)
Created attachment 229195 [details] [review] messageTray: Make toggle-message-tray available when up Adjust mode flags.
Created attachment 229196 [details] [review] recorder: Make toggle-recorder action available in the overview Adjust mode flags
Created attachment 229197 [details] [review] viewSelector: Make toggle-application-view binding available in overview Adjust mode flags
Florian, whatever patches you didn't change, can you mark as ACN? The first patch, for instance, has a different commit message, but does it have any code changes?
(In reply to comment #62) > Florian, whatever patches you didn't change, can you mark as ACN? Sure - I'm also marking patches with only trivial changes (e.g. passing Main.KeybindingMode.NORMAL in addition to the flags in the previous version). > The first patch, for instance, has a different commit message, but does > it have any code changes? Yes: (comment #48) > Use the hook regardless of compositor grabs
Review of attachment 229184 [details] [review]: Fine with me, although I thought we were going to drop this for now. But if you got it working, sure.
Review of attachment 229186 [details] [review]: Aha, OK, this is the approach you're going with :)
Review of attachment 229187 [details] [review]: Yes.
Review of attachment 229190 [details] [review]: ::: js/ui/main.js @@ +42,3 @@ +const KeybindingMode = { + NONE: 0, // filter out all keybindings "block all keybindings"
Review of attachment 229191 [details] [review]: ::: js/ui/windowManager.js @@ +471,3 @@ } + if (Main.keybindingMode == Main.keybindingMode.NORMAL && A comment about this would be nice: // This is a bit of convenience so we don't have to // explicitly allow all builtin keybindings in NORMAL // mode.
Attachment 229184 [details] pushed as 424fc52 - Add compositor hook to process keybindings selectively Attachment 229185 [details] pushed as aa43e71 - keybindings: Add MetaKeyBinding for overlay-key Attachment 229186 [details] pushed as 6004197 - keybindings: Add is_builtin() method
Attachment 229187 [details] pushed as 0344089 - windowManager: Implement keybinding_filter hook Attachment 229188 [details] pushed as c2065cc - main: Use Params for optional parameters to pushModal() Attachment 229189 [details] pushed as 5f36724 - grabHelper: Support optional parameters to pushModal() Attachment 229190 [details] pushed as b58f502 - main: Add optional keybindingMode parameter to pushModal() Attachment 229191 [details] pushed as 0d9f704 - windowManager: Add a generic mechanism for filtering keybindings Attachment 229192 [details] pushed as 76d7762 - windowManager: Use generic filter mechanism for all keybindings Attachment 229193 [details] pushed as 28b559e - windowManager: Replace sessionMode.allowKeybindingsWhenModal Attachment 229194 [details] pushed as ced7fa9 - Add screenshield and unlock dialog to ctrl-alt-tab Attachment 229195 [details] pushed as 2434af7 - messageTray: Make toggle-message-tray available when up Attachment 229196 [details] pushed as bd40cf1 - recorder: Make toggle-recorder action available in the overview Attachment 229197 [details] pushed as f084011 - viewSelector: Make toggle-application-view binding available in overview
*** Bug 682229 has been marked as a duplicate of this bug. ***