I'm not sure if anything changed in the core, but alt-tab processing happens before clutter gets the events remember; in mutter's event_callback.c, meta_display_process_key_event is called before meta_compositor_process_event.  Our keyboard grab code should probably override that.

Created attachment 139879 [details] [review]
Bug 590686 - Avoid keyboard grabs being broken by Alt-Tab etc.

Disable all mutter-internal keyboard processing in Main.startModal.
The idea of that function is that the plugin now owns the keyboard
entirely.

Remove the startModal() from altTab; it shouldn't be needed.

Hm, this does remove the ability to Alt-tab and Alt-f2 from the overlay which is probably not what we want.

Maybe we can tie the concept of "no WM builtins" into a "dialog manager" framework.  Concretely what I'd like to see here is that if I say enter the overlay then Alt-F2 "lg", focus should go from overlay->run dialog->LG->overlay.

(In reply to comment #3)
> Hm, this does remove the ability to Alt-tab and Alt-f2 from the overlay which
> is probably not what we want.

Huh? I thought Alt-Tab and Alt-F2 weren't supposed to work in the overlay anyway?

They don't work *correctly* now; after you do either of them, the input mask reverts to covering just the panel, so the overlay no longer functions.

I definitely want to be able to Alt-f2 from the overlay, or well what I really want is to be able to run lookingGlass; if we figure out another way to do that i'd be happy.

I have no opinion on whether Alt-Tab should work in the overlay or not.  I think it'd be nice if it did work, but just didn't show the popup maybe.

Not sure if this is of any help to you guys, but the Alt+Tab handling in Mutter always means that Mutter will acquire an active grab on the entire keybaoard; when it then releases this grab, this results in *all* passive grabs held by the entire process (including the plugin) being released. Mutter will automatically re-establish its own passive grabs for its key bindings, but the plugin needs to take care of it's own passive grabs (you can use MetaScreen::keyboar-grabbed property to get notification when the screen grab is released).

I should add, it cuts both ways -- you must not ever directly call XUngrabKeyboard() in the plugin if you expect any of the standard Mutter kbd shortcuts to work. MetaDisplay exposes API for doing kbd grabs, if it's not enough for your purposes, we can extend it.

(In reply to comment #6)
> Not sure if this is of any help to you guys, but the Alt+Tab handling in Mutter
> always means that Mutter will acquire an active grab on the entire keybaoard;

Right...well, what we need is a way to disable Alt-Tab selectively, and probably other internal keybindings like the overlay key.  *Ideally*, we'd be able to in this mode if we choose, run the same handler that MetaDisplay would have run had it gotten the event.  So for example in LookingGlass we can hammer away all mutter internal processing, but in the overlay we could enter this mode, but run the alt-tab handler manually. 

(In reply to comment #7)
> MetaDisplay exposes API for doing kbd grabs, if it's not
> enough for your purposes, we can extend it.

What's the API in MetaDisplay?

Note that meta_screen_grab_all_keys() explicitly calls meta_screen_ungrab_keys() to unestablish the standard keybindings - this isn't some side-effect of the normal X calls. This calls:

  XUngrabKey (display->xdisplay, AnyKey, AnyModifier,
              xwindow);

Which is why you are needing to reestablish your grabs when ::keyboard-grabbed becomes false. It's not a normal side-effect of XUngrabKeyboard(). In fact right now the gnome-shell plugin calls XGrabKeyboard()/XUngrabKeyboard() itself without any ill effects (other than that you can alt-tab out of that grab.)

I'm uncertain as to why Mutter/Metacity is ungrabbing its passive grabs when getting an active grab - X will ignore passive grabs when an active grab is active. This was introduced as part of a big patch that reworked grabbing in various ways. 
 
http://git.gnome.org/cgit/metacity/commit/?id=6b72d622a5c1b0c02672f73eeeca6403b946d1bb

I think it's just a mistake and removing it leaves things working as far as I can tell in a quick test. (Note that when the grab is on a particular window, as is is in most cases, only the keys grabbed *on that window* get ungrabbed then regrabbed.)

That's a bit secondary to what is being discussed here. The code in Metacity/Mutter to try and ensure exclusivity is pretty dodgy. If I'm in Move mode (Window-menu - move) and hit Alt-Tab:

 - process_keyboard_move_grab() handles the arrow press moves the window
 - The key is passed on to process_event() and handle_workspace_switch
 - handle_workspace_switch() tries call
   meta_display_begin_grab_op() which fails, so nothing further happens.

And in fact, I'm pretty sure, if I'm alt-Tabbing every subsequent Alt-Tab is handled in process_tab_grab(), then passed to process_event(), which eventually tries to call meta_display_begin_grab_op() again to start the tab op again, which fails.

In my opinion:

 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.

(In reply to comment #9)
>  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.

Agreed on all three counts here; I will copy this for reference to bug #590754 and we can continue the discussion there.

Created attachment 140521 [details] [review]
Use new plugin-modality functionality in Mutter

We now have functionality in Mutter to grab the keyboard on behalf
of a plugin. This avoids interactions with the key handling code
in Mutter that could leave the user with an inconsistent state
and no way to get out of it.

src/shell-global.[ch]: Change shell_global_grab_keyboard() and
  shell_global_grab_keyboard() to shell_global_begin_modal()
  shell_global_end_modal() and call mutter_plugin_begin_modal()
  mutter_plugin_end_modal() rather than directly grabbing the
  keyboard.
main.js: Call global.begin_modal/end_modal from Main.startModal()
  and Main.endModal()
altTab.js; Remove call to Main.startModal() - we're letting Mutter
  handle modality for Alt-Tab.
main.js lookingGlass.js overview.js runDialog.js: Rename
  Main.startModal() to Main.beginModal() for consistency with
  naming in mutter and ShellGlobal.

Attached patch depends on patches attached to the Mutter bug.

The parts of the Mutter bug that the patch depends on above have now been fixed. There is some regression from the patch above - in particular it's no longer possible to hit Alt-PrintScreen in the overview - but it was sort of an accident that it worked before, so I think we should handle that as an add-on fix. (If a pretty important one for development.)

Review would be appreciated so we can get this in

Comment on attachment 140521 [details] [review]
Use new plugin-modality functionality in Mutter


>+    let timestamp = global.screen.get_display().get_current_time();

In other places we're using Clutter.get_current_event_time(); I think they're the same thing, though since this is closer to "X" feel maybe using the display is right.

>@@ -475,66 +474,36 @@ _shell_global_set_plugin (ShellGlobal  *global,
> }
> 
> /**
>- * shell_global_grab_keyboard:
>+ * shell_global_begin_modal:
>  * @global: a #ShellGlobal
>  *
>- * Grab the keyboard to the stage window. The stage will receive
>- * all keyboard events until shell_global_ungrab_keyboard() is called.
>- * This is appropriate to do when the desktop goes into a special
>+ * Grab the keyboard and mouse to the stage window. The stage will
>+ * receive all keyboard and mouse events until shell_global_end_modal()
>+ * is called This is appropriate to do when the desktop goes into a special

Missing '.' (or ';').  I'd remove the chunk "the desktop goes into a special mode" since the second part explains what special is.

It would also be good if this function explained what happened if called twice (not supported?  no-op?).  Or maybe defer to the docs for mutter_plugin_begin_modal explicitly.

(In reply to comment #14)
> (From update of attachment 140521 [details] [review])
> 
> >+    let timestamp = global.screen.get_display().get_current_time();
> 
> In other places we're using Clutter.get_current_event_time(); I think they're
> the same thing, though since this is closer to "X" feel maybe using the display
> is right.

clutter_get_current_event_time() is only accurate in callbacks triggered from clutter. meta_display_get_current_time() should be accurate from any callback, I think, although looking at display.c:event_callback(), it looks like it won't be set during the processing of a startup notification message...

Comment on attachment 140521 [details] [review]
Use new plugin-modality functionality in Mutter

Thanks for the review

(In reply to comment #14)
> (From update of attachment 140521 [details] [review])
> 
> >+    let timestamp = global.screen.get_display().get_current_time();
> 
> In other places we're using Clutter.get_current_event_time(); I think they're
> the same thing, though since this is closer to "X" feel maybe using the 
> display is right.

They are the same thing if we're triggered out of a clutter callback. But for things like the Alt-F2 dialog we aren't triggered out of a clutter callback.

I think we should be using global.screen.get_display().get_current_time() pretty much everywhere, since there's always the potential for code to get copied into another context. 

Grepping, I see 4 examples of global.screen.get_display().get_current_time(), and one example of Clutter.get_current_event_time()

> >@@ -475,66 +474,36 @@ _shell_global_set_plugin (ShellGlobal  *global,
> > }
> > 
> > /**
> >- * shell_global_grab_keyboard:
> >+ * shell_global_begin_modal:
> >  * @global: a #ShellGlobal
> >  *
> >- * Grab the keyboard to the stage window. The stage will receive
> >- * all keyboard events until shell_global_ungrab_keyboard() is called.
> >- * This is appropriate to do when the desktop goes into a special
> >+ * Grab the keyboard and mouse to the stage window. The stage will
> >+ * receive all keyboard and mouse events until shell_global_end_modal()
> >+ * is called This is appropriate to do when the desktop goes into a special
> 
> Missing '.' (or ';').  I'd remove the chunk "the desktop goes into a special
> mode" since the second part explains what special is.

Hmm, I was trying to get at "typical use case" here. Made that more explicit as:

 * Grab the keyboard and mouse to the stage window. The stage will             
 * receive all keyboard and mouse events until shell_global_end_modal()
 * is called. This is used to implement "modes" for the shell, such as the 
 * overview mode or the "looking glass" debug overlay, that block               
 * application and normal key shortcuts.

> It would also be good if this function explained what happened if called twice
> (not supported?  no-op?).  Or maybe defer to the docs for
> mutter_plugin_begin_modal explicitly.

Added:

 * Return value: %TRUE if we succesfully enter the mode. %FALSE if we couldn't    
 *  enter the mode. Failure may occur because an application has the pointer      
 *  or keyboard grabbed, because Mutter is in a mode itself like moving a 
 *  window or alt-Tab window selection, or because shell_global_begin_modal() 
 *  was previouly called.

Pushing. Will leave this bug open to deal at least with the print-screen issue in some way.

(In reply to comment #15)
> clutter_get_current_event_time() is only accurate in callbacks triggered from
> clutter. 

The timestamps returned by clutter_get_event_time() reflects any event throttling / synthesizing that Clutter does, so I generally avoid passing them into X lib calls. There is an additional clutter_x11_get_current_event_time() which returns the timestamp of the last X event Clutter processed (providing it had a timestamp, of course).

(In reply to comment #13)
> The parts of the Mutter bug that the patch depends on above have now been
> fixed. There is some regression from the patch above - in particular it's no
> longer possible to hit Alt-PrintScreen in the overview - but it was sort of an
> accident that it worked before, so I think we should handle that as an add-on
> fix. (If a pretty important one for development.)

Plain PrintScreen works fine from the overview now (though you have to leave the overview to save the screenshot). Alt-PrintScreen seems to do a full-screen screenshot rather than a single-window screenshot, but it's not clear why you'd be trying to get a single-window screenshot from inside the overview anyway... so I think this is working and can be closed?

Yeah, Print was bound shortly after this was committed. There are some generality concerns - it's basically a hack that ignores the Mutter preferences, but that can be handled separately at some point when somebody noticed and complains and submits a patch :-)