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 d130b8e..0f9ea79 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 8b3f954..e6cd1c3 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 97c2bb6..2164d6b 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);
 }