Created attachment 282585 [details] [review]
backends: Add methods to handle keymaps

These are methods to set and get xkbcommon keymaps as well as locking
a specific layout in a layout group.

With this, we introduce dependencies on xkeyboard-config, xkbfile,
xkbcommon-x11 and an libX11 new enough to have xcb support.

Created attachment 282586 [details] [review]
wayland-keyboard: Use the backend's keymap

Instead of getting it from xwayland, let's just keep a reference to
the backend's keymap.

Created attachment 282587 [details] [review]
backends: Make MetaBackend available to introspection

This moves meta-backend.h under meta/ and, for now, just exposes to
introspection the methods that we actually need in gnome-shell.

Created attachment 282588 [details] [review]
Use libX11's Xkb* API unconditionally

At this point there shouldn't be any system capable of running mutter
that doesn't have it and we're introducing functionality like setting
the keymap that has an hard requirement on it.

Review of attachment 282585 [details] [review]:

::: src/backends/meta-backend-private.h
@@ +31,3 @@
 #include "meta-backend.h"
 
+#ifndef XKB_RULES_FILE

Is something else supposed to define this? `grep XKB_RULES_FILE /usr/include/` didn't come up with anything. If not, mind making it DEFAULT_XKB_RULES_FILE and dropping the ifndef's?

@@ +35,3 @@
+#endif
+#ifndef XKB_LAYOUT
+#define XKB_LAYOUT "us"

meta-backend-native doesn't use this. I'm a bit confused about what's supposed to come from where.

@@ +38,3 @@
+#endif
+#ifndef XKB_MODEL
+#define XKB_MODEL "pc105+inet"

Clutter's evdev manager uses "pc105", not "+inet", by default. What's the difference, and who's right?

::: src/backends/x11/meta-backend-x11.c
@@ +450,3 @@
+
+  /* Get it from the X property or fallback on defaults */
+  if (!XkbRF_GetNamesProp (xdisplay, rules, *var_defs) || !*rules)

Is there ever a valid case where GetNamesProp returns TRUE but returns a NULL rules?

@@ +462,3 @@
+
+  if (*rules[0] == '/')
+    *rules = g_strdup (*rules);

This is super messy. Can you do:

  char *rules = NULL;

  if (!XkbRF_GetNamesProp (xdisplay, &rules, *var_defs) || !rules)
    {
      rules = strdup (XKB_RULES_FILE);
      ...
    }

  /* Sometimes, the property is a filename, and sometimes it's not. Normalize it so it's always a file. */
  if (rules[0] == '/')
    *rules_p = g_strdup (rules);
  else
    *rules_p = g_build_filename (XKB_BASE, "rules", rules, NULL);

  free (rules);

Also, this API is terrible. What the hell?

Review of attachment 282586 [details] [review]:

Looks good.

Review of attachment 282587 [details] [review]:

::: src/backends/meta-backend.h
@@ +36,3 @@
 
+typedef struct _MetaMonitorManager        MetaMonitorManager;
+typedef struct _MetaCursorRenderer        MetaCursorRenderer;

If we don't want to expose these classes, we shouldn't expose their accessors. It's a bit weird that we already have meta-backend.h and meta-backend-private.h. Perhaps we should have a meta/meta-backend.h, for public JS stuff, a backends/meta-backend-private.h, which is where the other stuff is, and a backends/meta-backend-internal.h, which is where the implementation-specific stuff is.

I'm not sure I like that one, though.

Review of attachment 282588 [details] [review]:

OK.

Created attachment 282716 [details] [review]
backends: Add methods to handle keymaps

Created attachment 282717 [details] [review]
backends: Make MetaBackend available to introspection

Review of attachment 282716 [details] [review]:

::: src/backends/x11/meta-backend-x11.c
@@ +540,3 @@
+  gchar *rules_file_path;
+
+  get_xkbrf_var_defs (priv->xdisplay, &rules_file_path, &xkb_var_defs);

Is there a reason the VarDefsRec isn't on the stack?

@@ +543,3 @@
+
+  free (xkb_var_defs->layout);
+  xkb_var_defs->layout = strdup (layouts);

OK, so DEFAULT_XKB_LAYOUT is actually never used. I missed this before. You should drop the constant here then.

@@ +547,3 @@
+  xkb_var_defs->variant = strdup (variants);
+  free (xkb_var_defs->options);
+  xkb_var_defs->options = strdup (options);

OK, so what we're really doing here is building a vardefs with the proper definitions. I'd do:

    static void
    get_xkbrf_var_refs (Display *xdisplay,
                        const char *layouts,
                        const char *variants,
                        const char *options,
                        char **rules_file_path,
                        XkbRF_VarDefsRec *vardefs)
    {
        /* Fetch the existing rules used. */
        if (!XkbRF_GetNamesProp(...))
          {
            /* Yeesh, no existing property. Just fall back to some hardcoded defaults. */
            ...
          }

        /* Swap in our new options... */
        free (prop->layouts);
        prop->layouts = strdup (layouts);
        ...
    }

This means we don't have to do some useless strdup's for blank strings in some places, and have a confusing constant we'll never use.

@@ +550,3 @@
+
+  xkb_rules = XkbRF_Load (rules_file_path, NULL, True, True);
+  if (xkb_rules)

if (!xkb_rules)
  {
    g_warning (...);
    return;
  }

@@ +553,3 @@
+    {
+      XkbComponentNamesRec *xkb_comp_names;
+      xkb_comp_names = g_new0 (XkbComponentNamesRec, 1);

Is there a reason we don't simply have this on the stack as well?

(In reply to comment #5)
> Review of attachment 282585 [details] [review]:
> 
> ::: src/backends/meta-backend-private.h
> @@ +31,3 @@
>  #include "meta-backend.h"
> 
> +#ifndef XKB_RULES_FILE
> 
> Is something else supposed to define this? `grep XKB_RULES_FILE /usr/include/`
> didn't come up with anything. If not, mind making it DEFAULT_XKB_RULES_FILE and
> dropping the ifndef's?

Yeah, I got these defines originally from setxkbmap.c and didn't give it much thought. Done

> @@ +35,3 @@
> +#endif
> +#ifndef XKB_LAYOUT
> +#define XKB_LAYOUT "us"
> 
> meta-backend-native doesn't use this. I'm a bit confused about what's supposed
> to come from where.

Native doesn't need this because it can assume that clutter-evdev always has a keymap. If you look in clutter-evdev, you'll see that "us" is the default and initial value for this there.

> @@ +38,3 @@
> +#endif
> +#ifndef XKB_MODEL
> +#define XKB_MODEL "pc105+inet"
> 
> Clutter's evdev manager uses "pc105", not "+inet", by default. What's the
> difference, and who's right?

Short answer: it doesn't really matter because we'll always set our own keymap (from gnome-shell) over whatever keymap clutter-evdev initializes with. Although it would be nice to have clutter initialize the correct one from the very beginning to reduce the amount of work we do while going up...

Long answer:

An XKB keymap is an assembly of components. These components have types like Keycodes, Symbols or Geometry.

Geometry is basically irrelevant for the actual functionality but I *think* it needs to be there. Other components specify which keysyms are derived from a certain keycode+modifiers combo.

Then, there is a thing called rules. Rules are "recipes" of how to build a keymap, i.e. a set of components, from something that is more Human friendly: Model, Layouts, Variants and Options.

For instance, in the XKB data, the "us" symbols component only defines what regular keys do like:

    key <TLDE> {        [     grave,    asciitilde      ]       };
    key <AE01> {        [         1,    exclam          ]       };
...

Then, the "inet" symbols component defines extra stuff like:

    key <MUTE>   {      [ XF86AudioMute         ]       };
    key <VOL->   {      [ XF86AudioLowerVolume  ]       };
    key <VOL+>   {      [ XF86AudioRaiseVolume  ]       };
    key <POWR>   {      [ XF86PowerOff          ]       };
...

So, you actually want both merged. The Model part of a Rule, allows us to specify the components that don't usually change when you switch layouts, like the geometry ("pc105") and the "inet" symbols. The Layout, Variant and Options parts, are what end users usually want to switch.

I hope I was clear. This is all kind of complicated and the XKB specification doesn't do a good job of explaining this stuff IMO.

> ::: src/backends/x11/meta-backend-x11.c
> @@ +450,3 @@
> +
> +  /* Get it from the X property or fallback on defaults */
> +  if (!XkbRF_GetNamesProp (xdisplay, rules, *var_defs) || !*rules)
> 
> Is there ever a valid case where GetNamesProp returns TRUE but returns a NULL
> rules?

I lifted this code from http://cgit.freedesktop.org/xorg/app/setxkbmap/tree/setxkbmap.c#n598

The implementation seems to indeed do that in a corner case which we probably don't hit, but this doesn't cost us much IMO. See http://cgit.freedesktop.org/xorg/lib/libxkbfile/tree/src/maprules.c#n1398

> @@ +462,3 @@
> +
> +  if (*rules[0] == '/')
> +    *rules = g_strdup (*rules);
> 
> This is super messy. Can you do:

Sure.

> Also, this API is terrible. What the hell?

Unfortunately yes.

Created attachment 282723 [details] [review]
backends: Add methods to handle keymaps

(In reply to comment #11)
> Review of attachment 282716 [details] [review]:
> 
> ::: src/backends/x11/meta-backend-x11.c
> @@ +540,3 @@
> +  gchar *rules_file_path;
> +
> +  get_xkbrf_var_defs (priv->xdisplay, &rules_file_path, &xkb_var_defs);
> 
> Is there a reason the VarDefsRec isn't on the stack?

It can be on the stack. Done

> @@ +543,3 @@
> +
> +  free (xkb_var_defs->layout);
> +  xkb_var_defs->layout = strdup (layouts);
> 
> OK, so DEFAULT_XKB_LAYOUT is actually never used. I missed this before. You
> should drop the constant here then.

Well yeah, but I need to put something there for the !XkbRF_GetNamesProp(), because when GetNamesProp() actually works it allocates the memory for those strings so, always duping something makes it simpler to swap in the user's layouts, variants and options. It could be "" but it would still have to be duped.

> @@ +547,3 @@
> +  xkb_var_defs->variant = strdup (variants);
> +  free (xkb_var_defs->options);
> +  xkb_var_defs->options = strdup (options);
> 
> OK, so what we're really doing here is building a vardefs with the proper
> definitions. I'd do:
> 
>     static void
>     get_xkbrf_var_refs (Display *xdisplay,
>                         const char *layouts,
>                         const char *variants,
>                         const char *options,
>                         char **rules_file_path,
>                         XkbRF_VarDefsRec *vardefs)
>     {
>         /* Fetch the existing rules used. */
>         if (!XkbRF_GetNamesProp(...))
>           {
>             /* Yeesh, no existing property. Just fall back to some hardcoded
> defaults. */
>             ...
>           }
> 
>         /* Swap in our new options... */
>         free (prop->layouts);
>         prop->layouts = strdup (layouts);
>         ...
>     }
> 
> This means we don't have to do some useless strdup's for blank strings in some
> places, and have a confusing constant we'll never use.

Ok

> @@ +550,3 @@
> +
> +  xkb_rules = XkbRF_Load (rules_file_path, NULL, True, True);
> +  if (xkb_rules)
> 
> if (!xkb_rules)
>   {
>     g_warning (...);
>     return;
>   }

Not worth it since we still need to free the strings in the xkb_var_defs struct. Could be a goto but.. meh.

> @@ +553,3 @@
> +    {
> +      XkbComponentNamesRec *xkb_comp_names;
> +      xkb_comp_names = g_new0 (XkbComponentNamesRec, 1);
> 
> Is there a reason we don't simply have this on the stack as well?

It can. But it's not much of a win since we still need to free all the strings inside the struct. But sure.

(In reply to comment #14)
> Well yeah, but I need to put something there for the !XkbRF_GetNamesProp(),
> because when GetNamesProp() actually works it allocates the memory for those
> strings so, always duping something makes it simpler to swap in the user's
> layouts, variants and options. It could be "" but it would still have to be
> duped.

Why not just leave them as NULL? free(NULL); is a no-op.

Created attachment 282726 [details] [review]
backends: Add methods to handle keymaps

--

(In reply to comment #15)
> Why not just leave them as NULL? free(NULL); is a no-op.

I thought it crashed. Ok, amended

Review of attachment 282726 [details] [review]:

Looks good to me.

::: src/backends/meta-backend.c
@@ +224,3 @@
+}
+
+

Extra whitespace.

Review of attachment 282717 [details] [review]:

::: src/backends/meta-backend.c
@@ +155,3 @@
+/**
+ * meta_backend_get_idle_monitor: (skip)
+ */

These don't need the comments anymore, but meh, I don't particularly care.

::: src/core/window.c
@@ +59,3 @@
 #include "wayland/meta-wayland-private.h"
 
+#include "meta-backend-private.h"

backends/meta-backend-private.h

Try not to rely on the implicit -Ibackends in CFLAGS.

::: src/wayland/meta-wayland-keyboard.c
@@ +58,3 @@
 #include <sys/mman.h>
 
+#include "meta-backend-private.h"

Here as well.

All amended and pushed. Thanks

Attachment 282586 [details] pushed as 513628e - wayland-keyboard: Use the backend's keymap
Attachment 282588 [details] pushed as 6af48de - Use libX11's Xkb* API unconditionally
Attachment 282717 [details] pushed as 7d54631 - backends: Make MetaBackend available to introspection
Attachment 282726 [details] pushed as 101b215 - backends: Add methods to handle keymaps