Created attachment 123842 [details] [review]
Make MetaDisplay a real GObject

We need this to be able to add signals, among other things.

Created attachment 123843 [details] [review]
Add support for a "meta key" which initiates extended WM operations

This patch adds the concept of a special key for WM operations, and
the default is Super_L, which on extended PC hardware is the
"Windows key".  What we do is handle the special case of a press
and release of this key (without any other intervening keys).

Super_L+<key> should still be passed to applications.  In the future
we may want to also take some of these keybindings (e.g. Super+TAB)
though.

Created attachment 123892 [details] [review]
Add support for a "meta key" which initiates extended WM operations

This patch adds the concept of a special key for WM operations, and
the default is Super_L, which on extended PC hardware is the
"Windows key".  What we do is handle the special case of a press
and release of this key (without any other intervening keys).

Super_L+<key> should still be passed to applications.  In the future
we may want to also take some of these keybindings (e.g. Super+TAB)
though.

This second version correctly calls XGrabKey as the other binding code does.

Comment on attachment 123842 [details] [review]
Make MetaDisplay a real GObject

To correspond to what we've done elsewhere:

 - MetaDisplayClass definition should be in display-private.h
 - MetaDisplay parent member should be parent_instance

Otherwise look fine

    Reviewing the other patch:

    +      display->meta_key_combo.keycode 
    +        = XKeysymToKeycode(display->xdisplay, display->meta_key_combo.keysym);

    Minor style - missing ' ' before (. Also, maybe linebreak the same as the
    other similar lines?

    +  display_signals[META_KEY] =
    +    g_signal_new ("meta-key",
    +                  G_TYPE_FROM_CLASS (klass),
    +                  G_SIGNAL_RUN_LAST,
    +                  0,
    +                  NULL, NULL,
    +                  g_cclosure_marshal_VOID__VOID,
    +                  G_TYPE_NONE, 0);  

    Might I suggest that we not add something called "meta-key" in the code
    that starts off with a default value of Super_L! Considering the traditional
    X "Meta" keysym, that's like having a "shift-key" with a default value of
    Control_L.

    +  if (combo.keysym != None || combo.keycode != 0)
    +    {
    +      display->meta_key_combo = combo;
    +    }

    Doesn't handle unsetting the display value if the GConf value gets unset - note
    rebuild_binding_table starts off with a g_new0() each time.

    @@ -605,6 +625,7 @@ bindings_changed_callback (MetaPreference pref,
         {
         case META_PREF_SCREEN_KEYBINDINGS:
           rebuild_screen_binding_table (display);
    +      rebuild_special_screen_bindings (display); 

    Don't see how this works - doesn't META_PREF_SCREEN_KEYBINDINGS only
    get queued if a key is changed that starts with KEY_SCREEN_BINDINGS_PREFIX
    "/apps/metacity/global_keybindings" (and shouldn't the preference be there
    anyways?) Or looking down:

    +  else if (g_str_equal (key, KEY_META_KEY))
    +    {
    +      queue_changed (META_PREF_WINDOW_KEYBINDINGS);      
    +    }

    Was this meant to be SCREEN_KEYBINDINGS?

    A comment somewhere about why the "meta-key" is "special" and not "screen"
    would be quite helpful.

    I'm not convinced that the grab handling is correct - very roughly,
    to get the the behavior you've described:

     - We grab the key with keyboard_mode=GrabModeSync (already works that way)
     - When we get the key, we call AllowEvents with SyncKeyboard (not the
       AsyncKeyboard that we call currently)... that means, "give me one
       more event".
     - If the next key event we get is a KeyRelease for the MetaDisplay
       then activate the overlay
     - Otherwise call AllowEvents with ReplayKeyboard, which will cause
       the event to be redelivered.

    I'm a bit fuzzy both on the metacity keybinding handling, and the 
    exact semantics of XAllowEvents(), so that might take some trial and
    error. (For XAllowEvents(), see the protocol docs from 
    /usr/share/doc/xorg-docs-1.3-3.fc10/hardcopy/XProtocol/proto.pdf, man
    page is not useful.)

    +    }
    +  return TRUE;
    +}

    Missing newline.

    +  init_special_screen_bindings ();
    +    
    +  {
    +       char *val = gconf_client_get_string (default_client, KEY_META_KEY,
    &err);
    +    cleanup_error (&err);
    +    
    +    if (val && meta_ui_parse_accelerator (val, &meta_key_combo.keysym, 
    +                                      &meta_key_combo.keycode, 
    +                                      &meta_key_combo.modifiers))
    +      ;
    +    else
    +      {
    +        meta_topic (META_DEBUG_KEYBINDINGS,
    +                    "Failed to parse value for meta_key\n");
    +      }
    +    g_free (val);
    +  }

    A) Indentation for char *val. B) Don't understand why you put some stuff
    in init_special_screen_bindings() and other code inline. C) Don't think
    the standalone block fits in with the general character of the Metacity
    coding style.

    +         <short>Modifier to use for extended window management
    operations</short>
    +         <long>
    +                       This modifier will initiate various window operations;
    however all
    +                       operations from this key should be viewed as optional,
    because the default
    +                       key may not be available on target hardware.  This key
    will
    +                       have particular behavior when pressed and released
    alone, as 
    +                       well as defined operations when pressed in combination
    with
    +                       selected other keys such as TAB.
    +                       
    +                       It's expected that this binding either the default or
    set to 
    +                       the empty string.
    +         </long>
    +      </locale>

    I find that a little confusing. Think about reading those docs from the
    perspective of a sysadmin configuring things for his users. What are
    "various window operations"? What does the information "should be viewed
    as optional" mean to that sysadmin? 

    How does this relate to the "Movement Key" setting in gnome-window-properties?

    And why "default or the empty string" ... what if (say on Solaris) the
    available key is Meta_L not Super_L?

Created attachment 123914 [details] [review]
overlay key

Thanks for the review.  I've changed the name to "overlay" instead of meta.  Fixed a chunk of the comments.  On stuff I didn't fix:

* Grab behavior - Hm, right, needs analysis.
* Solaris - you mean Sun keyboards?  Well...maybe we need some sort of configure option for the "expected" keyboard?  Or we could try to determine it dynamically, though that seems like it would be hard.

commit c26534ec795f662d4f2e9b7c49e7649d8823b118
Author: Colin Walters <walters@verbum.org>
Date:   Tue Dec 2 18:13:11 2008 -0500

    Add support for a "meta key" which initiates extended WM operations
    
    This patch adds the concept of a special key for WM operations, and
    the default is Super_L, which on extended PC hardware is the
    "Windows key".  What we do is handle the special case of a press
    and release of this key (without any other intervening keys).
    
    Super_L+<key> should still be passed to applications.  In the future
    we may want to also take some of these keybindings (e.g. Super+TAB)
    though.
    
    http://bugzilla.gnome.org/show_bug.cgi?id=563047