[PATCH weston] input: don't send to clients key events eaten by bindings
Pekka Paalanen
ppaalanen at gmail.com
Mon Nov 10 07:15:52 PST 2014
On Wed, 5 Nov 2014 20:51:53 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 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.
No, _focus_in() is different. It gets the list of keys from the caller,
and simply overwrites the whole list we had internally. So it doesn't
use the internal list at all.
That's pretty logical: the pressed keys can very likely change while we
are out.
That's not even a problem in _focus_in(). It only needs to bump the
inhibit counter as many times as there are keys down. It assumes it
starts from no keys down wrt. to the counter.
_focus_out() OTOH is responsible for making all keys reduce the
counter. If there is a mismatch between a key-down bumping up, and
focus_out bumping down, the counter cannot ever reach zero anymore, and
so weston will never hit idle timeout anymore.
It is not _focus_in() that needs to match with _focus_out(),
but active-time key-downs need to match _focus_out(), and _focus_in()
needs to match active-time key-ups.
Did I get something wrong here?
Thanks,
pq
More information about the wayland-devel
mailing list