symbols: Skip interprets only for groups with explicit actions

Previously setting explicit actions for a group in symbols files made
the parser skip compatibility interpretations for the corresponding
*whole* key, so the other groups with *no* explicit actions could result
broken on some levels.

In the following example, `<RALT>` would have an action on group 2,
because it is explicit, but none on group 1 because interpretation are
also skipped there as a side effect:

```c
key <RALT> {
	symbols[1]= [              ISO_Level3_Shift ],
	symbols[2]= [              ISO_Level3_Shift ],
	actions[2]= [ SetMods(modifiers=LevelThree) ]
};
```

Fixed by skipping interpretations *only* for groups with explicit actions.

We still set `key->explicit |= EXPLICIT_INTERP` if at least one group
has explicit actions. In such case, when dumping a keymap, we will
write explicit actions for *all* groups, in order to ensure that X11 and
previous versions of libxkbcommon can parse the keymap as intended. One
side effect is that no interpretation will be run on this key anymore,
so we may have to set some extra fields explicitly: repeat, virtualMods.
Thus the previous example would be bumped as:

```c
key <RALT> {
	repeat= No,
	symbols[1]= [              ISO_Level3_Shift ],
	actions[1]= [ SetMods(modifiers=LevelThree,clearLocks) ],
	symbols[2]= [              ISO_Level3_Shift ],
	actions[2]= [ SetMods(modifiers=LevelThree) ]
};
```

changes/api/511.explicit-interp-per-group.bugfix.md

@@ -0,0 +1,6 @@
+Previously, setting *explicit actions* for a group in symbols files made the parser
+skip compatibility interpretations for *all* the groups in the corresponding key,
+resulting in possibly broken groups with *no* explicit actions or missing key fields.
+
+Fixed by skipping interpretations only for groups with explicit actions when parsing
+a keymap and setting relevant fields explicitly when serializing a keymap to a string.

meson.build

@@ -738,7 +738,13 @@ test(
 )
 test(
     'stringcomp',
-    executable('test-stringcomp', 'test/stringcomp.c', dependencies: test_dep),
+    executable(
+        'test-stringcomp',
+        'test/stringcomp.c',
+        'test/utils-text.c',
+        'test/utils-text.h',
+        dependencies: test_dep
+    ),
     env: test_env,
 )
 test(
@@ -787,10 +793,10 @@ test(
     executable(
         'test-compose',
         'test/compose.c',
-        'test/utils-text.c',
-        'test/utils-text.h',
         'test/compose-iter.c',
         'test/compose-iter.h',
+        'test/utils-text.c',
+        'test/utils-text.h',
         'src/compose/dump.c',
         'src/compose/dump.h',
         'src/compose/escape.h',

src/keymap.h

@@ -324,11 +324,28 @@ struct xkb_level {
     } u;
 };
 
+/**
+ * Group in a key
+ */
 struct xkb_group {
-    bool explicit_type;
-    /* Points to a type in keymap->types. */
+    /**
+     * Flag that indicates whether a group has explicit actions. In case it has,
+     * compatibility interpretations will not be used on it.
+     * See also EXPLICIT_INTERP flag at key level.
+     */
+    bool explicit_actions:1;
+    /**
+     * Flag that indicates whether a group has an explicit key type. In case it
+     * has, type detection will not be used on it.
+     */
+    bool explicit_type:1;
+    /**
+     * Key type of the group. Points to an entry in keymap->types.
+     */
     const struct xkb_key_type *type;
-    /* Use XkbKeyNumLevels for the number of levels. */
+    /**
+     * Array of group levels. Use `XkbKeyNumLevels` for the number of levels.
+     */
     struct xkb_level *levels;
 };
 

src/x11/keymap.c

@@ -602,8 +602,13 @@ get_explicits(struct xkb_keymap *keymap, xcb_connection_t *conn,
         if ((wire->explicit & XCB_XKB_EXPLICIT_KEY_TYPE_4) &&
             key->num_groups > 3)
             key->groups[3].explicit_type = true;
-        if (wire->explicit & XCB_XKB_EXPLICIT_INTERPRET)
+        if (wire->explicit & XCB_XKB_EXPLICIT_INTERPRET) {
             key->explicit |= EXPLICIT_INTERP;
+            /* Make all key groups have explicit actions too */
+            for (xkb_layout_index_t l = 0; l < key->num_groups; l++) {
+                key->groups[l].explicit_actions = true;
+            }
+        }
         if (wire->explicit & XCB_XKB_EXPLICIT_AUTO_REPEAT)
             key->explicit |= EXPLICIT_REPEAT;
         if (wire->explicit & XCB_XKB_EXPLICIT_V_MOD_MAP)

src/xkbcomp/keymap-dump.c

@@ -531,15 +531,30 @@ write_key(struct xkb_keymap *keymap, struct buf *buf,
         }
     }
 
-    if (key->explicit & EXPLICIT_REPEAT) {
+    /*
+     * NOTE: we use key->explicit and not key->group[i].explicit_actions, in
+     * order to have X11 and the previous versions of libxkbcommon (without this
+     * group property) parse the keymap as intended, by setting explicitly for
+     * this key all actions in all groups.
+     *
+     * One side effect is that no interpretation will be run on this key anymore,
+     * so we may have to set some extra fields explicitly: repeat, virtualMods.
+     */
+    show_actions = (key->explicit & EXPLICIT_INTERP);
+
+    /* If we show actions, interprets are not going to be used to set this
+     * field, so make it explicit. */
+    if ((key->explicit & EXPLICIT_REPEAT) || show_actions) {
         if (key->repeats)
             write_buf(buf, "\n\t\trepeat= Yes,");
         else
             write_buf(buf, "\n\t\trepeat= No,");
         simple = false;
     }
 
-    if (key->vmodmap && (key->explicit & EXPLICIT_VMODMAP))
+    /* If we show actions, interprets are not going to be used to set this
+     * field, so make it explicit. */
+    if (key->vmodmap && ((key->explicit & EXPLICIT_VMODMAP) || show_actions))
         write_buf(buf, "\n\t\tvirtualMods= %s,",
                   ModMaskText(keymap->ctx, &keymap->mods, key->vmodmap));
 
@@ -557,8 +572,6 @@ write_key(struct xkb_keymap *keymap, struct buf *buf,
         break;
     }
 
-    show_actions = (key->explicit & EXPLICIT_INTERP);
-
     if (key->num_groups > 1 || show_actions)
         simple = false;
 
@@ -584,8 +597,8 @@ write_key(struct xkb_keymap *keymap, struct buf *buf,
                     if (level != 0)
                         write_buf(buf, ", ");
                     write_action(keymap, buf,
-                                    &key->groups[group].levels[level].action,
-                                    NULL, NULL);
+                                 &key->groups[group].levels[level].action,
+                                 NULL, NULL);
                 }
                 write_buf(buf, " ]");
             }

src/xkbcomp/keymap.c

@@ -133,11 +133,11 @@ ApplyInterpsToKey(struct xkb_keymap *keymap, struct xkb_key *key)
     xkb_layout_index_t group;
     xkb_level_index_t level;
 
-    /* If we've been told not to bind interps to this key, then don't. */
-    if (key->explicit & EXPLICIT_INTERP)
-        return true;
-
     for (group = 0; group < key->num_groups; group++) {
+        /* Skip any interpretation for this group if it has explicit actions */
+        if (key->groups[group].explicit_actions)
+            continue;
+
         for (level = 0; level < XkbKeyNumLevels(key, group); level++) {
             const struct xkb_sym_interpret *interp;
 

src/xkbcomp/symbols.c

@@ -1494,8 +1494,13 @@ CopySymbolsDefToKeymap(struct xkb_keymap *keymap, SymbolsInfo *info,
     }
 
     /* Copy levels. */
-    darray_enumerate(i, groupi, keyi->groups)
+    darray_enumerate(i, groupi, keyi->groups) {
         darray_steal(groupi->levels, &key->groups[i].levels, NULL);
+        if (groupi->defined & GROUP_FIELD_ACTS) {
+            key->groups[i].explicit_actions = true;
+            key->explicit |= EXPLICIT_INTERP;
+        }
+    }
 
     key->out_of_range_group_number = keyi->out_of_range_group_number;
     key->out_of_range_group_action = keyi->out_of_range_group_action;
@@ -1510,13 +1515,6 @@ CopySymbolsDefToKeymap(struct xkb_keymap *keymap, SymbolsInfo *info,
         key->explicit |= EXPLICIT_REPEAT;
     }
 
-    darray_foreach(groupi, keyi->groups) {
-        if (groupi->defined & GROUP_FIELD_ACTS) {
-            key->explicit |= EXPLICIT_INTERP;
-            break;
-        }
-    }
-
     return true;
 }
 

test/compose.c

@@ -34,8 +34,8 @@
 #include "src/compose/parser.h"
 #include "src/compose/escape.h"
 #include "src/compose/dump.h"
-#include "test/utils-text.h"
 #include "test/compose-iter.h"
+#include "test/utils-text.h"
 
 static const char *
 compose_status_string(enum xkb_compose_status status)