[PATCH weston] input: don't send to clients key events eaten by bindings
Giulio Camuffo
giuliocamuffo at gmail.com
Wed Nov 5 10:51:53 PST 2014
2014-11-05 16:23 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> 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.
You're right, i'll move that for back before the binding call.
>
> 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.
>
Yes, but also notify_keyboard_focus_in() calls
weston_compositor_idle_inhibit() with only the keys in the array, so
it shouldn't be a problem.
Thanks for the review,
Giulio
> 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