Created attachment 175075 [details] [review]
Add an "Above_Tab" pseudo-keysym

Comment on attachment 175075 [details] [review]
Add an "Above_Tab" pseudo-keysym

you forgot to git add core/above-tab-keycode.c

Created attachment 175084 [details] [review]
Add an "Above_Tab" pseudo-keysym

With all files

Comment on attachment 175084 [details] [review]
Add an "Above_Tab" pseudo-keysym

>+  /* Now find the key that:
>+   *  - Has a horizontal center in the Tab key's horizonal bounds
>+   *  - Is above the Tab key at a distance closer than any other key
>+   *  - In case of ties, has its horizontal center as close as possible
>+   *    to the Tab keys horizontal center
>+   */

Sections can be rotated independently (eg, for ergonomic keyboards), so you can't always compare coordinates directly between different sections.

But I think it's fair for us to say that Above_Tab must be in the same section as Tab, because we want a key that's both physically and "logically" adjacent to Tab, not merely *somewhere* above it.

I would say that a keyboard only has a single candidate for Above_Tab, and that's the key in Tab's section that's in the row above Tab and has the same left coordinate as Tab. If such a key exists, and it's not "Esc", "F1", or "1" (in any level or group) then that key is Above_Tab. Otherwise there is no Above_Tab (or else it can fall back to something random, like F6, which IIRC was the old binding for cycle_windows.)

>+  /* Now we need to resolve the name of the best key back to a keycode */
>+  for (i = keyboard->min_key_code; i < keyboard->max_key_code; i++)
>+    {
>+      if (strncmp(best_key->name.name, keyboard->names->keys[i].name, XkbKeyNameLength) == 0)

space after strncmp

>+++ b/src/core/display-private.h
>+guint meta_display_get_above_tab_keycode (MetaDisplay *display);

if you're not planning to add an inverse for meta_display_get_keybinding_action(), then we'll need this function to be public so we can call it from altTab.js.

(In reply to comment #4)
> >+++ b/src/core/display-private.h
> >+guint meta_display_get_above_tab_keycode (MetaDisplay *display);
> 
> if you're not planning to add an inverse for
> meta_display_get_keybinding_action(), then we'll need this function to be
> public so we can call it from altTab.js.

Seems better to me to expose keybindings.c:display_get_keybinding()

The version of the patch I'm going to attach is about the same as the original except that it requires the key to be in the same section as the Tab key and fixes up a few missing spaces and comment typos.

Created attachment 177451 [details] [review]
Add an "Above_Tab" pseudo-keysym

We want switching between the windows of an application to be an easily
accessible operation. The convenient and memorable keybinding is the
key above the tab key - but the keysym for that key isn't consistent
across different keyboard layouts.

Add code that figures out the key from the XKB geometry and a magic
keysym name "Above_Tab" that refers to this key and switch
the default binding for cycle_group to <Alt>Above_Tab. (This will
have no effect for the normal case of getting the key binding from
GConf until this patch is applied to Metacity as well.)

(In reply to comment #5)
> > if you're not planning to add an inverse for
> > meta_display_get_keybinding_action(), then we'll need this function to be
> > public so we can call it from altTab.js.
> 
> Seems better to me to expose keybindings.c:display_get_keybinding()

I think we'd agreed before that altTab.js should call display.get_keybinding_action() on the key, and see if the result is CYCLE_WINDOWS. (And likewise SWITCH_WINDOWS instead of keycode==Clutter.Tab)

Comment on attachment 177451 [details] [review]
Add an "Above_Tab" pseudo-keysym

>the default binding for cycle_group to <Alt>Above_Tab. (This will
>have no effect for the normal case of getting the key binding from
>GConf until this patch is applied to Metacity as well.)

Did we add something that lets us override schema defaults from inside gnome-shell?

>+  XkbKeyPtr best_key;

needs to be initialized to NULL

>+  /* We need only the Names and the Geometry, but asking for these results
>+   * in the Keyboard information retrieval failing for unknown reasons.

IIRC from Xgl bugs years ago, calling XkbGetKeyboard() ends up causing the server to write out a tmp file and then invoke xkbcomp on it, and xkbcomp has various constraints on valid input, which XkbGetKeyboard itself doesn't make any attempt to enforce. (The xkbcomp spawning is also why it's so slow...)

>+  /* There could potentially be multiple keys with the Tab keysym on the keyboard;
>+   * but XKeysymToKeycode() returns us the one that the alt-Tab binding will
>+   * use which is good enough */

trivial, but the comments elsewhere put the "*/" on its own line

>+   *  - In case of ties, has its horizontal center as close as possible
>+   *    to the Tab keys horizontal center

"Tab key's"

(In reply to comment #8)
> (From update of attachment 177451 [details] [review])
> >the default binding for cycle_group to <Alt>Above_Tab. (This will
> >have no effect for the normal case of getting the key binding from
> >GConf until this patch is applied to Metacity as well.)
> 
> Did we add something that lets us override schema defaults from inside
> gnome-shell?

Yes, but it doesn't work for key bindings.
> 
> >+  XkbKeyPtr best_key;
> 
> needs to be initialized to NULL

Weird that GCC didnt' warn.
 
> >+  /* We need only the Names and the Geometry, but asking for these results
> >+   * in the Keyboard information retrieval failing for unknown reasons.
> 
> IIRC from Xgl bugs years ago, calling XkbGetKeyboard() ends up causing the
> server to write out a tmp file and then invoke xkbcomp on it, and xkbcomp has
> various constraints on valid input, which XkbGetKeyboard itself doesn't make
> any attempt to enforce. (The xkbcomp spawning is also why it's so slow...)

Reading the xserver code, this does seem to be the case :-(

> >+  /* There could potentially be multiple keys with the Tab keysym on the keyboard;
> >+   * but XKeysymToKeycode() returns us the one that the alt-Tab binding will
> >+   * use which is good enough */
> 
> trivial, but the comments elsewhere put the "*/" on its own line

That actually is intentional to indicate the comment is tightly bound to the next line instead of more loosely bound to the next several lines, but probably too subtle. Changed.

Pushed with minor fixes noted.

Attachment 177451 [details] pushed as 4ea00e1 - Add an "Above_Tab" pseudo-keysym

Also pushed patch with minimal port-over changes to Metacity.

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 177451 [details] [review] [details])
> > >the default binding for cycle_group to <Alt>Above_Tab. (This will
> > >have no effect for the normal case of getting the key binding from
> > >GConf until this patch is applied to Metacity as well.)
> > 
> > Did we add something that lets us override schema defaults from inside
> > gnome-shell?
> 
> Yes, but it doesn't work for key bindings.

Does this mean we have to add plain metacity to the jhbuild config now?