[PATCH weston] input: don't send to clients key events eaten by bindings

[Giulio Camuffo]

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(-)

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;
- *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 &&

[Bill Spitzak]

I really can't imagine this is a problem, and clients may even expect
the current behavior. It certainly is expected for shift keys: if I
alt+tab to a program it acts like alt is still held down. A quick test
on Ubuntu shows that a program you alt+tab to gets a map with both alt
and tab held down.

Does this have something to do with preventing the key from repeating?

Post by Giulio Camuffo
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.

[Giulio Camuffo]

I really can't imagine this is a problem, and clients may even expect the
current behavior. It certainly is expected for shift keys: if I alt+tab to a
program it acts like alt is still held down. A quick test on Ubuntu shows
that a program you alt+tab to gets a map with both alt and tab held down.

I don't really care much how it works on Ubuntu, honestly. This is a
problem because the key binding is supposed to not let the key
press/release get to the clients, but this isn't what is happening in
reality. Modifiers keys are not affected by this change.


--
Giulio

Does this have something to do with preventing the key from repeating?

Post by Giulio Camuffo
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.

_______________________________________________
wayland-devel mailing list
wayland-devel at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[Bill Spitzak]

Post by Giulio Camuffo

I really can't imagine this is a problem, and clients may even expect the
current behavior. It certainly is expected for shift keys: if I alt+tab to a
program it acts like alt is still held down. A quick test on Ubuntu shows
that a program you alt+tab to gets a map with both alt and tab held down.

I don't really care much how it works on Ubuntu, honestly. This is a
problem because the key binding is supposed to not let the key
press/release get to the clients, but this isn't what is happening in
reality. Modifiers keys are not affected by this change.

A key being held down on focus-in will not produce a "press" event, so I
don't see what the problem is.

If a client wants to check if X is held down I think the user expects it
to work even if it was held down as part of the command to switch
windows. Therefore I think the current behavior is correct.

All I can guess is that this is having some problem with the way wayland
is implemented. Is it causing unwanted key repeats? Or preventing them
when they are wanted?? Or are you worried about the client getting a
"release" event?

[Giulio Camuffo]

Post by Bill Spitzak

Post by Giulio Camuffo

I really can't imagine this is a problem, and clients may even expect the
current behavior. It certainly is expected for shift keys: if I alt+tab to a
program it acts like alt is still held down. A quick test on Ubuntu shows
that a program you alt+tab to gets a map with both alt and tab held down.

I don't really care much how it works on Ubuntu, honestly. This is a
problem because the key binding is supposed to not let the key
press/release get to the clients, but this isn't what is happening in
reality. Modifiers keys are not affected by this change.

A key being held down on focus-in will not produce a "press" event, so I
don't see what the problem is.

It produces a KeyPress in xwayland.

Post by Bill Spitzak
If a client wants to check if X is held down I think the user expects it to
work even if it was held down as part of the command to switch windows.
Therefore I think the current behavior is correct.

Why would it care about that? The compositor decided that the clients
shouldn't know that one key was pressed, so the client's shouldn't
know that. If some client gets to know that it's a bug.
Or else you should say that key bindings should always let the events
pass to clients. Are you saying that?

Post by Bill Spitzak
All I can guess is that this is having some problem with the way wayland is
implemented. Is it causing unwanted key repeats? Or preventing them when
they are wanted?? Or are you worried about the client getting a "release"
event?

[Bill Spitzak]

Post by Giulio Camuffo

Post by Bill Spitzak
A key being held down on focus-in will not produce a "press" event, so I
don't see what the problem is.

It produces a KeyPress in xwayland.

Okay that may be a problem. So you are saying the client cannot
distinguish between keys that were held down when it got the keyboard
focus, and ones that were pressed a very short time afterwards?

I was under the impression that what the client got was an xkb state
that showed that the keys were held down. Actual events caused by
pressing keys were different and distinguishable by the client.

Post by Giulio Camuffo

Post by Bill Spitzak
If a client wants to check if X is held down I think the user expects it to
work even if it was held down as part of the command to switch windows.
Therefore I think the current behavior is correct.

Why would it care about that? The compositor decided that the clients
shouldn't know that one key was pressed, so the client's shouldn't
know that. If some client gets to know that it's a bug.
Or else you should say that key bindings should always let the events
pass to clients. Are you saying that?

No. All I want is the client to know the current state of the device,
including the actual state of the keys. They should not see the keypress
any more than if the key was pressed in a different client and held down
until the focus switched to this one.

The most obvious one clients want are shift keys. If I type Alt+tab to
go to a different client and keep holding down Alt and type 'x', the
client should act like I typed Alt+x.

Yes I know this works because the modifier bits are being treated
differently than the key map. However I don't think that should be
special. What happens if everybody starts treating hold-down-space as a
modifier (this actually has some precedence in Photoshop and lots of 3D
software)?

[Jasper St. Pierre]

Post by Bill Spitzak
A key being held down on focus-in will not produce a "press" event, so I

Post by Giulio Camuffo

Post by Bill Spitzak
don't see what the problem is.

It produces a KeyPress in xwayland.

Okay that may be a problem. So you are saying the client cannot
distinguish between keys that were held down when it got the keyboard
focus, and ones that were pressed a very short time afterwards?
I was under the impression that what the client got was an xkb state that
showed that the keys were held down. Actual events caused by pressing keys
were different and distinguishable by the client.

X clients cannot distinguish between the two.

Post by Bill Spitzak
If a client wants to check if X is held down I think the user expects it

Post by Giulio Camuffo

Post by Bill Spitzak
to
work even if it was held down as part of the command to switch windows.
Therefore I think the current behavior is correct.

Why would it care about that? The compositor decided that the clients
shouldn't know that one key was pressed, so the client's shouldn't
know that. If some client gets to know that it's a bug.
Or else you should say that key bindings should always let the events
pass to clients. Are you saying that?

No. All I want is the client to know the current state of the device,
including the actual state of the keys. They should not see the keypress
any more than if the key was pressed in a different client and held down
until the focus switched to this one.
The most obvious one clients want are shift keys. If I type Alt+tab to go
to a different client and keep holding down Alt and type 'x', the client
should act like I typed Alt+x.

Too bad. We cannot go back in time 40 years and redesign X11 to be how you
want it. We wouldn't be here in the first place if we could.

Post by Bill Spitzak
Yes I know this works because the modifier bits are being treated
differently than the key map. However I don't think that should be special.
What happens if everybody starts treating hold-down-space as a modifier
(this actually has some precedence in Photoshop and lots of 3D software)?
_______________________________________________
wayland-devel mailing list
wayland-devel at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[Bill Spitzak]

Post by Bill Spitzak
I was under the impression that what the client got was an xkb state
that showed that the keys were held down. Actual events caused by
pressing keys were different and distinguishable by the client.
X clients cannot distinguish between the two.

Okay I must not understand what you are doing. An X client can easily
distinguish between KeymapNotify and KeyPress events!

Post by Bill Spitzak
Too bad. We cannot go back in time 40 years and redesign X11 to be how
you want it. We wouldn't be here in the first place if we could.

Huh? What I want is how X and Windows and OS/X work right now!

I suspect I have misunderstood what you are trying to do since this is
making no sense to me. I thought this patch was to remove the fact that
the key is held down in the xkb info being sent when the client gets
keyboard focus. All I can guess is that you are doing something else but
I am not clear what, since it is already true that the client does not
get key press events.

[Giulio Camuffo]

Oh, i didn't realize we dropped the ML. adding again.

Are you proposing something else?

IsSpaceKeyDown() should always return false.
What I'm looking for here is consistent behaviour. What happens in your
example if the user releases and presses space again? The compositor will
switch to another window, and the client will not do anymore what it is
supposed to do with space. That's broken behavior, and the user will wonder
why sometimes space works and some other time it has another effect
entirely.

I still think we are just not agreeing on terms because I am quite confused
as to what you are saying.
The client will not get an keypress event for the space. Therefore if they
have some action for the space key itself they will not do it. If typing
space is used by the compositor to switch focus then the client will *never*
see a keypress for space because the compositor will not deliver a keypress
event for space to any client. It is consistent in that space *never* works.
However imagine the client has a "hold down space and type 'x'" action. This
*should* work. This is because when the user types 'x' the client gets a
keypress event for 'x', and then uses the IsSpaceKeyDown() to determine that
space is held down, that returns true, and does this action.

Ok, so let's change the example. Space doesn't switch the active
window, but toggles the speaker mute. So hitting space will not send a
key event to the client, which will not know space is pressed, so your
nice space+x shortcut won't work.
If the user presses space and switches to another window before
releasing it, the shortcut will suddenly and inexplicably work, but
the user will be able to only use it when coming from another window.
Hardly a reliable feature, I would just call it broken and I don't
think we should implement broken behavior. The fact that X does so is
irrelevant.

I'm quite certain this is exactly what users expect. For an actual
real-world example if you type Alt+Tab in some window managers the app that
gets the focus acts like Alt is held down. I know you said this already
works for modifier keys, but I then don't understand why you don't want this
to work for other keys.

[Bill Spitzak]

Post by Giulio Camuffo
Ok, so let's change the example. Space doesn't switch the active
window, but toggles the speaker mute. So hitting space will not send a
key event to the client, which will not know space is pressed, so your
nice space+x shortcut won't work.

Yes this is what I am trying to fix. These is a much better example
because it does not confuse things with focus changes, and also makes it
more obvious where the error is.

The desired result is that the user presses space and the speaker turns
off. If they continue to hold down space and type 'x' then the client
should act exactly like it is supposed to for the space+x shortcut. If
it acts like 'x' without the space this is a serious failure to deliver
expected results for the user.

The client will do some kind of check to see if the space key is down.
This should resemble as much as possible a round trip to actually see if
the key is pressed on the hardware. I figured this involved peeking into
the xkb structure to see what keys are held down.

But with the current design of the xkb interface this is going to return
false, as changing the key-down map apparently is tied directly to key
press/release events and to the focus changing events. There is no way
to send changes to the key map without also sending one of these events.
This needs to be fixed.

I now think what happened is that somebody realized that a focus-change
would fix this bug (since it sends the updated key pressed map to the
client as a side-effect). However the patch is exactly backwards, it is
trying to break it in the one case where it works correctly!

[Giulio Camuffo]

Post by Giulio Camuffo
Ok, so let's change the example. Space doesn't switch the active
window, but toggles the speaker mute. So hitting space will not send a
key event to the client, which will not know space is pressed, so your
nice space+x shortcut won't work.

Yes this is what I am trying to fix. These is a much better example because
it does not confuse things with focus changes, and also makes it more
obvious where the error is.
The desired result is that the user presses space and the speaker turns off.
If they continue to hold down space and type 'x' then the client should act
exactly like it is supposed to for the space+x shortcut. If it acts like 'x'
without the space this is a serious failure to deliver expected results for
the user.

This is just broken. Assuming this worked, the user would toggle the
speaker mute every time he tries to run that client shortcut.

The client will do some kind of check to see if the space key is down. This
should resemble as much as possible a round trip to actually see if the key
is pressed on the hardware. I figured this involved peeking into the xkb
structure to see what keys are held down.

"some kind of check". Making a roundtrip is not acceptable.

But with the current design of the xkb interface this is going to return
false, as changing the key-down map apparently is tied directly to key
press/release events and to the focus changing events. There is no way to
send changes to the key map without also sending one of these events. This
needs to be fixed.
I now think what happened is that somebody realized that a focus-change
would fix this bug (since it sends the updated key pressed map to the client
as a side-effect). However the patch is exactly backwards, it is trying to
break it in the one case where it works correctly!

So, you after all have no objection to this patch, but to the general
way key bindings are implemented in Weston.
Please, find a way to make them work and send a patch.

[Bill Spitzak]

Post by Giulio Camuffo

The client will do some kind of check to see if the space key is down. This
should resemble as much as possible a round trip to actually see if the key
is pressed on the hardware. I figured this involved peeking into the xkb
structure to see what keys are held down.

"some kind of check". Making a roundtrip is not acceptable.

Yes this is why I expect it will check the local key-down map and that
there are events that update it.

Post by Giulio Camuffo
So, you after all have no objection to this patch, but to the general
way key bindings are implemented in Weston.
Please, find a way to make them work and send a patch.

I have an objection to this patch in that it moves things further from
what I think is correct behavior.

I agree that a patch to add the necessary event would help.

[Pekka Paalanen]

On Tue, 7 Oct 2014 22:30:25 +0300

Post by Giulio Camuffo
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.

Post by Giulio Camuffo
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 <***@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 <***@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?

Post by Giulio Camuffo
- *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

[Pekka Paalanen]

On Wed, 5 Nov 2014 16:23:29 +0200

Post by Pekka Paalanen
On Tue, 7 Oct 2014 22:30:25 +0300

Post by Giulio Camuffo
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.
- 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.

...

Post by Pekka Paalanen
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.

Sorry, the test case is correct, I just did it wrong. Here's how it
really works:

- 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
- 'k' starts repeating in xterm

The 'k' repeating continues even if you release Mod and 'k', until
keyboard leaves the xterm or you press another key.


Thanks,
pq

[Bill Spitzak]

It seems like it would be easy for the client to not repeat the key if
it did not see the key-down event. I find it pretty amazing that you
duplicated about the only bug that having clients do the repeat rather
than the compositor solves.

Any patch that removes the fact that the key is still being held down by
the user from the key array is incorrect.

Post by Pekka Paalanen
- 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.