Allow only the first group in symbols sections when using RMLVO

Currently `xkb_keymap_num_layouts` may return a greater number than the number of layouts configured using RMLVO, because we allow symbols sections to define various groups per key. This is unintuitive and kind of buggy: groups should be added via rules by setting an explicit `:n` modifier. Fix: when parsing a keymap using RMLVO resolution: - Get the expected layouts count from the resulting KcCGST. - Drop the groups after the first one in included symbols sections. This will ensure that a symbol section can only define one group per key. Notes: - Compiling a keymap string directly is unaffected. - RMLVO resolution may still produce more groups than the input layouts. Indeed, some legacy rules in xkeyboard-config rely on this to insert automatically a US layout before the given non-Latin one, resulting in two layouts while only one was given.
xkbcommon · Oct 2, 2024 · 8bb26a1 · 8bb26a1
1 parent bf1ea08
commit 8bb26a1
Show file tree

Hide file tree

Showing 13 changed files with 278 additions and 93 deletions.
diff --git a/bench/rules.c b/bench/rules.c
@@ -53,7 +53,7 @@ main(int argc, char *argv[])
     for (i = 0; i < BENCHMARK_ITERATIONS; i++) {
         struct xkb_component_names kccgst;
 
-        assert(xkb_components_from_rules(ctx, &rmlvo, &kccgst));
+        assert(xkb_components_from_rules(ctx, &rmlvo, &kccgst, NULL));
         free(kccgst.keycodes);
         free(kccgst.types);
         free(kccgst.compat);

diff --git a/changes/api/262.only-one-group-per-key.bugfix.md b/changes/api/262.only-one-group-per-key.bugfix.md
@@ -0,0 +1,6 @@
+`xkb_keymap_new_from_names`: Allow only one group per key in symbols sections.
+While the original issue was [fixed in `xkeyboard-config`](https://gitlab.freedesktop.org/xkeyboard-config/xkeyboard-config/-/merge_requests/253)
+project, the previous handling in `libxkbcommon` of extra key groups was deemed unintuitive.
+
+Note: rules resolution may still produce more groups than the input layouts.
+This is currently true for some [legacy rules in `xkeyboard-config`](https://gitlab.freedesktop.org/xkeyboard-config/xkeyboard-config/-/blob/6a2eb9e63bcb3c52584580570d31cd91110d1f2e/rules/0013-modellayout_symbols.part#L2).
diff --git a/src/keymap.h b/src/keymap.h
@@ -387,7 +387,9 @@ struct xkb_keymap {
 
     struct xkb_mod_set mods;
 
-    /* Number of groups in the key with the most groups. */
+    /* This field has 2 uses:
+     * • During parsing: Expected layouts count after RMLVO resolution, if any;
+     * • After parsing: Number of groups in the key with the most groups. */
     xkb_layout_index_t num_groups;
     /* Not all groups must have names. */
     xkb_layout_index_t num_group_names;

diff --git a/src/xkbcomp/rules.c b/src/xkbcomp/rules.c
@@ -1536,7 +1536,8 @@ read_rules_file(struct xkb_context *ctx,
 bool
 xkb_components_from_rules(struct xkb_context *ctx,
                           const struct xkb_rule_names *rmlvo,
-                          struct xkb_component_names *out)
+                          struct xkb_component_names *out,
+                          xkb_layout_index_t *explicit_layouts)
 {
     bool ret = false;
     FILE *file;
@@ -1594,6 +1595,23 @@ xkb_components_from_rules(struct xkb_context *ctx,
                     "Unrecognized RMLVO option \"%.*s\" was ignored\n",
                     mval->sval.len, mval->sval.start);
 
+    /* Set the number of explicit layouts */
+    if (out->symbols != NULL && explicit_layouts != NULL) {
+        *explicit_layouts = 1; /* at least one group */
+        const char *symbols = out->symbols;
+        /* Take the highest modifier */
+        while ((symbols = strchr(symbols, ':')) != NULL && symbols[1] != '\0') {
+            char *endptr;
+            unsigned long group = strtoul(++symbols, &endptr, 10);
+            /* Update only when valid group index, but continue parsing
+             * even on invalid ones, as we do not handle them here. */
+            if (group > 0 && group <= XKB_MAX_GROUPS)
+                *explicit_layouts = MAX(*explicit_layouts,
+                                        (xkb_layout_index_t)group);
+            symbols = endptr;
+        }
+    }
+
 err_out:
     if (file)
         fclose(file);

diff --git a/src/xkbcomp/rules.h b/src/xkbcomp/rules.h
@@ -27,7 +27,8 @@
 bool
 xkb_components_from_rules(struct xkb_context *ctx,
                           const struct xkb_rule_names *rmlvo,
-                          struct xkb_component_names *out);
+                          struct xkb_component_names *out,
+                          xkb_layout_index_t *explicit_layouts);
 
 /* Maximum length of a layout index string:
  * [NOTE] Currently XKB_MAX_GROUPS is 4, but the following code is

diff --git a/src/xkbcomp/symbols.c b/src/xkbcomp/symbols.c
@@ -598,7 +598,17 @@ HandleIncludeSymbols(SymbolsInfo *info, IncludeStmt *include)
                 next_incl.explicit_group = info->explicit_group;
             }
         }
+        else if (info->keymap->num_groups != 0 && next_incl.include_depth == 1) {
+            /* If keymap is the result of RMLVO resolution and we are at the
+             * first include depth, transform e.g. `pc` into `pc:1` in order to
+             * force only one group per key using the explicit group.
+             *
+             * NOTE: X11’s xkbcomp does not apply this rule. */
+            next_incl.explicit_group = 0;
+        }
         else {
+            /* The keymap was not generated from rules or this is not the first
+             * level of include: take the parent’s explicit group. */
             next_incl.explicit_group = info->explicit_group;
         }
 
@@ -1115,10 +1125,10 @@ SetExplicitGroup(SymbolsInfo *info, KeyInfo *keyi)
 
     if (warn) {
         log_warn(info->ctx, XKB_WARNING_MULTIPLE_GROUPS_AT_ONCE,
-                 "For the map %s an explicit group specified, "
+                 "For the map %s the explicit group %u is specified, "
                  "but key %s has more than one group defined; "
                  "All groups except first one will be ignored\n",
-                 info->name, KeyInfoText(info, keyi));
+                 info->name, info->explicit_group + 1, KeyInfoText(info, keyi));
     }
 
     darray_resize0(keyi->groups, info->explicit_group + 1);

diff --git a/src/xkbcomp/xkbcomp.c b/src/xkbcomp/xkbcomp.c
@@ -65,7 +65,10 @@ text_v1_keymap_new_from_names(struct xkb_keymap *keymap,
             rmlvo->rules, rmlvo->model, rmlvo->layout, rmlvo->variant,
             rmlvo->options);
 
-    ok = xkb_components_from_rules(keymap->ctx, rmlvo, &kccgst);
+    /* Resolve the RMLVO component to KcCGST components and get the
+     * expected number of layouts */
+    ok = xkb_components_from_rules(keymap->ctx, rmlvo, &kccgst,
+                                   &keymap->num_groups);
     if (!ok) {
         log_err(keymap->ctx, XKB_ERROR_KEYMAP_COMPILATION_FAILED,
                 "Couldn't look up rules '%s', model '%s', layout '%s', "

diff --git a/test/data/rules/multiple-groups b/test/data/rules/multiple-groups
@@ -0,0 +1,9 @@
+// Insert US layout before a single layout
+! model layout = symbols
+  *     us     = pc+%l%(v)
+  *     *      = pc+us+%l%(v):2
+
+! option          = symbols
+  multiple-groups = +multiple-groups
+
+! include evdev
diff --git a/test/data/symbols/multiple-groups b/test/data/symbols/multiple-groups
@@ -0,0 +1,30 @@
+// Define a key with various groups at the same time.
+// Use RALT because the option `lv3:ralt_alt` used to have a similar
+// (buggy) implementation, that triggered an unexpected result
+// for `xkb_keymap_num_layouts` and `xkb_keymap_num_layouts_for_key`.
+default partial modifier_keys
+xkb_symbols "1" {
+  key <RALT> {
+    type[Group1]="ONE_LEVEL",
+    type[Group2]="ONE_LEVEL",
+    type[Group3]="ONE_LEVEL",
+    type[Group4]="ONE_LEVEL",
+    symbols[Group1] = [ a ],
+    symbols[Group2] = [ b ],
+    symbols[Group3] = [ c ],
+    symbols[Group4] = [ d ]
+  };
+};
+
+xkb_symbols "2" {
+	key <RALT> {
+	  type[Group1]="ONE_LEVEL",
+	  type[Group2]="ONE_LEVEL",
+	  type[Group3]="ONE_LEVEL",
+	  type[Group4]="ONE_LEVEL",
+	  symbols[Group1] = [ a ],
+	  symbols[Group2] = [ B ],
+	  symbols[Group3] = [ C ],
+	  symbols[Group4] = [ D ]
+	};
+      };
diff --git a/test/keymap.c b/test/keymap.c
@@ -35,7 +35,7 @@
 static void
 test_garbage_key(void)
 {
-    struct xkb_context *context = test_get_context(0);
+    struct xkb_context *context = test_get_context(CONTEXT_NO_FLAG);
     struct xkb_keymap *keymap;
     xkb_keycode_t kc;
     int nsyms;
@@ -78,7 +78,7 @@ test_garbage_key(void)
 static void
 test_keymap(void)
 {
-    struct xkb_context *context = test_get_context(0);
+    struct xkb_context *context = test_get_context(CONTEXT_NO_FLAG);
     struct xkb_keymap *keymap;
     xkb_keycode_t kc;
     const char *keyname;
@@ -150,14 +150,99 @@ test_keymap(void)
     xkb_context_unref(context);
 }
 
+static void
+test_no_extra_groups(void)
+{
+    struct xkb_keymap *keymap;
+    xkb_keycode_t kc;
+    const xkb_keysym_t *syms;
+    struct xkb_context *context = test_get_context(CONTEXT_NO_FLAG);
+    assert(context);
+
+    /* RMLVO: Legacy rules may add more layouts than the input RMLVO */
+    keymap = test_compile_rules(context, "multiple-groups",
+                                "old", "de", NULL, NULL);
+    assert(keymap);
+    kc = xkb_keymap_key_by_name(keymap, "AD01");
+    assert(kc != XKB_KEYCODE_INVALID);
+    /* 2 layouts, while only 1 was provided */
+    assert(xkb_keymap_num_layouts_for_key(keymap, kc) == 2);
+    assert(xkb_keymap_num_layouts(keymap) == 2);
+    xkb_keymap_unref(keymap);
+
+    /* RMLVO: “one group per key” in symbols sections */
+    const char *layouts[] = {"us", "us,us", "us,us,us", "us,us,us,us"};
+    for (xkb_layout_index_t k = 0; k < ARRAY_SIZE(layouts); k++) {
+        /* `multiple-groups` option defines 4 groups for a key */
+        keymap = test_compile_rules(context, "multiple-groups", NULL,
+                                    layouts[k], NULL, "multiple-groups");
+        assert(keymap);
+        kc = xkb_keymap_key_by_name(keymap, "RALT");
+        assert(kc != XKB_KEYCODE_INVALID);
+        /* Groups after the first one should be dropped */
+        assert(xkb_keymap_num_layouts_for_key(keymap, kc) == 1);
+        /* Here we expect RMLVO layouts count = keymap layouts count */
+        assert(xkb_keymap_num_layouts(keymap) == k + 1);
+        for (xkb_layout_index_t l = 0; l <= k; l++) {
+            assert(xkb_keymap_key_get_syms_by_level(keymap, kc, l, 0, &syms) == 1);
+            assert(syms[0] == XKB_KEY_a);
+            syms = NULL;
+        }
+        xkb_keymap_unref(keymap);
+    }
+
+    /* RMLVO: Ensure the rule “one group per key” in symbols sections works
+     * for the 2nd layout */
+    keymap = test_compile_rules(context, NULL, NULL,
+                                "multiple-groups,multiple-groups", "1,2", NULL);
+    assert(keymap);
+    kc = xkb_keymap_key_by_name(keymap, "RALT");
+    assert(kc != XKB_KEYCODE_INVALID);
+    assert(xkb_keymap_num_layouts_for_key(keymap, kc) == 2);
+    assert(xkb_keymap_num_layouts(keymap) == 2);
+    for (xkb_layout_index_t l = 0; l < 2; l++) {
+        assert(xkb_keymap_key_get_syms_by_level(keymap, kc, l, 0, &syms) == 1);
+        assert(syms[0] == XKB_KEY_a);
+        syms = NULL;
+    }
+    xkb_keymap_unref(keymap);
+
+    /* Same configuration as previous, but without using RMLVO resolution:
+     * We do accept more than one group per key in symbols sections, but only
+     * when not using a modifier: the second layout (`:2`) will have its extra
+     * groups dropped. */
+    const char keymap_str[] =
+        "xkb_keymap {"
+        "  xkb_keycodes { include \"evdev+aliases(qwerty)\" };"
+        "  xkb_types { include \"complete\" };"
+        "  xkb_compat { include \"complete\" };"
+        "  xkb_symbols { include \"pc+multiple-groups(1)+multiple-groups(2):2"
+                                  "+inet(evdev)\" };"
+        "};";
+    keymap = test_compile_string(context, keymap_str);
+    kc = xkb_keymap_key_by_name(keymap, "RALT");
+    assert(kc != XKB_KEYCODE_INVALID);
+    assert(xkb_keymap_num_layouts_for_key(keymap, kc) == 4);
+    assert(xkb_keymap_num_layouts(keymap) == 4);
+    xkb_keysym_t ref_syms[] = { XKB_KEY_a, XKB_KEY_a, XKB_KEY_c, XKB_KEY_d };
+    for (xkb_layout_index_t l = 0; l < 4; l++) {
+        assert(xkb_keymap_key_get_syms_by_level(keymap, kc, l, 0, &syms) == 1);
+        assert(syms[0] == ref_syms[l]);
+        syms = NULL;
+    }
+    xkb_keymap_unref(keymap);
+
+    xkb_context_unref(context);
+}
+
 #define Mod1Mask (1 << 3)
 #define Mod2Mask (1 << 4)
 #define Mod3Mask (1 << 5)
 
 static void
 test_numeric_keysyms(void)
 {
-    struct xkb_context *context = test_get_context(0);
+    struct xkb_context *context = test_get_context(CONTEXT_NO_FLAG);
     struct xkb_keymap *keymap;
     const struct xkb_key *key;
     xkb_keycode_t kc;
@@ -201,7 +286,7 @@ test_numeric_keysyms(void)
 static void
 test_multiple_keysyms_per_level(void)
 {
-    struct xkb_context *context = test_get_context(0);
+    struct xkb_context *context = test_get_context(CONTEXT_NO_FLAG);
     struct xkb_keymap *keymap;
     xkb_keycode_t kc;
     int keysyms_count;
@@ -237,6 +322,7 @@ main(void)
 
     test_garbage_key();
     test_keymap();
+    test_no_extra_groups();
     test_numeric_keysyms();
     test_multiple_keysyms_per_level();
 

diff --git a/test/rules-file-includes.c b/test/rules-file-includes.c
@@ -67,7 +67,7 @@ test_rules(struct xkb_context *ctx, struct test_data *data)
         fprintf(stderr, "Expecting: %s\t%s\t%s\t%s\n",
                 data->keycodes, data->types, data->compat, data->symbols);
 
-    if (!xkb_components_from_rules(ctx, &rmlvo, &kccgst)) {
+    if (!xkb_components_from_rules(ctx, &rmlvo, &kccgst, NULL)) {
         fprintf(stderr, "Received : FAILURE\n");
         return data->should_fail;
     }