state: correctly wrap state->locked_group and ->group

These values weren't wrapped before, which caused group_index_is_active
to stop working after a few group switches.

Also, the current group-wrapping function didn't take into consideration
actions such as LockGroup=-1, which need to wrap around, etc.
xkb_layout_index_t is unsigned, but it was used to hold possibly
negative values (e.g. locked_group is 0 and gets a -1 action).

This group wrapping function should now act like the XkbAdjustGroup
function from xserver, and at least ./test/interactive doesn't bring up
any problems with group switching any more.

Signed-off-by: Ran Benita <ran234@gmail.com>

diff --git a/src/state.c b/src/state.c
index 695db2c..bfe35bb 100644 (file)
--- a/src/state.c
+++ b/src/state.c
@@ -73,9 +73,10 @@ struct xkb_filter {
 };
 
 struct xkb_state {
-    xkb_layout_index_t base_group; /**< depressed */
-    xkb_layout_index_t latched_group;
-    xkb_layout_index_t locked_group;
+    /* These may be negative, because of -1 group actions. */
+    int32_t base_group; /**< depressed */
+    int32_t latched_group;
+    int32_t locked_group;
     xkb_layout_index_t group; /**< effective */
 
     xkb_mod_mask_t base_mods; /**< depressed */
@@ -143,7 +144,7 @@ xkb_state_key_get_level(struct xkb_state *state, xkb_keycode_t kc,
 }
 
 static xkb_layout_index_t
-wrap_group_into_range(xkb_layout_index_t group,
+wrap_group_into_range(int32_t group,
                       xkb_layout_index_t num_groups,
                       enum xkb_range_exceed_type out_of_range_group_action,
                       xkb_layout_index_t out_of_range_group_number)
@@ -161,11 +162,21 @@ wrap_group_into_range(xkb_layout_index_t group,
         return out_of_range_group_number;
 
     case RANGE_SATURATE:
-        return num_groups - 1;
+        if (group < 0)
+            return 0;
+        else
+            return num_groups - 1;
 
     case RANGE_WRAP:
     default:
-        return group % num_groups;
+        /*
+         * C99 says a negative dividend in a modulo operation always
+         * gives a negative result.
+         */
+        if (group < 0)
+            return ((int) num_groups + (group % (int) num_groups));
+        else
+            return group % num_groups;
     }
 }
 
@@ -619,12 +630,23 @@ xkb_state_led_update_all(struct xkb_state *state)
 static void
 xkb_state_update_derived(struct xkb_state *state)
 {
-    state->mods =
-        (state->base_mods | state->latched_mods | state->locked_mods);
-    /* FIXME: Clamp/wrap locked_group */
-    state->group = state->locked_group + state->base_group +
-                   state->latched_group;
-    /* FIXME: Clamp/wrap effective group */
+    xkb_layout_index_t num_groups = xkb_keymap_num_layouts(state->keymap);
+
+    state->mods = (state->base_mods |
+                   state->latched_mods |
+                   state->locked_mods);
+
+    /* TODO: Use groups_wrap control instead of always RANGE_WRAP. */
+
+    state->locked_group = wrap_group_into_range(state->locked_group,
+                                                num_groups,
+                                                RANGE_WRAP, 0);
+
+    state->group = wrap_group_into_range(state->base_group +
+                                         state->latched_group +
+                                         state->locked_group,
+                                         num_groups,
+                                         RANGE_WRAP, 0);
 
     xkb_state_led_update_all(state);
 }

diff --git a/test/keyseq.c b/test/keyseq.c
index 4508ab1..d5993ec 100644 (file)
--- a/test/keyseq.c
+++ b/test/keyseq.c
@@ -379,6 +379,46 @@ main(void)
                         KEY_V,           BOTH,  XKB_KEY_p,               FINISH));
 
     xkb_keymap_unref(keymap);
+    assert(ctx);
+    keymap = test_compile_rules(ctx, "evdev", "", "us,il,ru", "",
+                                "grp:alt_shift_toggle_bidir,grp:menu_toggle");
+    assert(keymap);
+
+    assert(test_key_seq(keymap,
+                        KEY_LEFTSHIFT, DOWN, XKB_KEY_Shift_L,        NEXT,
+                        KEY_LEFTALT,   DOWN, XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTALT,   UP,   XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTSHIFT, UP,   XKB_KEY_Shift_L,        FINISH));
+
+    assert(test_key_seq(keymap,
+                        KEY_LEFTALT,   DOWN, XKB_KEY_Alt_L,          NEXT,
+                        KEY_LEFTSHIFT, DOWN, XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTSHIFT, UP,   XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTALT,   UP,   XKB_KEY_Alt_L,          FINISH));
+
+    /* Check backwards (negative) group switching and wrapping. */
+    assert(test_key_seq(keymap,
+                        KEY_H,         BOTH, XKB_KEY_h,              NEXT,
+                        KEY_COMPOSE,   BOTH, XKB_KEY_ISO_Next_Group, NEXT,
+                        KEY_H,         BOTH, XKB_KEY_hebrew_yod,     NEXT,
+                        KEY_COMPOSE,   BOTH, XKB_KEY_ISO_Next_Group, NEXT,
+                        KEY_H,         BOTH, XKB_KEY_Cyrillic_er,    NEXT,
+                        KEY_LEFTSHIFT, DOWN, XKB_KEY_Shift_L,        NEXT,
+                        KEY_LEFTALT,   BOTH, XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTSHIFT, UP,   XKB_KEY_Shift_L,        NEXT,
+                        KEY_H,         BOTH, XKB_KEY_hebrew_yod,     NEXT,
+                        KEY_LEFTSHIFT, DOWN, XKB_KEY_Shift_L,        NEXT,
+                        KEY_LEFTALT,   BOTH, XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTSHIFT, UP,   XKB_KEY_Shift_L,        NEXT,
+                        KEY_H,         BOTH, XKB_KEY_h,              NEXT,
+                        KEY_LEFTSHIFT, DOWN, XKB_KEY_Shift_L,        NEXT,
+                        KEY_LEFTALT,   BOTH, XKB_KEY_ISO_Prev_Group, NEXT,
+                        KEY_LEFTSHIFT, UP,   XKB_KEY_Shift_L,        NEXT,
+                        KEY_H,         BOTH, XKB_KEY_Cyrillic_er,    NEXT,
+                        KEY_COMPOSE,   BOTH, XKB_KEY_ISO_Next_Group, NEXT,
+                        KEY_H,         BOTH, XKB_KEY_h,              FINISH));
+
+    xkb_keymap_unref(keymap);
     xkb_context_unref(ctx);
     return 0;
 }