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

Submitted by Giulio Camuffo on Oct. 7, 2014, 7:30 p.m.

Details

Message ID 1412710225-11243-1-git-send-email-giuliocamuffo@gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Giulio Camuffo Oct. 7, 2014, 7:30 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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?

On 10/07/2014 12:30 PM, Giulio Camuffo 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.
2014-10-08 0:50 GMT+03:00 Bill Spitzak <spitzak@gmail.com>:
> 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?
>
> On 10/07/2014 12:30 PM, Giulio Camuffo 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.
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On 10/08/2014 01:41 AM, Giulio Camuffo wrote:
> 2014-10-08 0:50 GMT+03:00 Bill Spitzak <spitzak@gmail.com>:
>> 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?
2014-10-08 21:00 GMT+03:00 Bill Spitzak <spitzak@gmail.com>:
>
>
> On 10/08/2014 01:41 AM, Giulio Camuffo wrote:
>>
>> 2014-10-08 0:50 GMT+03:00 Bill Spitzak <spitzak@gmail.com>:
>>>
>>> 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.

>
> 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?

>
> 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?
On 10/08/2014 11:58 AM, Giulio Camuffo wrote:

>> 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.

>> 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)?
On Wed, Oct 8, 2014 at 3:23 PM, Bill Spitzak <spitzak@gmail.com> wrote:

> On 10/08/2014 11:58 AM, Giulio Camuffo wrote:
>
>  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.
>

X clients cannot distinguish between the two.


>  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.
>

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.


> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On 10/08/2014 03:25 PM, Jasper St. Pierre wrote:

>     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!

> 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.
Oh, i didn't realize we dropped the ML. adding again.

2014-10-10 21:17 GMT+03:00 Bill Spitzak <spitzak@gmail.com>:
> On 10/09/2014 11:42 PM, giuliocamuffo@gmail.com wrote:
>
>>> 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.
On 10/10/2014 11:41 AM, Giulio Camuffo wrote:

> 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!
2014-10-10 23:11 GMT+03:00 Bill Spitzak <spitzak@gmail.com>:
> On 10/10/2014 11:41 AM, Giulio Camuffo wrote:
>
>> 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.
>> 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.
> 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.