state: fix consumed modifier calculation

The current calculation is in short:
    entry ? (entry->mask & ~entry->preserve) : 0

This changes it be
    type->mask & ~(entry ? entry->preserve : 0)

This is what Xlib does. While less intuitive, it is actually more
correct, if you follow this deduction:

- The key group's type->mask defines which modifiers the key even cares
  about. The others are completely irrelevant (and in fact they are
  masked out from all sided in the level calculation). Example: NumLock
  for an alphabetic key.

- The type->mask, the mods which are not masked out, are *all* relevant
  (and in fact in the level calculation they must match *exactly* to the
  state). These mods affect which level is chosen for the key, whether
  they are active or not.

- Because the type->mask mods are all relevant, they must be considered
  as consumed by the calculation *even if they are not active*.

Therefore we use type->mask instead of entry->mask.

The second change is what happens when no entry is found: return 0 or
just take preserve to be 0? Let's consider an example, the basic type

    type "ALPHABETIC" {
        modifiers = Shift+Lock;
        map[Shift] = Level2;
        map[Lock] = Level2;
        level_name[Level1] = "Base";
        level_name[Level2] = "Caps";
    };

Suppose Shift+Lock is active - it doesn't match any entry, thus it gets
to level 0. The first interpretation would take them both to be
unconsumed, the second (new one) would take them both to be consumed.
This seems much better: Caps is active, and Shift disables it, they both
do something.

This change also fixes a pretty lousy bug (since 0.3.2), where Shift
appears to apparently *not* disable Caps. What actually happens is that
Caps is not consumed (see above) but active, thus the implicit
capitalization in get_one_sym() kicks in and capitalizes it anyway.

Reported-by: Davinder Pal Singh Bhamra
Signed-off-by: Ran Benita <ran234@gmail.com>

diff --git a/src/state.c b/src/state.c
index d130b8e7e79e72df0a76fb3a3e327f88e651a8be..0f9ea792641eca6243372fa616be929c6483543a 100644 (file)
--- a/src/state.c
+++ b/src/state.c
@@ -1276,18 +1276,24 @@ xkb_state_led_name_is_active(struct xkb_state *state, const char *name)
 static xkb_mod_mask_t
 key_get_consumed(struct xkb_state *state, const struct xkb_key *key)
 {
+    const struct xkb_key_type *type;
     const struct xkb_key_type_entry *entry;
+    xkb_mod_mask_t preserve;
     xkb_layout_index_t group;
 
     group = xkb_state_key_get_layout(state, key->keycode);
     if (group == XKB_LAYOUT_INVALID)
         return 0;
 
+    type = key->groups[group].type;
+
     entry = get_entry_for_key_state(state, key, group);
-    if (!entry)
-        return 0;
+    if (entry)
+        preserve = entry->preserve.mask;
+    else
+        preserve = 0;
 
-    return entry->mods.mask & ~entry->preserve.mask;
+    return type->mods.mask & ~preserve;
 }
 
 /**

diff --git a/test/common.c b/test/common.c
index 8b3f954b532f4d9a963f32df6912c4cd5bdaeb4a..e6cd1c3674f6c037d156e04ce089eab853bf2f50 100644 (file)
--- a/test/common.c
+++ b/test/common.c
@@ -64,6 +64,7 @@ test_key_seq_va(struct xkb_keymap *keymap, va_list ap)
     xkb_keysym_t keysym;
 
     const xkb_keysym_t *syms;
+    xkb_keysym_t sym;
     unsigned int nsyms, i;
     char ksbuf[64];
 
@@ -77,6 +78,11 @@ test_key_seq_va(struct xkb_keymap *keymap, va_list ap)
         op = va_arg(ap, int);
 
         nsyms = xkb_state_key_get_syms(state, kc, &syms);
+        if (nsyms == 1) {
+            sym = xkb_state_key_get_one_sym(state, kc);
+            syms = &sym;
+        }
+
         fprintf(stderr, "got %u syms for key 0x%x: [", nsyms, kc);
 
         if (op == DOWN || op == BOTH)

diff --git a/test/state.c b/test/state.c
index 97c2bb6a266f8970e7801bfb7304948170bd0f10..2164d6bf740d466e0d54ad85c0166d8f38b80de3 100644 (file)
--- a/test/state.c
+++ b/test/state.c
@@ -309,17 +309,25 @@ test_repeat(struct xkb_keymap *keymap)
 static void
 test_consume(struct xkb_keymap *keymap)
 {
-    struct xkb_state *state = xkb_state_new(keymap);
-    xkb_mod_index_t alt, shift;
+    struct xkb_state *state;
+    xkb_mod_index_t alt, shift, caps, ctrl, mod5;
     xkb_mod_mask_t mask;
 
+    state = xkb_state_new(keymap);
     assert(state);
 
     alt = xkb_keymap_mod_get_index(keymap, XKB_MOD_NAME_ALT);
     assert(alt != XKB_MOD_INVALID);
     shift = xkb_keymap_mod_get_index(keymap, XKB_MOD_NAME_SHIFT);
     assert(shift != XKB_MOD_INVALID);
+    caps = xkb_keymap_mod_get_index(keymap, XKB_MOD_NAME_CAPS);
+    assert(caps != XKB_MOD_INVALID);
+    ctrl = xkb_keymap_mod_get_index(keymap, XKB_MOD_NAME_CTRL);
+    assert(ctrl != XKB_MOD_INVALID);
+    mod5 = xkb_keymap_mod_get_index(keymap, "Mod5");
+    assert(mod5 != XKB_MOD_INVALID);
 
+    /* Test remove_consumed() */
     xkb_state_update_key(state, KEY_LEFTALT + EVDEV_OFFSET, XKB_KEY_DOWN);
     xkb_state_update_key(state, KEY_LEFTSHIFT + EVDEV_OFFSET, XKB_KEY_DOWN);
     xkb_state_update_key(state, KEY_EQUAL + EVDEV_OFFSET, XKB_KEY_DOWN);
@@ -333,9 +341,56 @@ test_consume(struct xkb_keymap *keymap)
                                               mask);
     assert(mask == (1U << alt));
 
+    /* Test get_consumed_mods() */
     mask = xkb_state_key_get_consumed_mods(state, KEY_EQUAL + EVDEV_OFFSET);
     assert(mask == (1U << shift));
 
+    mask = xkb_state_key_get_consumed_mods(state, KEY_ESC + EVDEV_OFFSET);
+    assert(mask == 0);
+
+    xkb_state_unref(state);
+
+    /* Test is_consumed() - simple ALPHABETIC type. */
+    state = xkb_state_new(keymap);
+    assert(state);
+
+    mask = xkb_state_key_get_consumed_mods(state, KEY_A + EVDEV_OFFSET);
+    assert(mask == ((1U << shift) | (1U << caps)));
+
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, caps) > 0);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, shift) > 0);
+    xkb_state_update_key(state, KEY_CAPSLOCK + EVDEV_OFFSET, XKB_KEY_DOWN);
+    xkb_state_update_key(state, KEY_CAPSLOCK + EVDEV_OFFSET, XKB_KEY_UP);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, caps) > 0);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, shift) > 0);
+    xkb_state_update_key(state, KEY_LEFTSHIFT + EVDEV_OFFSET, XKB_KEY_DOWN);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, caps) > 0);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, shift) > 0);
+    xkb_state_update_key(state, KEY_LEFTSHIFT + EVDEV_OFFSET, XKB_KEY_UP);
+    xkb_state_update_key(state, KEY_CAPSLOCK + EVDEV_OFFSET, XKB_KEY_DOWN);
+    xkb_state_update_key(state, KEY_CAPSLOCK + EVDEV_OFFSET, XKB_KEY_UP);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, caps) > 0);
+    assert(xkb_state_mod_index_is_consumed(state, KEY_A + EVDEV_OFFSET, shift) > 0);
+
+    xkb_state_unref(state);
+
+    /* More complicated - CTRL+ALT */
+    state = xkb_state_new(keymap);
+
+    mask = xkb_state_key_get_consumed_mods(state, KEY_F1 + EVDEV_OFFSET);
+    assert(mask == ((1U << shift) | (1U << alt) | (1U << ctrl) | (1U << mod5)));
+
+    /* Shift is preserved. */
+    xkb_state_update_key(state, KEY_LEFTSHIFT + EVDEV_OFFSET, XKB_KEY_DOWN);
+    mask = xkb_state_key_get_consumed_mods(state, KEY_F1 + EVDEV_OFFSET);
+    assert(mask == ((1U << alt) | (1U << ctrl) | (1U << mod5)));
+    xkb_state_update_key(state, KEY_LEFTSHIFT + EVDEV_OFFSET, XKB_KEY_UP);
+
+    mask = xkb_state_key_get_consumed_mods(state, KEY_F1 + EVDEV_OFFSET);
+    assert(mask == ((1U << shift) | (1U << alt) | (1U << ctrl) | (1U << mod5)));
+
+    assert(state);
+
     xkb_state_unref(state);
 }