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 688202 - Introduce a more fine-grained approach to keybindings
Introduce a more fine-grained approach to keybindings
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 682229 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-11-12 19:32 UTC by Florian Müllner
Modified: 2012-11-17 01:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add hook for selectively processing keybindings during compositor grabs (4.15 KB, patch)
2012-11-12 19:32 UTC, Florian Müllner
accepted-commit_now Details | Review
keybindings: Add MetaKeyBinding for overlay-key (3.18 KB, patch)
2012-11-12 19:32 UTC, Florian Müllner
reviewed Details | Review
windowManager: Implement keybinding_filter hook (9.84 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
main: Use Params for optional parameters to pushModal() (4.17 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
grabHelper: Support optional parameters to pushModal() (1.50 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
main: Add optional keybindingMode parameter to pushModal() (9.14 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
reviewed Details | Review
windowManager: Add a generic mechanism for filtering keybindings (3.69 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
reviewed Details | Review
windowManager: Use generic filter mechanism for all keybindings (9.41 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
reviewed Details | Review
windowManager: Replace sessionMode.allowKeybindingsWhenModal (3.25 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
reviewed Details | Review
Add screenshield and unlock dialog to ctrl-alt-tab (1.77 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
messageTray: Make toggle-message-tray available when up (2.12 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
recorder: Make toggle-recorder action available in the overview (1.59 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
viewSelector: Make toggle-application-view binding available in overview (1.69 KB, patch)
2012-11-12 19:33 UTC, Florian Müllner
accepted-commit_now Details | Review
main: Add optional keybindingMode parameter to pushModal() (9.22 KB, patch)
2012-11-13 19:13 UTC, Florian Müllner
reviewed Details | Review
windowManager: Add a generic mechanism for filtering keybindings (3.00 KB, patch)
2012-11-13 19:26 UTC, Florian Müllner
needs-work Details | Review
windowManager: Use generic filter mechanism for all keybindings (9.32 KB, patch)
2012-11-13 19:27 UTC, Florian Müllner
none Details | Review
main: Add optional keybindingMode parameter to pushModal() (9.41 KB, patch)
2012-11-13 19:50 UTC, Florian Müllner
reviewed Details | Review
windowManager: Add a generic mechanism for filtering keybindings (2.19 KB, patch)
2012-11-13 19:51 UTC, Florian Müllner
accepted-commit_now Details | Review
windowManager: Use generic filter mechanism for all keybindings (9.20 KB, patch)
2012-11-13 19:53 UTC, Florian Müllner
accepted-commit_now Details | Review
windowManager: Replace sessionMode.allowKeybindingsWhenModal (1.95 KB, patch)
2012-11-13 19:55 UTC, Florian Müllner
accepted-commit_now Details | Review
Add compositor hook to process keybindings selectively (5.83 KB, patch)
2012-11-16 23:39 UTC, Florian Müllner
committed Details | Review
keybindings: Add MetaKeyBinding for overlay-key (3.16 KB, patch)
2012-11-16 23:39 UTC, Florian Müllner
committed Details | Review
keybindings: Add is_builtin() method (1.56 KB, patch)
2012-11-16 23:39 UTC, Florian Müllner
committed Details | Review
windowManager: Implement keybinding_filter hook (10.00 KB, patch)
2012-11-16 23:40 UTC, Florian Müllner
committed Details | Review
main: Use Params for optional parameters to pushModal() (4.17 KB, patch)
2012-11-16 23:40 UTC, Florian Müllner
committed Details | Review
grabHelper: Support optional parameters to pushModal() (1.50 KB, patch)
2012-11-16 23:40 UTC, Florian Müllner
committed Details | Review
main: Add optional keybindingMode parameter to pushModal() (9.42 KB, patch)
2012-11-16 23:42 UTC, Florian Müllner
committed Details | Review
windowManager: Add a generic mechanism for filtering keybindings (2.38 KB, patch)
2012-11-16 23:43 UTC, Florian Müllner
committed Details | Review
windowManager: Use generic filter mechanism for all keybindings (10.01 KB, patch)
2012-11-16 23:43 UTC, Florian Müllner
committed Details | Review
windowManager: Replace sessionMode.allowKeybindingsWhenModal (1.96 KB, patch)
2012-11-16 23:43 UTC, Florian Müllner
committed Details | Review
Add screenshield and unlock dialog to ctrl-alt-tab (1.77 KB, patch)
2012-11-16 23:44 UTC, Florian Müllner
committed Details | Review
messageTray: Make toggle-message-tray available when up (2.23 KB, patch)
2012-11-16 23:44 UTC, Florian Müllner
committed Details | Review
recorder: Make toggle-recorder action available in the overview (1.65 KB, patch)
2012-11-16 23:44 UTC, Florian Müllner
committed Details | Review
viewSelector: Make toggle-application-view binding available in overview (1.75 KB, patch)
2012-11-16 23:44 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2012-11-12 19:32:21 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, ...).
Comment 1 Florian Müllner 2012-11-12 19:32:24 UTC
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.
Comment 2 Florian Müllner 2012-11-12 19:32:27 UTC
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.
Comment 3 Florian Müllner 2012-11-12 19:33:00 UTC
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.
Comment 4 Florian Müllner 2012-11-12 19:33:06 UTC
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.
Comment 5 Florian Müllner 2012-11-12 19:33:11 UTC
Created attachment 228812 [details] [review]
grabHelper: Support optional parameters to pushModal()
Comment 6 Florian Müllner 2012-11-12 19:33:16 UTC
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.
Comment 7 Florian Müllner 2012-11-12 19:33:20 UTC
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.
Comment 8 Florian Müllner 2012-11-12 19:33:25 UTC
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.
Comment 9 Florian Müllner 2012-11-12 19:33:30 UTC
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.
Comment 10 Florian Müllner 2012-11-12 19:33:38 UTC
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.
Comment 11 Florian Müllner 2012-11-12 19:33:42 UTC
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.
Comment 12 Florian Müllner 2012-11-12 19:33:47 UTC
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 ...
Comment 13 Florian Müllner 2012-11-12 19:33:52 UTC
Created attachment 228820 [details] [review]
viewSelector: Make toggle-application-view binding available in overview
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:41:04 UTC
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.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:41:41 UTC
Review of attachment 228808 [details] [review]:

This is surprisingly nifty.
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:42:09 UTC
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?
Comment 17 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:43:24 UTC
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.
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:44:16 UTC
Review of attachment 228811 [details] [review]:

.
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:44:28 UTC
Review of attachment 228812 [details] [review]:

Fine with me.
Comment 20 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:46:31 UTC
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?
Comment 21 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:47:37 UTC
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?
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:50:22 UTC
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?
Comment 23 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:51:32 UTC
Review of attachment 228817 [details] [review]:

Sure.
Comment 24 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:52:11 UTC
Review of attachment 228818 [details] [review]:

Sure.
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:52:37 UTC
Review of attachment 228820 [details] [review]:

Sure.
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-11-12 19:52:42 UTC
Review of attachment 228819 [details] [review]:

OK.
Comment 27 Matthias Clasen 2012-11-13 00:40:05 UTC
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 ?
Comment 28 Florian Müllner 2012-11-13 17:55:50 UTC
(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.
Comment 29 Florian Müllner 2012-11-13 17:56:46 UTC
(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.
Comment 30 Florian Müllner 2012-11-13 18:04:10 UTC
(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?
Comment 31 Jasper St. Pierre (not reading bugmail) 2012-11-13 18:09:11 UTC
(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.)
Comment 32 Florian Müllner 2012-11-13 18:33:05 UTC
(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)
Comment 33 Florian Müllner 2012-11-13 19:13:57 UTC
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?
Comment 34 Jasper St. Pierre (not reading bugmail) 2012-11-13 19:17:46 UTC
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.
Comment 35 Florian Müllner 2012-11-13 19:26:33 UTC
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.
Comment 36 Florian Müllner 2012-11-13 19:27:49 UTC
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.
Comment 37 Jasper St. Pierre (not reading bugmail) 2012-11-13 19:30:32 UTC
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);
Comment 38 Florian Müllner 2012-11-13 19:50:44 UTC
Created attachment 228925 [details] [review]
main: Add optional keybindingMode parameter to pushModal()

Added some additional comments
Comment 39 Florian Müllner 2012-11-13 19:51:02 UTC
Created attachment 228926 [details] [review]
windowManager: Add a generic mechanism for filtering keybindings

(re-attaching)
Comment 40 Florian Müllner 2012-11-13 19:53:02 UTC
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)
Comment 41 Florian Müllner 2012-11-13 19:55:58 UTC
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.
Comment 42 Jasper St. Pierre (not reading bugmail) 2012-11-13 20:55:03 UTC
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?
Comment 43 Jasper St. Pierre (not reading bugmail) 2012-11-13 20:55:22 UTC
Review of attachment 228926 [details] [review]:

Sure.
Comment 44 Jasper St. Pierre (not reading bugmail) 2012-11-13 20:56:12 UTC
Review of attachment 228927 [details] [review]:

"built-in" in commit message, otherwise fine.
Comment 45 Jasper St. Pierre (not reading bugmail) 2012-11-13 20:58:14 UTC
Review of attachment 228928 [details] [review]:

I'm not sure if this will work, but whatever.
Comment 46 Florian Müllner 2012-11-14 00:04:56 UTC
(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).
Comment 47 Florian Müllner 2012-11-14 00:11:05 UTC
(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.
Comment 48 Florian Müllner 2012-11-16 23:39:04 UTC
Created attachment 229184 [details] [review]
Add compositor hook to process keybindings selectively

Use the hook regardless of compositor grabs
Comment 49 Florian Müllner 2012-11-16 23:39:17 UTC
Created attachment 229185 [details] [review]
keybindings: Add MetaKeyBinding for overlay-key

(reattaching)
Comment 50 Florian Müllner 2012-11-16 23:39:22 UTC
Created attachment 229186 [details] [review]
keybindings: Add is_builtin() method
Comment 51 Florian Müllner 2012-11-16 23:40:22 UTC
Created attachment 229187 [details] [review]
windowManager: Implement keybinding_filter hook

Update for mutter changes
Comment 52 Florian Müllner 2012-11-16 23:40:36 UTC
Created attachment 229188 [details] [review]
main: Use Params for optional parameters to pushModal()

(reattaching)
Comment 53 Florian Müllner 2012-11-16 23:40:45 UTC
Created attachment 229189 [details] [review]
grabHelper: Support optional parameters to pushModal()

(reattaching)
Comment 54 Florian Müllner 2012-11-16 23:42:00 UTC
Created attachment 229190 [details] [review]
main: Add optional keybindingMode parameter to pushModal()

Add additional NORMAL mode
Comment 55 Florian Müllner 2012-11-16 23:43:00 UTC
Created attachment 229191 [details] [review]
windowManager: Add a generic mechanism for filtering keybindings

reattaching with minor changes due to mutter changes
Comment 56 Florian Müllner 2012-11-16 23:43:42 UTC
Created attachment 229192 [details] [review]
windowManager: Use generic filter mechanism for all keybindings

Pass KeybindingMode.NORMAL as necessary
Comment 57 Florian Müllner 2012-11-16 23:43:56 UTC
Created attachment 229193 [details] [review]
windowManager: Replace sessionMode.allowKeybindingsWhenModal

(reattaching)
Comment 58 Florian Müllner 2012-11-16 23:44:06 UTC
Created attachment 229194 [details] [review]
Add screenshield and unlock dialog to ctrl-alt-tab

(reattaching)
Comment 59 Florian Müllner 2012-11-16 23:44:27 UTC
Created attachment 229195 [details] [review]
messageTray: Make toggle-message-tray available when up

Adjust mode flags.
Comment 60 Florian Müllner 2012-11-16 23:44:37 UTC
Created attachment 229196 [details] [review]
recorder: Make toggle-recorder action available in the overview

Adjust mode flags
Comment 61 Florian Müllner 2012-11-16 23:44:47 UTC
Created attachment 229197 [details] [review]
viewSelector: Make toggle-application-view binding available in overview

Adjust mode flags
Comment 62 Jasper St. Pierre (not reading bugmail) 2012-11-16 23:53:21 UTC
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?
Comment 63 Florian Müllner 2012-11-17 00:10:21 UTC
(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
Comment 64 Jasper St. Pierre (not reading bugmail) 2012-11-17 00:17:41 UTC
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.
Comment 65 Jasper St. Pierre (not reading bugmail) 2012-11-17 00:18:08 UTC
Review of attachment 229186 [details] [review]:

Aha, OK, this is the approach you're going with :)
Comment 66 Jasper St. Pierre (not reading bugmail) 2012-11-17 00:18:47 UTC
Review of attachment 229187 [details] [review]:

Yes.
Comment 67 Jasper St. Pierre (not reading bugmail) 2012-11-17 00:19:54 UTC
Review of attachment 229190 [details] [review]:

::: js/ui/main.js
@@ +42,3 @@
 
+const KeybindingMode = {
+    NONE:          0,       // filter out all keybindings

"block all keybindings"
Comment 68 Jasper St. Pierre (not reading bugmail) 2012-11-17 00:21:07 UTC
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.
Comment 69 Florian Müllner 2012-11-17 00:58:47 UTC
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
Comment 70 Florian Müllner 2012-11-17 01:02:26 UTC
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
Comment 71 Florian Müllner 2012-11-17 01:12:55 UTC
*** Bug 682229 has been marked as a duplicate of this bug. ***