i3 - improved tiling WM

Reviews
Bugs

Refactor binding accessor

Patch status: merged

Patch by Tony Crisci

Long description:

Change the primary binding accessor to `get_binding_from_xcb_event`.

This function gets a binding from a generic xcb event of type KeyPress,
KeyRelease, ButtonPress, or ButtonRelease by determining the input type
(keyboard or mouse), the modifiers pressed from the filtered event
`state`, managing the proper fall back in case mode switch is enabled,
and finally querying the bindings for a binding that matches the event.

The logic of querying keyboard bindings is not intended to be altered by
this change.

The general accessor has been slightly modified to work with mouse
bindings and made private because it is only used in bindings.c

To apply this patch, use:
curl http://cr.i3wm.org/patch/532/raw.patch | git am

b/include/bindings.h

@@ -31,11 +31,11 @@ Binding *configure_binding(const char *bindtype, const char *modifiers, const ch
 void grab_all_keys(xcb_connection_t *conn, bool bind_mode_switch);
 
 /**
- * Returns a pointer to the keyboard Binding with the specified modifiers and
- * keycode or NULL if no such binding exists.
+ * Returns a pointer to the Binding that matches the given xcb event or NULL if
+ * no such binding exists.
  *
  */
-Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_t keycode);
+Binding *get_binding_from_xcb_event(xcb_generic_event_t *event);
 
 /**
  * Translates keysymbols to keycodes for all bindings which use keysyms.

b/include/data.h

@@ -92,6 +92,14 @@ typedef enum {
 } layout_t;
 
 /**
+ * Binding input types. See Binding::input_type.
+ */
+typedef enum {
+    B_KEYBOARD = 0,
+    B_MOUSE = 1
+} input_type_t;
+
+/**
  * Stores a rectangle, for example the size of a window, the child window etc.
  * It needs to be packed so that the compiler will not add any padding bytes.
  * (it is used in src/ewmh.c for example)
@@ -215,12 +223,7 @@ struct regex {
 struct Binding {
     /* The type of input this binding is for. (Mouse bindings are not yet
      * implemented. All bindings are currently assumed to be keyboard bindings.) */
-    enum {
-        /* Created with "bindsym", "bindcode", and "bind" */
-        B_KEYBOARD = 0,
-        /* Created with "bindmouse" (not yet implemented). */
-        B_MOUSE = 1,
-    } input_type;
+    input_type_t input_type;
 
     /** If true, the binding should be executed upon a KeyRelease event, not a
      * KeyPress (the default). */

b/src/bindings.c

@@ -123,18 +123,18 @@ void grab_all_keys(xcb_connection_t *conn, bool bind_mode_switch) {
 }
 
 /*
- * Returns a pointer to the keyboard Binding with the specified modifiers and
+ * Returns a pointer to the Binding with the specified modifiers and
  * keycode or NULL if no such binding exists.
  *
  */
-Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_t keycode) {
+static Binding *get_binding(uint16_t modifiers, bool is_release, uint16_t input_code, input_type_t input_type) {
     Binding *bind;
 
-    if (!key_release) {
-        /* On a KeyPress event, we first reset all
-         * B_UPON_KEYRELEASE_IGNORE_MODS bindings back to B_UPON_KEYRELEASE */
+    if (!is_release) {
+        /* On a press event, we first reset all B_UPON_KEYRELEASE_IGNORE_MODS
+         * bindings back to B_UPON_KEYRELEASE */
         TAILQ_FOREACH(bind, bindings, bindings) {
-            if (bind->input_type != B_KEYBOARD)
+            if (bind->input_type != input_type)
                 continue;
             if (bind->release == B_UPON_KEYRELEASE_IGNORE_MODS)
                 bind->release = B_UPON_KEYRELEASE;
@@ -145,37 +145,37 @@ Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_
         /* First compare the modifiers (unless this is a
          * B_UPON_KEYRELEASE_IGNORE_MODS binding and this is a KeyRelease
          * event) */
-        if (bind->input_type != B_KEYBOARD)
+        if (bind->input_type != input_type)
             continue;
         if (bind->mods != modifiers &&
             (bind->release != B_UPON_KEYRELEASE_IGNORE_MODS ||
-             !key_release))
+             !is_release))
             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) {
+        /* For keyboard bindings where a symbol was specified by the user, we
+         * need to look in the array of translated keycodes for the event’s
+         * keycode */
+        if (input_type == B_KEYBOARD && bind->symbol != NULL) {
             if (memmem(bind->translated_to,
                        bind->number_keycodes * sizeof(xcb_keycode_t),
-                       &keycode, sizeof(xcb_keycode_t)) == NULL)
+                       &input_code, sizeof(xcb_keycode_t)) == NULL)
                 continue;
         } else {
             /* This case is easier: The user specified a keycode */
-            if (bind->keycode != keycode)
+            if (bind->keycode != input_code)
                 continue;
         }
 
-        /* If this keybinding is a KeyRelease binding, it matches the key which
-         * the user pressed. We therefore mark it as
-         * B_UPON_KEYRELEASE_IGNORE_MODS for later, so that the user can
-         * release the modifiers before the actual key and the KeyRelease will
-         * still be matched. */
-        if (bind->release == B_UPON_KEYRELEASE && !key_release)
+        /* If this binding is a release binding, it matches the key which the
+         * user pressed. We therefore mark it as B_UPON_KEYRELEASE_IGNORE_MODS
+         * for later, so that the user can release the modifiers before the
+         * actual key or button and the release event will still be matched. */
+        if (bind->release == B_UPON_KEYRELEASE && !is_release)
             bind->release = B_UPON_KEYRELEASE_IGNORE_MODS;
 
-        /* Check if the binding is for a KeyPress or a KeyRelease event */
-        if ((bind->release == B_UPON_KEYPRESS && key_release) ||
-            (bind->release >= B_UPON_KEYRELEASE && !key_release))
+        /* Check if the binding is for a press or a release event */
+        if ((bind->release == B_UPON_KEYPRESS && is_release) ||
+            (bind->release >= B_UPON_KEYRELEASE && !is_release))
             continue;
 
         break;
@@ -185,6 +185,59 @@ Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_
 }
 
 /*
+ * Returns a pointer to the Binding that matches the given xcb button or key
+ * event or NULL if no such binding exists.
+ *
+ */
+Binding *get_binding_from_xcb_event(xcb_generic_event_t *event) {
+    bool is_release = (event->response_type == XCB_KEY_RELEASE
+                        || event->response_type == XCB_BUTTON_RELEASE);
+
+    input_type_t input_type = ((event->response_type == XCB_BUTTON_RELEASE
+                                || event->response_type == XCB_BUTTON_PRESS)
+                                ? B_MOUSE
+                                : B_KEYBOARD);
+
+    uint16_t event_state = ((xcb_key_press_event_t *)event)->state;
+    uint16_t event_detail = ((xcb_key_press_event_t *)event)->detail;
+
+    /* Remove the numlock bit, all other bits are modifiers we can bind to */
+    uint16_t state_filtered = event_state & ~(xcb_numlock_mask | XCB_MOD_MASK_LOCK);
+    DLOG("(removed numlock, state = %d)\n", state_filtered);
+    /* Only use the lower 8 bits of the state (modifier masks) so that mouse
+     * button masks are filtered out */
+    state_filtered &= 0xFF;
+    DLOG("(removed upper 8 bits, state = %d)\n", state_filtered);
+
+    if (xkb_current_group == XkbGroup2Index)
+        state_filtered |= BIND_MODE_SWITCH;
+
+    DLOG("(checked mode_switch, state %d)\n", state_filtered);
+
+    /* Find the binding */
+    Binding *bind = get_binding(state_filtered, is_release, event_detail, input_type);
+
+    /* No match? Then the user has Mode_switch enabled but does not have a
+     * specific keybinding. Fall back to the default keybindings (without
+     * Mode_switch). Makes it much more convenient for users of a hybrid
+     * layout (like ru). */
+    if (bind == NULL) {
+        state_filtered &= ~(BIND_MODE_SWITCH);
+        DLOG("no match, new state_filtered = %d\n", state_filtered);
+        if ((bind = get_binding(state_filtered, is_release, event_detail, input_type)) == NULL) {
+            /* This is not a real error since we can have release and
+             * non-release bindings. On a press event for which there is only a
+             * !release-binding, but no release-binding, the corresponding
+             * release event will trigger this. No problem, though. */
+            DLOG("Could not lookup key binding (modifiers %d, keycode %d)\n",
+                 state_filtered, event_detail);
+        }
+    }
+
+    return bind;
+}
+
+/*
  * Translates keysymbols to keycodes for all bindings which use keysyms.
  *
  */

b/src/key_press.c

@@ -70,40 +70,11 @@ void handle_key_press(xcb_key_press_event_t *event) {
 
     DLOG("%s %d, state raw = %d\n", (key_release ? "KeyRelease" : "KeyPress"), event->detail, event->state);
 
-    /* Remove the numlock bit, all other bits are modifiers we can bind to */
-    uint16_t state_filtered = event->state & ~(xcb_numlock_mask | XCB_MOD_MASK_LOCK);
-    DLOG("(removed numlock, state = %d)\n", state_filtered);
-    /* Only use the lower 8 bits of the state (modifier masks) so that mouse
-     * button masks are filtered out */
-    state_filtered &= 0xFF;
-    DLOG("(removed upper 8 bits, state = %d)\n", state_filtered);
-
-    if (xkb_current_group == XkbGroup2Index)
-        state_filtered |= BIND_MODE_SWITCH;
-
-    DLOG("(checked mode_switch, state %d)\n", state_filtered);
-
-    /* Find the binding */
-    Binding *bind = get_keyboard_binding(state_filtered, key_release, event->detail);
-
-    /* No match? Then the user has Mode_switch enabled but does not have a
-     * specific keybinding. Fall back to the default keybindings (without
-     * Mode_switch). Makes it much more convenient for users of a hybrid
-     * layout (like us, ru). */
-    if (bind == NULL) {
-        state_filtered &= ~(BIND_MODE_SWITCH);
-        DLOG("no match, new state_filtered = %d\n", state_filtered);
-        if ((bind = get_keyboard_binding(state_filtered, key_release, event->detail)) == NULL) {
-            /* This is not a real error since we can have release and
-             * non-release keybindings. On a KeyPress event for which there is
-             * only a !release-binding, but no release-binding, the
-             * corresponding KeyRelease event will trigger this. No problem,
-             * though. */
-            DLOG("Could not lookup key binding (modifiers %d, keycode %d)\n",
-                 state_filtered, event->detail);
-            return;
-        }
-    }
+    Binding *bind = get_binding_from_xcb_event((xcb_generic_event_t *)event);
+
+    /* if we couldn't find a binding, we are done */
+    if (bind == NULL)
+        return;
 
     char *command_copy = sstrdup(bind->command);
     struct CommandResult *command_output = parse_command(command_copy);

81	@@ -123,18 +123,18 @@ void grab_all_keys(xcb_connection_t *conn, bool bind_mode_switch) {
82	}
83
84	/*
85	- * Returns a pointer to the keyboard Binding with the specified modifiers and
86	+ * Returns a pointer to the Binding with the specified modifiers and
87	* keycode or NULL if no such binding exists.
88	*
89	*/
90	-Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_t keycode) {
91	+static Binding *get_binding(uint16_t modifiers, bool is_release, uint16_t input_code, input_type_t input_type) {
92	Binding *bind;
93
94	- if (!key_release) {
95	- /* On a KeyPress event, we first reset all
96	- * B_UPON_KEYRELEASE_IGNORE_MODS bindings back to B_UPON_KEYRELEASE */
97	+ if (!is_release) {
98	+ /* On a press event, we first reset all B_UPON_KEYRELEASE_IGNORE_MODS
99	+ * bindings back to B_UPON_KEYRELEASE */
100	TAILQ_FOREACH(bind, bindings, bindings) {
101	- if (bind->input_type != B_KEYBOARD)
102	+ if (bind->input_type != input_type)
103	continue;
104	if (bind->release == B_UPON_KEYRELEASE_IGNORE_MODS)
105	bind->release = B_UPON_KEYRELEASE;
106	@@ -145,37 +145,37 @@ Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_
107	/* First compare the modifiers (unless this is a
108	* B_UPON_KEYRELEASE_IGNORE_MODS binding and this is a KeyRelease
109	* event) */
110	- if (bind->input_type != B_KEYBOARD)
111	+ if (bind->input_type != input_type)
112	continue;
113	if (bind->mods != modifiers &&
114	(bind->release != B_UPON_KEYRELEASE_IGNORE_MODS \|\|
115	- !key_release))
116	+ !is_release))
117	continue;
118
119	- /* If a symbol was specified by the user, we need to look in
120	- * the array of translated keycodes for the event’s keycode */
121	- if (bind->symbol != NULL) {
122	+ /* For keyboard bindings where a symbol was specified by the user, we
123	+ * need to look in the array of translated keycodes for the event’s
124	+ * keycode */
125	+ if (input_type == B_KEYBOARD && bind->symbol != NULL) {
126	if (memmem(bind->translated_to,
127	bind->number_keycodes * sizeof(xcb_keycode_t),
128	- &keycode, sizeof(xcb_keycode_t)) == NULL)
129	+ &input_code, sizeof(xcb_keycode_t)) == NULL)
130	continue;
131	} else {
132	/* This case is easier: The user specified a keycode */
133	- if (bind->keycode != keycode)
134	+ if (bind->keycode != input_code)
135	continue;
136	}
137
138	- /* If this keybinding is a KeyRelease binding, it matches the key which
139	- * the user pressed. We therefore mark it as
140	- * B_UPON_KEYRELEASE_IGNORE_MODS for later, so that the user can
141	- * release the modifiers before the actual key and the KeyRelease will
142	- * still be matched. */
143	- if (bind->release == B_UPON_KEYRELEASE && !key_release)
144	+ /* If this binding is a release binding, it matches the key which the
145	+ * user pressed. We therefore mark it as B_UPON_KEYRELEASE_IGNORE_MODS
146	+ * for later, so that the user can release the modifiers before the
147	+ * actual key or button and the release event will still be matched. */
148	+ if (bind->release == B_UPON_KEYRELEASE && !is_release)
149	bind->release = B_UPON_KEYRELEASE_IGNORE_MODS;
150
151	- /* Check if the binding is for a KeyPress or a KeyRelease event */
152	- if ((bind->release == B_UPON_KEYPRESS && key_release) \|\|
153	- (bind->release >= B_UPON_KEYRELEASE && !key_release))
154	+ /* Check if the binding is for a press or a release event */
155	+ if ((bind->release == B_UPON_KEYPRESS && is_release) \|\|
156	+ (bind->release >= B_UPON_KEYRELEASE && !is_release))
157	continue;
158
159	break;
160	@@ -185,6 +185,59 @@ Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_
161	}
162
163	/*
164	+ * Returns a pointer to the Binding that matches the given xcb button or key
165	+ * event or NULL if no such binding exists.
166	+ *
167	+ */
168	+Binding get_binding_from_xcb_event(xcb_generic_event_t event) {
169	+ bool is_release = (event->response_type == XCB_KEY_RELEASE
170	+ \|\| event->response_type == XCB_BUTTON_RELEASE);
171	+
172	+ input_type_t input_type = ((event->response_type == XCB_BUTTON_RELEASE
173	+ \|\| event->response_type == XCB_BUTTON_PRESS)
174	+ ? B_MOUSE
175	+ : B_KEYBOARD);
176	+
177	+ uint16_t event_state = ((xcb_key_press_event_t *)event)->state;
178	+ uint16_t event_detail = ((xcb_key_press_event_t *)event)->detail;
179	+
180	+ /* Remove the numlock bit, all other bits are modifiers we can bind to */
181	+ uint16_t state_filtered = event_state & ~(xcb_numlock_mask \| XCB_MOD_MASK_LOCK);
182	+ DLOG("(removed numlock, state = %d)\n", state_filtered);
183	+ /* Only use the lower 8 bits of the state (modifier masks) so that mouse
184	+ * button masks are filtered out */
185	+ state_filtered &= 0xFF;
186	+ DLOG("(removed upper 8 bits, state = %d)\n", state_filtered);
187	+
188	+ if (xkb_current_group == XkbGroup2Index)
189	+ state_filtered \|= BIND_MODE_SWITCH;
190	+
191	+ DLOG("(checked mode_switch, state %d)\n", state_filtered);
192	+
193	+ /* Find the binding */
194	+ Binding *bind = get_binding(state_filtered, is_release, event_detail, input_type);
195	+
196	+ /* No match? Then the user has Mode_switch enabled but does not have a
197	+ * specific keybinding. Fall back to the default keybindings (without
198	+ * Mode_switch). Makes it much more convenient for users of a hybrid
199	+ * layout (like ru). */
200	+ if (bind == NULL) {
201	+ state_filtered &= ~(BIND_MODE_SWITCH);
202	+ DLOG("no match, new state_filtered = %d\n", state_filtered);
203	+ if ((bind = get_binding(state_filtered, is_release, event_detail, input_type)) == NULL) {
204	+ /* This is not a real error since we can have release and
205	+ * non-release bindings. On a press event for which there is only a
206	+ * !release-binding, but no release-binding, the corresponding
207	+ * release event will trigger this. No problem, though. */
208	+ DLOG("Could not lookup key binding (modifiers %d, keycode %d)\n",
209	+ state_filtered, event_detail);
210	+ }
211	+ }
212	+
213	+ return bind;
214	+}
215	+
216	+/*
217	* Translates keysymbols to keycodes for all bindings which use keysyms.
218	*
219	*/

224	@@ -70,40 +70,11 @@ void handle_key_press(xcb_key_press_event_t *event) {
225
226	DLOG("%s %d, state raw = %d\n", (key_release ? "KeyRelease" : "KeyPress"), event->detail, event->state);
227
228	- /* Remove the numlock bit, all other bits are modifiers we can bind to */
229	- uint16_t state_filtered = event->state & ~(xcb_numlock_mask \| XCB_MOD_MASK_LOCK);
230	- DLOG("(removed numlock, state = %d)\n", state_filtered);
231	- /* Only use the lower 8 bits of the state (modifier masks) so that mouse
232	- * button masks are filtered out */
233	- state_filtered &= 0xFF;
234	- DLOG("(removed upper 8 bits, state = %d)\n", state_filtered);
235	-
236	- if (xkb_current_group == XkbGroup2Index)
237	- state_filtered \|= BIND_MODE_SWITCH;
238	-
239	- DLOG("(checked mode_switch, state %d)\n", state_filtered);
240	-
241	- /* Find the binding */
242	- Binding *bind = get_keyboard_binding(state_filtered, key_release, event->detail);
243	-
244	- /* No match? Then the user has Mode_switch enabled but does not have a
245	- * specific keybinding. Fall back to the default keybindings (without
246	- * Mode_switch). Makes it much more convenient for users of a hybrid
247	- * layout (like us, ru). */
248	- if (bind == NULL) {
249	- state_filtered &= ~(BIND_MODE_SWITCH);
250	- DLOG("no match, new state_filtered = %d\n", state_filtered);
251	- if ((bind = get_keyboard_binding(state_filtered, key_release, event->detail)) == NULL) {
252	- /* This is not a real error since we can have release and
253	- * non-release keybindings. On a KeyPress event for which there is
254	- * only a !release-binding, but no release-binding, the
255	- * corresponding KeyRelease event will trigger this. No problem,
256	- * though. */
257	- DLOG("Could not lookup key binding (modifiers %d, keycode %d)\n",
258	- state_filtered, event->detail);
259	- return;
260	- }
261	- }
262	+ Binding bind = get_binding_from_xcb_event((xcb_generic_event_t )event);
263	+
264	+ /* if we couldn't find a binding, we are done */
265	+ if (bind == NULL)
266	+ return;
267
268	char *command_copy = sstrdup(bind->command);
269	struct CommandResult *command_output = parse_command(command_copy);

29	@@ -31,11 +31,11 @@ Binding configure_binding(const char bindtype, const char *modifiers, const ch
30	void grab_all_keys(xcb_connection_t *conn, bool bind_mode_switch);
31
32	/**
33	- * Returns a pointer to the keyboard Binding with the specified modifiers and
34	- * keycode or NULL if no such binding exists.
35	+ * Returns a pointer to the Binding that matches the given xcb event or NULL if
36	+ * no such binding exists.
37	*
38	*/
39	-Binding *get_keyboard_binding(uint16_t modifiers, bool key_release, xcb_keycode_t keycode);
40	+Binding get_binding_from_xcb_event(xcb_generic_event_t event);
41
42	/**
43	* Translates keysymbols to keycodes for all bindings which use keysyms.

48	@@ -92,6 +92,14 @@ typedef enum {
49	} layout_t;
50
51	/**
52	+ * Binding input types. See Binding::input_type.
53	+ */
54	+typedef enum {
55	+ B_KEYBOARD = 0,
56	+ B_MOUSE = 1
57	+} input_type_t;
58	+
59	+/**
60	* Stores a rectangle, for example the size of a window, the child window etc.
61	* It needs to be packed so that the compiler will not add any padding bytes.
62	* (it is used in src/ewmh.c for example)
63	@@ -215,12 +223,7 @@ struct regex {
64	struct Binding {
65	/* The type of input this binding is for. (Mouse bindings are not yet
66	* implemented. All bindings are currently assumed to be keyboard bindings.) */
67	- enum {
68	- /* Created with "bindsym", "bindcode", and "bind" */
69	- B_KEYBOARD = 0,
70	- /* Created with "bindmouse" (not yet implemented). */
71	- B_MOUSE = 1,
72	- } input_type;
73	+ input_type_t input_type;
74
75	/** If true, the binding should be executed upon a KeyRelease event, not a
76	* KeyPress (the default). */