[PATCH weston] input: don't send to clients key events eaten by bindings
Pekka Paalanen
ppaalanen at gmail.com
Wed Nov 5 06:23:29 PST 2014
On Tue, 7 Oct 2014 22:30:25 +0300
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> weston key bindings are supposed to eat the key events, and not pass it
> on to clients, and indeed the wl_keyboard.key event is not sent. But
> we must also not put the key in the keys array to pass to client with
> the wl_keyboard.enter event, or else we may send the 'eaten' one too.
> In the case of a key binding hiding a surface having the keyboard focus,
> the shell may decide to give the focus to another surface, but that will
> happen before the key is released, so the new focus surface will receive
> the code of the bound key in the wl_keyboard.enter array.
> ---
> src/bindings.c | 7 +++++--
> src/compositor.h | 2 +-
> src/input.c | 35 +++++++++++++++++++----------------
> 3 files changed, 25 insertions(+), 19 deletions(-)
Ok, reproducing this problem with visible effects requires Xwayland.
Here's how I did it:
- open xterm (via Xwayland)
- open weston-terminal
- press Mod
- press 'k' (Mod+k is a weston hotkey for killing an app)
- weston-terminal gets killed, focus moves to xterm
- before xterm gets killed too, release Mod
- 'k' starts repeating in xterm until you release 'k'
The premise here is that since 'k' was the final key that triggered a
keybinding, the press and release of 'k' must not be sent to any client.
This obviously fails in the above experiment, not via a key-press event
but via the list of pressed keys in keyboard enter event. (You cannot
see it with WAYLAND_DEBUG, because it cannot decode wl_array type.)
So, problem confirmed.
> diff --git a/src/bindings.c b/src/bindings.c
> index 5e24725..452d224 100644
> --- a/src/bindings.c
> +++ b/src/bindings.c
> @@ -255,16 +255,17 @@ install_binding_grab(struct weston_seat *seat, uint32_t time, uint32_t key)
> weston_keyboard_start_grab(seat->keyboard, &grab->grab);
> }
>
> -WL_EXPORT void
> +WL_EXPORT int
> weston_compositor_run_key_binding(struct weston_compositor *compositor,
> struct weston_seat *seat,
> uint32_t time, uint32_t key,
> enum wl_keyboard_key_state state)
> {
> struct weston_binding *b, *tmp;
> + int eaten = 0;
>
> if (state == WL_KEYBOARD_KEY_STATE_RELEASED)
> - return;
> + return 0;
>
> /* Invalidate all active modifier bindings. */
> wl_list_for_each(b, &compositor->modifier_binding_list, link)
> @@ -281,8 +282,10 @@ weston_compositor_run_key_binding(struct weston_compositor *compositor,
> if (seat->keyboard->grab ==
> &seat->keyboard->default_grab)
> install_binding_grab(seat, time, key);
> + ++eaten;
> }
> }
> + return eaten;
> }
>
> WL_EXPORT void
> diff --git a/src/compositor.h b/src/compositor.h
> index 44379fe..5b6f33b 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -1140,7 +1140,7 @@ weston_binding_destroy(struct weston_binding *binding);
> void
> weston_binding_list_destroy_all(struct wl_list *list);
>
> -void
> +int
> weston_compositor_run_key_binding(struct weston_compositor *compositor,
> struct weston_seat *seat, uint32_t time,
> uint32_t key,
> diff --git a/src/input.c b/src/input.c
> index 530904d..d969949 100644
> --- a/src/input.c
> +++ b/src/input.c
> @@ -1316,6 +1316,7 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
> struct weston_keyboard *keyboard = seat->keyboard;
> struct weston_keyboard_grab *grab = keyboard->grab;
> uint32_t *k, *end;
> + int eaten = 0;
>
> if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> weston_compositor_idle_inhibit(compositor);
> @@ -1325,28 +1326,30 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
> weston_compositor_idle_release(compositor);
> }
>
> - end = keyboard->keys.data + keyboard->keys.size;
> - for (k = keyboard->keys.data; k < end; k++) {
> - if (*k == key) {
> - /* Ignore server-generated repeats. */
> - if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
> - return;
This early return worries me. There must have been some reason this is
here, but the commit that added it says little:
commit c6587ea1553f4b8f58a70047fba91f203e03ecfa
Author: Daniel Stone <daniel at fooishbar.org>
Date: Fri Jun 22 13:21:31 2012 +0100
Ignore repeat keys in notify_key
Let compositors just blithely post through every event they get,
including repeating keys.
Signed-off-by: Daniel Stone <daniel at fooishbar.org>
diff --git a/src/compositor.c b/src/compositor.c
index d914b67..52ef89e 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -1858,8 +1858,12 @@ notify_key(struct wl_seat *seat, uint32_t time, uint32_t key,
mods = update_modifier_state(ws, key, state);
end = seat->keyboard->keys.data + seat->keyboard->keys.size;
for (k = seat->keyboard->keys.data; k < end; k++) {
- if (*k == key)
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return;
*k = *--end;
+ }
}
seat->keyboard->keys.size = (void *) end - seat->keyboard->keys.data;
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
To me it looks like this code protects against repeated notify_key()
calls with the same key pressed. I'd assume it's for the x11 backend,
but I cannot make it repeat.
When you move this code to below, key bindings are no longer protected
from the repeat, I think.
But do we even need to protect against that repeat?
> - *k = *--end;
> - }
> - }
> - keyboard->keys.size = (void *) end - keyboard->keys.data;
> - if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> - k = wl_array_add(&keyboard->keys, sizeof *k);
> - *k = key;
> - }
> -
> if (grab == &keyboard->default_grab ||
> grab == &keyboard->input_method_grab) {
> - weston_compositor_run_key_binding(compositor, seat, time, key,
> + eaten = weston_compositor_run_key_binding(compositor, seat, time, key,
> state);
> grab = keyboard->grab;
> }
>
> + if (eaten == 0) {
> + end = keyboard->keys.data + keyboard->keys.size;
> + for (k = keyboard->keys.data; k < end; k++) {
> + if (*k == key) {
> + /* Ignore server-generated repeats. */
> + if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
> + return;
> + *k = *--end;
> + }
> + }
> + keyboard->keys.size = (void *) end - keyboard->keys.data;
> + if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> + k = wl_array_add(&keyboard->keys, sizeof *k);
> + *k = key;
> + }
> + }
> +
> grab->interface->key(grab, time, key, state);
>
> if (keyboard->pending_keymap &&
This patch does fix the problem I confirmed, there is that one detail
I'm unsure about.
I also have another worry. A swallowed key will not be in the keys
array, and I think there are other places that also use the keys array,
other than just sending it with the keyboard enter event. Could
something break there?
Looks like notify_keyboard_focus_out() could fail to call idle_release
enough times.
I assume a key-release can never be swallowed. If it could, it would
be another case for the keys array to become out of date.
Maybe Daniel could comment?
Hmm, btw, why does the repeating of 'k' stop when I release the key in
my experiment? I suppose the grab ends when I release the Mod key, so
the release event gets sent. And that's actually correct in that case,
because on keyboard enter the 'k' was listed depressed. So my test case
is actually wrong! ...is it? This is starting to hurt my head now.
Thanks,
pq
More information about the wayland-devel
mailing list