Created attachment 270814 [details] [review]
keybindings: Make MetaKeyPref, MetaKeyCombo and MetaKeyHandler private

There's no need for these to be public and keeping them all together
in the same header makes it easier when reading the code.

Created attachment 270815 [details] [review]
keybindings: Rename MetaKeyPref.bindings to MetaKeyPref.combos

Let's call it what it really is, otherwise it's just confusing since
MetaKeyBinding also exists.

Created attachment 270816 [details] [review]
keybindings: Keep keybindings in an hash table instead of an array

This allows us to look for a match with an O(1) search instead of O(n)
which is nice, particularly when running as a wayland compositor in
which case we have to do this search for every key press event (as
opposed to only when our passive grab triggers in the X compositor
case).

We actually need two hash tables. On one we keep all the keybindings
themselves which allows us to add external grabs without constantly
re-allocating the array we were using previously.

The other hash table is an index of the keybindings in the first table
by their keycodes and mask which is how we actually match the key
press events. This second table thus needs to be rebuilt when the
keymap changes since keycodes have to be resolved then but since we're
only keeping pointers to the first table it's a fast operation.

Review of attachment 270815 [details] [review]:

OK.

Review of attachment 270814 [details] [review]:

Yeah.

Review of attachment 270816 [details] [review]:

::: src/core/keybindings.c
@@ +165,3 @@
+{
+  guint32 key = keycode & 0xffff;
+  return (key << 16) | (mask & 0xffff);

We take a 32-bit keycode and 32-bit mask, and then truncate them to 16-bits. Is it a guarantee that keycodes and masks will only ever be 16-bits? Can we make key_binding_key take uint16_t's instead?

@@ +1101,3 @@
 }
 
+struct ChangeKeygrabData

Please use typedef struct { ... } ChangeKeygrabData;

@@ +1114,3 @@
+                        gpointer data)
+{
+  struct ChangeKeygrabData *d = data;

Can you change this to data = user_data;

@@ +1329,3 @@
 
+  g_hash_table_add (display->key_bindings, binding);
+  foreach_binding_index (binding, binding, display);

If you want to use the foreach function outside of a function, please extract the common functionality. I thought this would actually iterate over all the bindings and rebuild the index.

So, we'd have:

    static void
    index_binding (MetaDisplay    *display,
                   MetaKeyBinding *binding)
    {
      /* ... */
    }

    static void
    foreach_binding_index (gpointer key,
                           gpointer value,
                           gpointer data)
    {
      MetaDisplay *display = META_DISPLAY (data);
      MetaKeyBinding *binding = (MetaKeyBinding *) value;
      index_binding (display, binding);
    }

(In a related note, I don't like the "foreach_" naming convention, because they imply an iteration. Perhaps index_binding_foreach is a bit better?)

@@ +1648,1 @@
                MetaScreen           *screen,

I wonder if the bindings / n_bindings cleanup makes sense as a prereq patch as well, considering it already has the MetaDisplay.

@@ +1657,3 @@
     return FALSE;
 
+  binding = display_get_keybinding (display,

I wonder if all the display_get_keybinding cleanups would make sense split out. Would certainly make the patch easier to read for me.

@@ +1696,2 @@
   meta_topic (META_DEBUG_KEYBINDINGS,
               "No handler found for this event in this binding table\n");

Heh, staring at it now, if we filter out a compositor keybinding, we'll log this.

We should probably remove this log, since it will probably drive us up the wall in a Wayland world where this is run on every event.

Also, the "not_found" label name doesn't really make sense in the compositor-filtered case. I can't really think of another one, though.

Perhaps break it out into a separate if statement and add a comment?

    /* If the compositor filtered out the keybindings, that
     * means they don't want the binding to trigger, so we do
     * the same thing as if the binding didn't exist. */
    if (meta_compositor_filter_keybinding (...))
      goto out;

I'll do that as a follow-up patch, since it was already like this to begin with.

Created attachment 270834 [details] [review]
keybinding: Use display_get_keybinding() instead of looping explicitly

Created attachment 270835 [details] [review]
keybindings: Keep keybindings in an hash table instead of an array

(In reply to comment #6)
> We take a 32-bit keycode and 32-bit mask, and then truncate them to 16-bits. Is
> it a guarantee that keycodes and masks will only ever be 16-bits? Can we make
> key_binding_key take uint16_t's instead?

I added a comment that hopefully explains the rationale. Also, I'd prefer to keep the arguments there as guint32 so that the whole concatenation is explicit and kept in a single place.

Note that Clutter events use a 16 bit value for keycodes and an enum for the modifiers (which is likely 32 bits since the biggest value in the enum is 1 << 30). XIDeviceEvent keeps both the keycode and the modifiers mask in 32 bit values.

> I wonder if the bindings / n_bindings cleanup makes sense as a prereq patch as
> well, considering it already has the MetaDisplay.

This one would just create pointless churn on its own, see rebuild_binding_table() for why.

> @@ +1657,3 @@
>      return FALSE;
> 
> +  binding = display_get_keybinding (display,
> 
> I wonder if all the display_get_keybinding cleanups would make sense split out.
> Would certainly make the patch easier to read for me.

Yeah, this makes sense. I've split it out now.
 
> @@ +1696,2 @@
>    meta_topic (META_DEBUG_KEYBINDINGS,
>                "No handler found for this event in this binding table\n");
> 
> Heh, staring at it now, if we filter out a compositor keybinding, we'll log
> this.
> 
> We should probably remove this log, since it will probably drive us up the wall
> in a Wayland world where this is run on every event.
> 
> Also, the "not_found" label name doesn't really make sense in the
> compositor-filtered case. I can't really think of another one, though.
> 
> Perhaps break it out into a separate if statement and add a comment?
> 
>     /* If the compositor filtered out the keybindings, that
>      * means they don't want the binding to trigger, so we do
>      * the same thing as if the binding didn't exist. */
>     if (meta_compositor_filter_keybinding (...))
>       goto out;
> 
> I'll do that as a follow-up patch, since it was already like this to begin
> with.

That makes sense. Thanks

Review of attachment 270834 [details] [review]:

A slightly expanded commit message here would be nice:

    Instead of looping over an array of keybindings to find the
    correct binding, just use display_get_keybinding().

    In the next commit, we'll change the array to be a hash map,
    so this helps the patch be cleaner.

Review of attachment 270835 [details] [review]:

Looks good.

Ammended the commit msg and pushed, thanks

Attachment 270814 [details] pushed as 91389c8 - keybindings: Make MetaKeyPref, MetaKeyCombo and MetaKeyHandler private
Attachment 270815 [details] pushed as 2b3fc74 - keybindings: Rename MetaKeyPref.bindings to MetaKeyPref.combos
Attachment 270834 [details] pushed as 266ac00 - keybindings: Use display_get_keybinding() instead of looping explicitly
Attachment 270835 [details] pushed as 7e8833a - keybindings: Keep keybindings in an hash table instead of an array