Don’t use SYNC key bindings for Mode_switch but re-grab keys

Before this commit, i3 used key bindings in SYNC mode for bindings
like Mode_switch + <a> and replayed the key if the current state
did not include Mode_switch. This had some problems:

1) The WM needed to acknowledge much more key presses than you
   actually had bindings for, thus making the system a bit laggy
   sometimes.
2) Users of layouts who constantly type in the third level (like
   russian layouts) did not get their cyrillic symbols correctly
   (they were not replayed right), neither did the keybindings
   work in both modes.

So, the current implementation uses the following approach: XKB
provides an event which contains the current state (including
the current level). i3 signs up for this event and upon receival,
it re-maps the bindings using Mode_switch (enables them when the
level goes to the third level and disables them as soon as the
level goes back to normal). This fixes both problems.

include/config.h

@@ -133,6 +133,12 @@ struct Config {
  */
 void load_configuration(xcb_connection_t *conn, const char *override_configfile, bool reload);
 
+/**
+ * Translates keysymbols to keycodes for all bindings which use keysyms.
+ *
+ */
+void translate_keysyms();
+
 /**
  * Ungrabs all keys, to be called before re-grabbing the keys because of a
  * mapping_notify event or a configuration file reload
@@ -144,7 +150,7 @@ void ungrab_all_keys(xcb_connection_t *conn);
  * Grab the bound keys (tell X to send us keypress events for those keycodes)
  *
  */
-void grab_all_keys(xcb_connection_t *conn);
+void grab_all_keys(xcb_connection_t *conn, bool bind_mode_switch);
 
 /**
  * Switches the key bindings to the given mode, if the mode exists
@@ -152,6 +158,13 @@ void grab_all_keys(xcb_connection_t *conn);
  */
 void switch_mode(xcb_connection_t *conn, const char *new_mode);
 
+/**
+ * Returns a pointer to the Binding with the specified modifiers and keycode
+ * or NULL if no such binding exists.
+ *
+ */
+Binding *get_binding(uint16_t modifiers, xcb_keycode_t keycode);
+
 /* prototype for src/cfgparse.y */
 void parse_file(const char *f);
 

include/handlers.h

@@ -13,14 +13,6 @@
 
 #include <xcb/randr.h>
 
-/**
- * Due to bindings like Mode_switch + <a>, we need to bind some keys in
- * XCB_GRAB_MODE_SYNC.  Therefore, we just replay all key presses.
- *
- */
-int handle_key_release(void *ignored, xcb_connection_t *conn,
-                       xcb_key_release_event_t *event);
-
 /**
  * There was a key press. We compare this key code with our bindings table and
  * pass the bound action to parse_command().

include/i3.h

@@ -27,6 +27,7 @@ extern xcb_connection_t *global_conn;
 extern xcb_key_symbols_t *keysyms;
 extern char **start_argv;
 extern Display *xkbdpy;
+extern int xkb_current_group;
 extern TAILQ_HEAD(bindings_head, Binding) *bindings;
 extern TAILQ_HEAD(autostarts_head, Autostart) autostarts;
 extern TAILQ_HEAD(assignments_head, Assignment) assignments;

src/config.c

@@ -70,31 +70,62 @@ void ungrab_all_keys(xcb_connection_t *conn) {
 
 static void grab_keycode_for_binding(xcb_connection_t *conn, Binding *bind, uint32_t keycode) {
         DLOG("Grabbing %d\n", keycode);
-        if ((bind->mods & BIND_MODE_SWITCH) != 0)
-                xcb_grab_key(conn, 0, root, 0, keycode,
-                        XCB_GRAB_MODE_SYNC, XCB_GRAB_MODE_SYNC);
-        else {
         /* Grab the key in all combinations */
-                #define GRAB_KEY(modifier) xcb_grab_key(conn, 0, root, modifier, keycode, \
+        #define GRAB_KEY(modifier) \
-                                                        XCB_GRAB_MODE_SYNC, XCB_GRAB_MODE_ASYNC)
+                do { \
-                GRAB_KEY(bind->mods);
+                        xcb_grab_key(conn, 0, root, modifier, keycode, \
-                GRAB_KEY(bind->mods | xcb_numlock_mask);
+                                     XCB_GRAB_MODE_SYNC, XCB_GRAB_MODE_ASYNC); \
-                GRAB_KEY(bind->mods | xcb_numlock_mask | XCB_MOD_MASK_LOCK);
+                } while (0)
+        int mods = bind->mods;
+        if ((bind->mods & BIND_MODE_SWITCH) != 0) {
+                mods &= ~BIND_MODE_SWITCH;
+                if (mods == 0)
+                        mods = XCB_MOD_MASK_ANY;
         }
+        GRAB_KEY(mods);
+        GRAB_KEY(mods | xcb_numlock_mask);
+        GRAB_KEY(mods | xcb_numlock_mask | XCB_MOD_MASK_LOCK);
 }
 
 /*
- * Grab the bound keys (tell X to send us keypress events for those keycodes)
+ * Returns a pointer to the Binding with the specified modifiers and keycode
+ * or NULL if no such binding exists.
  *
  */
-void grab_all_keys(xcb_connection_t *conn) {
+Binding *get_binding(uint16_t modifiers, xcb_keycode_t keycode) {
+        Binding *bind;
+
+        TAILQ_FOREACH(bind, bindings, bindings) {
+                /* First compare the modifiers */
+                if (bind->mods != modifiers)
+                        continue;
+
+                /* If a symbol was specified by the user, we need to look in
+                 * the array of translated keycodes for the event’s keycode */
+                if (bind->symbol != NULL) {
+                        if (memmem(bind->translated_to,
+                                   bind->number_keycodes * sizeof(xcb_keycode_t),
+                                   &keycode, sizeof(xcb_keycode_t)) != NULL)
+                                break;
+                } else {
+                        /* This case is easier: The user specified a keycode */
+                        if (bind->keycode == keycode)
+                                break;
+                }
+        }
+
+        return (bind == TAILQ_END(bindings) ? NULL : bind);
+}
+
+/*
+ * Translates keysymbols to keycodes for all bindings which use keysyms.
+ *
+ */
+void translate_keysyms() {
         Binding *bind;
         TAILQ_FOREACH(bind, bindings, bindings) {
-                /* The easy case: the user specified a keycode directly. */
+                if (bind->keycode > 0)
-                if (bind->keycode > 0) {
-                        grab_keycode_for_binding(conn, bind, bind->keycode);
                         continue;
-                }
 
                 /* We need to translate the symbol to a keycode */
                 xcb_keysym_t keysym = XStringToKeysym(bind->symbol);
@@ -125,7 +156,6 @@ void grab_all_keys(xcb_connection_t *conn) {
                          * and skip them */
                         if (last_keycode == *walk)
                                 continue;
-                        grab_keycode_for_binding(conn, bind, *walk);
                         last_keycode = *walk;
                         bind->number_keycodes++;
                 }
@@ -137,6 +167,29 @@ void grab_all_keys(xcb_connection_t *conn) {
         }
 }
 
+/*
+ * Grab the bound keys (tell X to send us keypress events for those keycodes)
+ *
+ */
+void grab_all_keys(xcb_connection_t *conn, bool bind_mode_switch) {
+        Binding *bind;
+        TAILQ_FOREACH(bind, bindings, bindings) {
+                if ((bind_mode_switch && (bind->mods & BIND_MODE_SWITCH) == 0) ||
+                    (!bind_mode_switch && (bind->mods & BIND_MODE_SWITCH) != 0))
+                        continue;
+
+                /* The easy case: the user specified a keycode directly. */
+                if (bind->keycode > 0) {
+                        grab_keycode_for_binding(conn, bind, bind->keycode);
+                        continue;
+                }
+
+                xcb_keycode_t *walk = bind->translated_to;
+                for (int i = 0; i < bind->number_keycodes; i++)
+                        grab_keycode_for_binding(conn, bind, *walk);
+        }
+}
+
 /*
  * Switches the key bindings to the given mode, if the mode exists
  *
@@ -152,7 +205,7 @@ void switch_mode(xcb_connection_t *conn, const char *new_mode) {
 
                 ungrab_all_keys(conn);
                 bindings = mode->bindings;
-                grab_all_keys(conn);
+                grab_all_keys(conn, false);
                 return;
         }
 
@@ -304,8 +357,10 @@ void load_configuration(xcb_connection_t *conn, const char *override_configpath,
 
         parse_configuration(override_configpath);
 
-        if (reload)
+        if (reload) {
-                grab_all_keys(conn);
+                translate_keysyms();
+                grab_all_keys(conn, false);
+        }
 
         REQUIRED_OPTION(font);
 

src/handlers.c

@@ -82,17 +82,6 @@ static bool event_is_ignored(const int sequence) {
         return false;
 }
 
-/*
- * Due to bindings like Mode_switch + <a>, we need to bind some keys in XCB_GRAB_MODE_SYNC.
- * Therefore, we just replay all key presses.
- *
- */
-int handle_key_release(void *ignored, xcb_connection_t *conn, xcb_key_release_event_t *event) {
-        xcb_allow_events(conn, XCB_ALLOW_REPLAY_KEYBOARD, event->time);
-        xcb_flush(conn);
-        return 1;
-}
-
 /*
  * There was a key press. We compare this key code with our bindings table and pass
  * the bound action to parse_command().
@@ -109,52 +98,29 @@ int handle_key_press(void *ignored, xcb_connection_t *conn, xcb_key_press_event_
         state_filtered &= 0xFF;
         DLOG("(removed upper 8 bits, state = %d)\n", state_filtered);
 
-        if (xkb_supported) {
+        if (xkb_current_group == XkbGroup2Index)
-                /* We need to get the keysym group (There are group 1 to group 4, each holding
-                   two keysyms (without shift and with shift) using Xkb because X fails to
-                   provide them reliably (it works in Xephyr, it does not in real X) */
-                XkbStateRec state;
-                if (XkbGetState(xkbdpy, XkbUseCoreKbd, &state) == Success && (state.group+1) == 2)
                 state_filtered |= BIND_MODE_SWITCH;
-        }
 
         DLOG("(checked mode_switch, state %d)\n", state_filtered);
 
         /* Find the binding */
-        Binding *bind;
+        Binding *bind = get_binding(state_filtered, event->detail);
-        TAILQ_FOREACH(bind, bindings, bindings) {
-                /* First compare the modifiers */
-                if (bind->mods != state_filtered)
-                        continue;
 
-                /* If a symbol was specified by the user, we need to look in
+        /* No match? Then the user has Mode_switch enabled but does not have a
-                 * the array of translated keycodes for the event’s keycode */
+         * specific keybinding. Fall back to the default keybindings (without
-                if (bind->symbol != NULL) {
+         * Mode_switch). Makes it much more convenient for users of a hybrid
-                        if (memmem(bind->translated_to,
+         * layout (like us, ru). */
-                                   bind->number_keycodes * sizeof(xcb_keycode_t),
+        if (bind == NULL) {
-                                   &(event->detail), sizeof(xcb_keycode_t)) != NULL)
+                state_filtered &= ~(BIND_MODE_SWITCH);
-                                break;
+                DLOG("no match, new state_filtered = %d\n", state_filtered);
-                } else {
+                if ((bind = get_binding(state_filtered, event->detail)) == NULL) {
-                        /* This case is easier: The user specified a keycode */
+                        ELOG("Could not lookup key binding (modifiers %d, keycode %d)\n",
-                        if (bind->keycode == event->detail)
+                             state_filtered, event->detail);
-                                break;
-                }
-        }
-
-        /* No match? Then it was an actively grabbed key, that is with Mode_switch, and
-           the user did not press Mode_switch, so just pass it… */
-        if (bind == TAILQ_END(bindings)) {
-                xcb_allow_events(conn, ReplayKeyboard, event->time);
-                xcb_flush(conn);
                         return 1;
                 }
+        }
 
         parse_command(conn, bind->command);
-        if (state_filtered & BIND_MODE_SWITCH) {
-                DLOG("Mode_switch -> allow_events(SyncKeyboard)\n");
-                xcb_allow_events(conn, SyncKeyboard, event->time);
-                xcb_flush(conn);
-        }
         return 1;
 }
 
@@ -295,7 +261,8 @@ int handle_mapping_notify(void *ignored, xcb_connection_t *conn, xcb_mapping_not
         xcb_get_numlock_mask(conn);
 
         ungrab_all_keys(conn);
-        grab_all_keys(conn);
+        translate_keysyms();
+        grab_all_keys(conn, false);
 
         return 0;
 }

src/mainx.c

@@ -51,6 +51,10 @@
 #include "log.h"
 #include "sighandler.h"
 
+static int xkb_event_base;
+
+int xkb_current_group;
+
 xcb_connection_t *global_conn;
 
 /* This is the path to i3, copied from argv[0] when starting up */
@@ -125,14 +129,54 @@ static void xcb_check_cb(EV_P_ ev_check *w, int revents) {
  */
 static void xkb_got_event(EV_P_ struct ev_io *w, int revents) {
         DLOG("Handling XKB event\n");
-        XEvent ev;
+        XkbEvent ev;
+
         /* When using xmodmap, every change (!) gets an own event.
          * Therefore, we just read all events and only handle the
-         * mapping_notify once (we do not receive any other XKB
+         * mapping_notify once. */
-         * events anyway). */
+        bool mapping_changed = false;
-        while (XPending(xkbdpy))
+        while (XPending(xkbdpy)) {
-                XNextEvent(xkbdpy, &ev);
+                XNextEvent(xkbdpy, (XEvent*)&ev);
+                /* While we should never receive a non-XKB event,
+                 * better do sanity checking */
+                if (ev.type != xkb_event_base)
+                        continue;
 
+                if (ev.any.xkb_type == XkbMapNotify) {
+                        mapping_changed = true;
+                        continue;
+                }
+
+                if (ev.any.xkb_type != XkbStateNotify) {
+                        ELOG("Unknown XKB event received (type %d)\n", ev.any.xkb_type);
+                        continue;
+                }
+
+                /* See The XKB Extension: Library Specification, section 14.1 */
+                /* We check if the current group (each group contains
+                 * two levels) has been changed. Mode_switch activates
+                 * group XkbGroup2Index */
+                if (xkb_current_group == ev.state.group)
+                        continue;
+
+                xkb_current_group = ev.state.group;
+
+                if (ev.state.group == XkbGroup2Index) {
+                        DLOG("Mode_switch enabled\n");
+                        grab_all_keys(global_conn, true);
+                }
+
+                if (ev.state.group == XkbGroup1Index) {
+                        DLOG("Mode_switch disabled\n");
+                        ungrab_all_keys(global_conn);
+                        grab_all_keys(global_conn, false);
+                }
+        }
+
+        if (!mapping_changed)
+                return;
+
+        DLOG("Keyboard mapping changed, updating keybindings\n");
         xcb_key_symbols_free(keysyms);
         keysyms = xcb_key_symbols_alloc(global_conn);
 
@@ -140,9 +184,9 @@ static void xkb_got_event(EV_P_ struct ev_io *w, int revents) {
 
         ungrab_all_keys(global_conn);
         DLOG("Re-grabbing...\n");
-        grab_all_keys(global_conn);
+        translate_keysyms();
+        grab_all_keys(global_conn, (xkb_current_group == XkbGroup2Index));
         DLOG("Done\n");
-
 }
 
 
@@ -284,9 +328,9 @@ int main(int argc, char *argv[], char *env[]) {
         major = XkbMajorVersion;
         minor = XkbMinorVersion;
 
-        int evBase, errBase;
+        int errBase;
 
-        if ((xkbdpy = XkbOpenDisplay(getenv("DISPLAY"), &evBase, &errBase, &major, &minor, &error)) == NULL) {
+        if ((xkbdpy = XkbOpenDisplay(getenv("DISPLAY"), &xkb_event_base, &errBase, &major, &minor, &error)) == NULL) {
                 ELOG("ERROR: XkbOpenDisplay() failed, disabling XKB support\n");
                 xkb_supported = false;
         }
@@ -298,13 +342,15 @@ int main(int argc, char *argv[], char *env[]) {
                 }
 
                 int i1;
-                if (!XkbQueryExtension(xkbdpy,&i1,&evBase,&errBase,&major,&minor)) {
+                if (!XkbQueryExtension(xkbdpy,&i1,&xkb_event_base,&errBase,&major,&minor)) {
                         fprintf(stderr, "XKB not supported by X-server\n");
                         return 1;
                 }
                 /* end of ugliness */
 
-                if (!XkbSelectEvents(xkbdpy, XkbUseCoreKbd, XkbMapNotifyMask, XkbMapNotifyMask)) {
+                if (!XkbSelectEvents(xkbdpy, XkbUseCoreKbd,
+                                     XkbMapNotifyMask | XkbStateNotifyMask,
+                                     XkbMapNotifyMask | XkbStateNotifyMask)) {
                         fprintf(stderr, "Could not set XKB event mask\n");
                         return 1;
                 }
@@ -352,9 +398,8 @@ int main(int argc, char *argv[], char *env[]) {
         /* Expose = an Application should redraw itself, in this case it’s our titlebars. */
         xcb_event_set_expose_handler(&evenths, handle_expose_event, NULL);
 
-        /* Key presses/releases are pretty obvious, I think */
+        /* Key presses are pretty obvious, I think */
         xcb_event_set_key_press_handler(&evenths, handle_key_press, NULL);
-        xcb_event_set_key_release_handler(&evenths, handle_key_release, NULL);
 
         /* Enter window = user moved his mouse over the window */
         xcb_event_set_enter_notify_handler(&evenths, handle_enter_notify, NULL);
@@ -475,7 +520,8 @@ int main(int argc, char *argv[], char *env[]) {
 
         xcb_get_numlock_mask(conn);
 
-        grab_all_keys(conn);
+        translate_keysyms();
+        grab_all_keys(conn, false);
 
         int randr_base;
         if (force_xinerama) {