Discussion:
[PATCH weston] input: don't send to clients key events eaten by bindings
(too old to reply)
Giulio Camuffo
2014-10-07 19:30:25 UTC
Permalink
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 &&
--
2.1.2
Bill Spitzak
2014-10-07 21:50:51 UTC
Permalink
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
2014-10-08 08:41:04 UTC
Permalink
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
2014-10-08 18:00:25 UTC
Permalink
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
2014-10-08 18:58:37 UTC
Permalink
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
2014-10-08 22:23:35 UTC
Permalink
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
2014-10-08 22:25:30 UTC
Permalink
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
--
Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141008/46ca379f/attachment.html>
Bill Spitzak
2014-10-08 22:53:42 UTC
Permalink
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
2014-10-10 18:41:20 UTC
Permalink
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
2014-10-10 20:11:08 UTC
Permalink
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
2014-10-10 20:24:58 UTC
Permalink
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
2014-10-11 01:32:15 UTC
Permalink
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
2014-11-05 14:23:29 UTC
Permalink
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
2014-11-05 14:50:58 UTC
Permalink
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
2014-11-05 21:16:31 UTC
Permalink
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.
Giulio Camuffo
2014-11-05 18:51:53 UTC
Permalink
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 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
commit c6587ea1553f4b8f58a70047fba91f203e03ecfa
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.
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.
Post by Pekka Paalanen
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.
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
Post by Pekka Paalanen
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
2014-11-10 15:15:52 UTC
Permalink
On Wed, 5 Nov 2014 20:51:53 +0200
Post by Giulio Camuffo
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 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
commit c6587ea1553f4b8f58a70047fba91f203e03ecfa
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.
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.
Post by Pekka Paalanen
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.
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
Giulio Camuffo
2014-11-06 17:55:33 UTC
Permalink
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.
---


v2: made sure not to insert the same key in the keys array multiple times

src/bindings.c | 7 +++++--
src/compositor.h | 3 ++-
src/input.c | 48 ++++++++++++++++++++++++++++++++----------------
3 files changed, 39 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;

struct wl_array keys;
+ struct wl_array eaten_keys;

struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..17afcbb 100644
--- a/src/input.c
+++ b/src/input.c
@@ -1307,6 +1307,24 @@ update_keymap(struct weston_seat *seat)
}
#endif

+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1333,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;

if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,28 +1345,24 @@ 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 (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;

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;
}

+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);

if (keyboard->pending_keymap &&
--
2.1.3
Bill Spitzak
2014-11-06 19:48:02 UTC
Permalink
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.
Why can't the client distinguish this from an actual key press after it
got the focus?
Giulio Camuffo
2014-11-06 19:55:38 UTC
Permalink
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.
Why can't the client distinguish this from an actual key press after it got
the focus?
It may, but that's not the point. The key was eaten by the compositor
binding, it must not be sent to the client.
Bill Spitzak
2014-11-06 23:57:37 UTC
Permalink
Post by Giulio Camuffo
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.
Why can't the client distinguish this from an actual key press after it got
the focus?
It may, but that's not the point. The key was eaten by the compositor
binding, it must not be sent to the client.
The key is not being "sent". A map of what keys are pushed down right
now is what is sent.

The user certainly expects the client to act like the key is held down
for shift keys, and because of this you are sending an inconsistent
shift state that does not match the key-down state.

What if a client considers the key that triggered the event to be a
modifier and acts different if held down? The user is certainly holding
it down, so the client will not act correctly if it does not know this.

More of a concern, what if the user accidentally thinks they have to
hold down some unrelated key like 'x'? This will mean that the x will
repeat, because it was not "used", but the user certainly cannot see
this. Instead, all keys pressed before the action that transferred
keyboard focus must act identically, whether or not they were a command.
g***@gmail.com
2014-11-07 08:14:15 UTC
Permalink
Post by Bill Spitzak
Post by Giulio Camuffo
It may, but that's not the point. The key was eaten by the compositor
binding, it must not be sent to the client.
The key is not being "sent". A map of what keys are pushed down right
now is what is sent.
The user certainly expects the client to act like the key is held down
for shift keys, and because of this you are sending an inconsistent
shift state that does not match the key-down state.
What if a client considers the key that triggered the event to be a
modifier and acts different if held down? The user is certainly holding
it down, so the client will not act correctly if it does not know this.
More of a concern, what if the user accidentally thinks they have to
hold down some unrelated key like 'x'? This will mean that the x will
repeat, because it was not "used", but the user certainly cannot see
this. Instead, all keys pressed before the action that transferred
keyboard focus must act identically, whether or not they were a command.
Listen, there's no point in repeating the same thing over and over again. This doesn't work, as I've explained in http://lists.freedesktop.org/archives/wayland-devel/2014-October/017741.html

--
Sent from my Jolla
Bill Spitzak
2014-11-07 22:25:07 UTC
Permalink
You are right I was forgetting about the old discussion, and am getting
confused as to where the bug is.

IMHO the behavior when the client gets the focus currently is correct.
However it is broken when the client already has the focus and the
compositor gets a key. The patch you are proposing breaks the case that
works, rather than fixing the bad case.

The bug is "client with keyboard focus does not have correct values for
currently-pressed keys if if the compositor consumes a key event".
Currently the only way to change the client's key-down array is for it
to get a wl_keyboard::enter event, or a wl_keyboard::key event. So if
the client already has focus then the compositor has no way to update
the map to show the result of a key-down event that the compositor consumed.

My recommendation: compositor should send a redundant wl_keyboard::enter
event.

The client should also get a correct wl_keyboard::modifiers event.

The example with the Alt+x changing focus but then the x repeating when
alt is released has nothing to do with compositor shortcuts. A client
can have the exact same bug if Alt+x was a shortcut it handled itself.
If the repeating x is considered wrong, it is the client's responsibilty
to realize the Alt was pressed or held after the x was pressed and not
repeat the unshifted value.
Post by g***@gmail.com
Listen, there's no point in repeating the same thing over and over again. This doesn't work, as I've explained in http://lists.freedesktop.org/archives/wayland-devel/2014-October/017741.html
Pekka Paalanen
2014-11-10 15:13:09 UTC
Permalink
On Fri, 07 Nov 2014 14:25:07 -0800
Post by Bill Spitzak
The bug is "client with keyboard focus does not have correct values for
currently-pressed keys if if the compositor consumes a key event".
That is not a bug. That is the intended, correct behaviour.

- pq
Giulio Camuffo
2014-11-10 20:08:40 UTC
Permalink
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.
---

v3: call weston_compositor_idle_inhibit() for all the keys pressed

src/bindings.c | 7 +++++--
src/compositor.h | 3 ++-
src/input.c | 57 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 48 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;

struct wl_array keys;
+ struct wl_array eaten_keys;

struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..7819a41 100644
--- a/src/input.c
+++ b/src/input.c
@@ -1307,6 +1307,26 @@ update_keymap(struct weston_seat *seat)
}
#endif

+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1335,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;

if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,28 +1347,26 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;

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;
}

+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);

if (keyboard->pending_keymap &&
@@ -1430,6 +1450,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }

seat->modifier_state = 0;
--
2.1.3
Pekka Paalanen
2014-11-11 07:21:48 UTC
Permalink
On Mon, 10 Nov 2014 22:08:40 +0200
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.
---
v3: call weston_compositor_idle_inhibit() for all the keys pressed
src/bindings.c | 7 +++++--
src/compositor.h | 3 ++-
src/input.c | 57 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 48 insertions(+), 19 deletions(-)
Hi Giulio,

I would have committed this as is, but you forgot to add array init and
release for the new eaten_keys array.

Since we need a v4 anyway, I'll comment on the unimportant issues below,
too. ;-)
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;
Using 'return eaten' would have more documentary value, no?
Post by Giulio Camuffo
/* 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;
struct wl_array keys;
+ struct wl_array eaten_keys;
struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..7819a41 100644
--- a/src/input.c
+++ b/src/input.c
@@ -1307,6 +1307,26 @@ update_keymap(struct weston_seat *seat)
}
#endif
+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1335,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,28 +1347,26 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
And also pressed rather than released.
Post by Giulio Camuffo
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;
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);
Could use realignment.
Post by Giulio Camuffo
grab = keyboard->grab;
}
+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);
if (keyboard->pending_keymap &&
@@ -1430,6 +1450,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }
seat->modifier_state = 0;
Thanks,
pq
Giulio Camuffo
2014-11-11 08:00:15 UTC
Permalink
Post by Pekka Paalanen
On Mon, 10 Nov 2014 22:08:40 +0200
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.
---
v3: call weston_compositor_idle_inhibit() for all the keys pressed
src/bindings.c | 7 +++++--
src/compositor.h | 3 ++-
src/input.c | 57 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 48 insertions(+), 19 deletions(-)
Hi Giulio,
I would have committed this as is, but you forgot to add array init and
release for the new eaten_keys array.
Gah, sorry.
Post by Pekka Paalanen
Since we need a v4 anyway, I'll comment on the unimportant issues below,
too. ;-)
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;
Using 'return eaten' would have more documentary value, no?
Post by Giulio Camuffo
/* 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;
struct wl_array keys;
+ struct wl_array eaten_keys;
struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..7819a41 100644
--- a/src/input.c
+++ b/src/input.c
@@ -1307,6 +1307,26 @@ update_keymap(struct weston_seat *seat)
}
#endif
+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1335,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,28 +1347,26 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
And also pressed rather than released.
Uh? there are no released keys in the array. Or am i misunderstanding
what you mean?
Post by Pekka Paalanen
Post by Giulio Camuffo
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;
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);
Could use realignment.
Post by Giulio Camuffo
grab = keyboard->grab;
}
+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);
if (keyboard->pending_keymap &&
@@ -1430,6 +1450,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }
seat->modifier_state = 0;
Thanks,
pq
Pekka Paalanen
2014-11-11 09:35:35 UTC
Permalink
On Tue, 11 Nov 2014 10:00:15 +0200
Post by Giulio Camuffo
Post by Pekka Paalanen
On Mon, 10 Nov 2014 22:08:40 +0200
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.
---
v3: call weston_compositor_idle_inhibit() for all the keys pressed
src/bindings.c | 7 +++++--
src/compositor.h | 3 ++-
src/input.c | 57 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 48 insertions(+), 19 deletions(-)
Hi Giulio,
I would have committed this as is, but you forgot to add array init and
release for the new eaten_keys array.
Gah, sorry.
Post by Pekka Paalanen
Since we need a v4 anyway, I'll comment on the unimportant issues below,
too. ;-)
...
Post by Giulio Camuffo
Post by Pekka Paalanen
Post by Giulio Camuffo
+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1335,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,28 +1347,26 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
And also pressed rather than released.
Uh? there are no released keys in the array. Or am i misunderstanding
what you mean?
It returns -1 only if a) the key is in the array, and b) state=PRESSED.

If state is not pressed, then it merely guarantees the key will no
longer be in the array, and always returns 0 even if the key was in
the array.

Hair-splitting, I know, nevermind. :-)
Post by Giulio Camuffo
Post by Pekka Paalanen
Post by Giulio Camuffo
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;
Thanks,
pq
Bill Spitzak
2014-11-11 19:27:47 UTC
Permalink
Hate to be a pain about this, but the description of the bug (with
Mod+x) can be reproduced and has nothing to do with whether the
compositor shortcut was triggered. Try Mod+y (or any other key not
consumed by the compositor) in an xterm.

You can argue about whether the 'x' or 'y' should repeat after the Mod
key was released, but there is absolutely no reason the result should
differ depending on whether the shortcut went to the server or the client.

I therefore still believe that this proposed fix is a serious blunder
and should not be done. The correct fix is for clients to get an
accurate key-down map whenever they have keyboard focus. This proposed
fix instead breaks it in the case that it works (when the client gets
focus) instead of the correct fix of doing something when the shortcut
does not cause a focus change.

If clients respond in unexpected ways when keys change state than this
is a bug in the client and needs to be fixed there.
Giulio Camuffo
2014-11-11 19:40:08 UTC
Permalink
I'm unsure whether i should reply, but let's try one last time.
Hate to be a pain about this, but the description of the bug (with Mod+x)
can be reproduced and has nothing to do with whether the compositor shortcut
was triggered. Try Mod+y (or any other key not consumed by the compositor)
in an xterm.
It works as expected, what's the problem?
You can argue about whether the 'x' or 'y' should repeat after the Mod key
was released, but there is absolutely no reason the result should differ
depending on whether the shortcut went to the server or the client.
I therefore still believe that this proposed fix is a serious blunder and
should not be done. The correct fix is for clients to get an accurate
Too late, it's in already. It appears evident you're the only one who
thinks that compositor's bindings should not eat the key events, so
your only chance of convincing anyone is to bring real usecases, not
abstract ideas.


--
Giulio
key-down map whenever they have keyboard focus. This proposed fix instead
breaks it in the case that it works (when the client gets focus) instead of
the correct fix of doing something when the shortcut does not cause a focus
change.
If clients respond in unexpected ways when keys change state than this is a
bug in the client and needs to be fixed there.
Bill Spitzak
2014-11-11 20:39:10 UTC
Permalink
Better example:

- Make an xterm have focus.

- Make a new wayland terminal and give it focus.

- In the Wayland terminal, press Ctrl, then D. The wayland terminal
exits due to the shell getting EOF and quitting. The xterm gets focus.

- Release Ctrl but continue holding down 'd'. The 'd' starts to repeat
in the xterm.

The bug has nothing to do with whether a shortcut is a "server" shortcut
or not, and can be easily demonstrated with a "client shortcut".

You are making a mistake. Sorry.
Giulio Camuffo
2014-11-11 20:48:04 UTC
Permalink
Post by Bill Spitzak
- Make an xterm have focus.
- Make a new wayland terminal and give it focus.
- In the Wayland terminal, press Ctrl, then D. The wayland terminal exits
due to the shell getting EOF and quitting. The xterm gets focus.
- Release Ctrl but continue holding down 'd'. The 'd' starts to repeat in
the xterm.
The bug has nothing to do with whether a shortcut is a "server" shortcut or
not, and can be easily demonstrated with a "client shortcut".
Very well, so xwayland seems to have bug, since this doesn't happen
with two weston-terminals. However, this has little to do with this
patch, but rather with xwayland apparently ignoring the modifiers
event it gets on focus-in.
Post by Bill Spitzak
You are making a mistake. Sorry.
Pekka Paalanen
2014-11-12 11:15:10 UTC
Permalink
On Tue, 11 Nov 2014 22:48:04 +0200
Post by Giulio Camuffo
Post by Bill Spitzak
- Make an xterm have focus.
- Make a new wayland terminal and give it focus.
- In the Wayland terminal, press Ctrl, then D. The wayland terminal exits
due to the shell getting EOF and quitting. The xterm gets focus.
- Release Ctrl but continue holding down 'd'. The 'd' starts to repeat in
the xterm.
I don't get this behaviour. The xterm gets Ctrl+d, interprets it as
EOF, and causes it eventually exit.
Post by Giulio Camuffo
Post by Bill Spitzak
The bug has nothing to do with whether a shortcut is a "server" shortcut or
not, and can be easily demonstrated with a "client shortcut".
Very well, so xwayland seems to have bug, since this doesn't happen
with two weston-terminals. However, this has little to do with this
patch, but rather with xwayland apparently ignoring the modifiers
event it gets on focus-in.
Furthermore, lets do a test completely without Wayland at all, in X.

I am in a normal X session, nothing Wayland. I open an xterm. In that
xterm:

1. press down Ctrl
2. press down 'k'

Nothing happens.

3. release Ctrl

'k' starts to repeat in xterm!

So, it looks like X already has the behaviour you say is incorrect.

Xwayland is not meant to change the behaviour of X programs if
possible, and in this particular case Xwayland manages to reproduce the
exact same behaviour as native X.

If I do the same test with terminals and Ctrl+d in X, it will exit all
terminals one by one where the focus shifts to.


Thanks,
pq
Bill Spitzak
2014-11-12 20:06:16 UTC
Permalink
Post by Pekka Paalanen
Post by Bill Spitzak
- In the Wayland terminal, press Ctrl, then D. The wayland terminal exits
due to the shell getting EOF and quitting. The xterm gets focus.
- Release Ctrl but continue holding down 'd'. The 'd' starts to repeat in
the xterm.
I don't get this behaviour. The xterm gets Ctrl+d, interprets it as
EOF, and causes it eventually exit.
That does not happen to me. My xterm seems to ignore the fact that 'd'
is held down until Ctrl is released, at which point it starts producing
the 'd'.

However it does not matter.

The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in
the key-down array attached to the focus-in event!!!!

Apparently if Ctrl+D was a "compositor shortcut" to close a window, then
this patch will cause the 'd' to not be there. But since Ctrl+D is
handled by a client to close the window, the client with the new focus
*does* see it. WHY IS THIS DIFFERENT??????

IMHO they should not be different, therefore this patch is wrong. There
are two possible solutions:

1. Revert it. The actual bug is that a client that already has focus
before a "compositor shortcut" will not get any changes to the keymap.
Add something to fix this instead. I recommend a redundant focus-in event.

2. Always send a blank key-down array in the focus-in event, since it is
possible that any key held down was the "shortcut" that caused the
focus-in event.
Pekka Paalanen
2014-11-13 07:35:28 UTC
Permalink
On Wed, 12 Nov 2014 12:06:16 -0800
Post by Bill Spitzak
The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in
the key-down array attached to the focus-in event!!!!
Correct. Apparently X apps interpret a "key already down" the same way
as a "key goes down" event. That is not a Wayland issue at all.
Post by Bill Spitzak
Apparently if Ctrl+D was a "compositor shortcut" to close a window, then
this patch will cause the 'd' to not be there. But since Ctrl+D is
handled by a client to close the window, the client with the new focus
*does* see it. WHY IS THIS DIFFERENT??????
Because when you hit a compositor hotkey, that final key-press in the
combination must not be sent to any client at all. Otherwise there
would be two consumers of the same event: the compositor and a client.
This would lead to duplicate action, which is not wanted for obvious
reasons.

If a key combination is not a compositor hotkey, also the final
key-press event is directed to exactly the one client with the keyboard
focus.
Post by Bill Spitzak
IMHO they should not be different, therefore this patch is wrong. There
1. Revert it. The actual bug is that a client that already has focus
before a "compositor shortcut" will not get any changes to the keymap.
Add something to fix this instead. I recommend a redundant focus-in event.
2. Always send a blank key-down array in the focus-in event, since it is
possible that any key held down was the "shortcut" that caused the
focus-in event.
No, we are not going to break the Wayland core protocol. Also, I do not
see a problem with the current behaviour.

We are not going to screw up Wayland just so that X apps in Xwayland
would work "better" for some cornercases.

Instead, we fixed Weston to act consistently, which corrected the
original problem.


Thanks,
pq
Daniel Stone
2014-11-13 09:20:53 UTC
Permalink
Hi,
Per previous email ...
Post by Pekka Paalanen
On Wed, 12 Nov 2014 12:06:16 -0800
Post by Bill Spitzak
The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in
the key-down array attached to the focus-in event!!!!
Correct. Apparently X apps interpret a "key already down" the same way
as a "key goes down" event. That is not a Wayland issue at all.
No, X _apps_ don't - XWayland does.
Post by Pekka Paalanen
Post by Bill Spitzak
Apparently if Ctrl+D was a "compositor shortcut" to close a window, then
this patch will cause the 'd' to not be there. But since Ctrl+D is
handled by a client to close the window, the client with the new focus
*does* see it. WHY IS THIS DIFFERENT??????
Because when you hit a compositor hotkey, that final key-press in the
combination must not be sent to any client at all. Otherwise there
would be two consumers of the same event: the compositor and a client.
This would lead to duplicate action, which is not wanted for obvious
reasons.
If a key combination is not a compositor hotkey, also the final
key-press event is directed to exactly the one client with the keyboard
focus.
Agreed. This is why we need to be clear about the differences between key
and enter. On key, you update state and run any actions. On enter, you just
update your state and don't do anything which would have a direct effect,
since the key has already been consumed by someone else.

I really need to update the documentation to be clear here. :)
Post by Pekka Paalanen
Post by Bill Spitzak
IMHO they should not be different, therefore this patch is wrong. There
1. Revert it. The actual bug is that a client that already has focus
before a "compositor shortcut" will not get any changes to the keymap.
Add something to fix this instead. I recommend a redundant focus-in
event.
Post by Bill Spitzak
2. Always send a blank key-down array in the focus-in event, since it is
possible that any key held down was the "shortcut" that caused the
focus-in event.
No, we are not going to break the Wayland core protocol. Also, I do not
see a problem with the current behaviour.
We are not going to screw up Wayland just so that X apps in Xwayland
would work "better" for some cornercases.
Instead, we fixed Weston to act consistently, which corrected the
original problem.
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.

Again, sorry for letting this sit for way too long.

Cheers,
Daniel
Daniel Stone
2014-11-13 09:53:56 UTC
Permalink
Hi,
Post by Daniel Stone
Post by Pekka Paalanen
On Wed, 12 Nov 2014 12:06:16 -0800
Post by Bill Spitzak
The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in
the key-down array attached to the focus-in event!!!!
Correct. Apparently X apps interpret a "key already down" the same way
as a "key goes down" event. That is not a Wayland issue at all.
No, X _apps_ don't - XWayland does.
To be clear: X11 has this as well. FocusIn + KeymapNotify events are
equivalent to wl_keyboard::enter with a fully-populated key list.
Post by Daniel Stone
Post by Pekka Paalanen
IMHO they should not be different, therefore this patch is wrong. There
Post by Bill Spitzak
1. Revert it. The actual bug is that a client that already has focus
before a "compositor shortcut" will not get any changes to the keymap.
Add something to fix this instead. I recommend a redundant focus-in
event.
Post by Bill Spitzak
2. Always send a blank key-down array in the focus-in event, since it is
possible that any key held down was the "shortcut" that caused the
focus-in event.
No, we are not going to break the Wayland core protocol. Also, I do not
see a problem with the current behaviour.
We are not going to screw up Wayland just so that X apps in Xwayland
would work "better" for some cornercases.
Instead, we fixed Weston to act consistently, which corrected the
original problem.
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.
And the consistency argument is backed up by different consumers.
wl_keyboard::enter gives you the list of currently-pressed keys, to be used
for bookkeeping and _not_ for actions.

If we enter from a VT switch, we'll send Ctrl+Alt+Fx to the clients in
the enter event when they get the focus. If it was a compositor key
binding, with this patch, we won't send the key that immediately triggered
the binding, but we will send the other keys.

I'm struggling to see a case where this makes sense, especially if you take
it as a given that the current XWayland behaviour (which this patch tries
to work around) as broken and in need of fixing.

wl_keyboard::enter sends the list of currently-depressed keys to the
client. If we make it a partial list, depending on context, we make it
useless. We'll send releases for keys the client never knew were down -
which is _exactly_ the problem this event is trying to prevent.

If we're making it an arbitrary list depending on whether it was a
compositor hotkey, or depending on how you're nested, the event becomes
useless as a guarantee and we might as well just drop it completely.

Cheers,
Daniel
Bill Spitzak
2014-11-13 20:51:16 UTC
Permalink
Post by Daniel Stone
wl_keyboard::enter sends the list of currently-depressed keys to the
client. If we make it a partial list, depending on context, we make it
useless. We'll send releases for keys the client never knew were down -
which is _exactly_ the problem this event is trying to prevent.
If we're making it an arbitrary list depending on whether it was a
compositor hotkey, or depending on how you're nested, the event becomes
useless as a guarantee and we might as well just drop it completely.
Cheers,
Daniel
Thank you for stepping in on this. This is what I have been trying to
say for quite a while, though I admit I would use the wrong terms a lot
and may have confused things. I believe you are clearly stating what my
objection to this patch is.
Pekka Paalanen
2014-11-19 11:40:28 UTC
Permalink
On Thu, 13 Nov 2014 09:53:56 +0000
Post by Daniel Stone
Hi,
Post by Daniel Stone
Post by Pekka Paalanen
On Wed, 12 Nov 2014 12:06:16 -0800
Post by Bill Spitzak
The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in
the key-down array attached to the focus-in event!!!!
Correct. Apparently X apps interpret a "key already down" the same way
as a "key goes down" event. That is not a Wayland issue at all.
No, X _apps_ don't - XWayland does.
To be clear: X11 has this as well. FocusIn + KeymapNotify events are
equivalent to wl_keyboard::enter with a fully-populated key list.
Ah, I didn't know that. Since there was a problem here, I assumed
X11 as a protocol has no way to differentiate between that and key.

It's all starting to make sense now. I think.


Thanks,
pq
Daniel Stone
2014-11-13 09:18:36 UTC
Permalink
Hi,
Sorry for the long absence from this thread.
Post by Pekka Paalanen
I don't get this behaviour. The xterm gets Ctrl+d, interprets it as
EOF, and causes it eventually exit.
That does not happen to me. My xterm seems to ignore the fact that 'd' is
held down until Ctrl is released, at which point it starts producing the
'd'.
However it does not matter.
The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in the
key-down array attached to the focus-in event!!!!
Apparently if Ctrl+D was a "compositor shortcut" to close a window, then
this patch will cause the 'd' to not be there. But since Ctrl+D is handled
by a client to close the window, the client with the new focus *does* see
it. WHY IS THIS DIFFERENT??????
In a fairly shock twist of events, I actually agree with Bill here. Part of
the problem is wl_keyboard's, er, rather sparse documentation, for which my
many apologies. It's sitting on my TODO for after I clear out my patch
review backlog.

wl_keyboard::key is fairly obvious: a key was pressed, it was directed at
you, so please take any action required by this (printing keys, running
shortcuts). So, any key event received here will update the internal state,
and trigger actions.

wl_keyboard::enter, OTOH, is strictly advisory: you've got the focus now,
oh and by the way here's the state of the keyboard which changed whilst you
were away. Any key event received here will update internal state, but
_not_ trigger actions.

Think about the case where pressing Alt on its own will focus the system
menu, but pressing Alt-F will raise the file menu directly. Let's say the
compositor has an Alt-F12 hotkey which triggers some action and then
immediately returns focus. We'll come back to the client with Alt held
down, and nothing else. When we come back, we do _not_ want the client to
focus the menu as if Alt was pressed normally. However, we _do_ want the
client to raise the file menu if F is then pressed (so, Alt+F). This
exactly matches the previous paragraph: update your internal state, but do
not immediately trigger any actions.

This also matches exactly the new semantics of notify_keyboard_focus_in(),
in Pekka's last mail above. Why should the compositor be different from any
other client, especially when the compositor _is_ a client itself?

If you think about it, the notify_keyboard_focus_in() semantics are the
only ones which makes sense. Say you trigger Exposay or something on a raw
Ctrl+Alt press, and switch VTs on Ctrl+Alt+Fx. The user presses Ctrl+Alt+F2
to focus the compositor on VT 2, and that binding is triggered on press.
We'll land on that compositor, which will see Ctrl+Alt+F2 down on its focus
event. Running keybindings would make it switch VTs over to itself in a
tight loop, so obviously we don't want to do that. But if we release F2 and
press F3, we want the Ctrl+Alt+F3 binding (switch to VT 3) to fire: so
again, we're using the focus events to update state, nothing else.

Looking at what we have now:
- notify_keyboard_focus_in runs bindings - as above, this is a bug
- compositor-drm does the right thing: in evdev_notify_keyboard_focus, it
gets the current state of keys down from evdev, and calls
notify_keyboard_focus_in, and not notify_key - this is the right thing to
do, modulo notify_keyboard_focus_in running bindings
- compositor-x11 also does the right thing: in the
XCB_FOCUS_IN/XCB_KEYMAP_NOTIFY handler, it pulls the list of
currently-pressed keys, and calls notify_keyboard_focus_in with them - same
caveat about notifykeyboard_focus_in running bindings
- compositor-wayland does the right thing: in the keyboard_enter handler,
it calls notify_keyboard_focus_in with the list of keys down - same caveat
- XWayland does the wrong thing: in the keyboard_enter handler, it queues
KeyPress events for every key listed as down

We can see exactly one outlier here, doing the exactly wrong thing, which
is XWayland. So it stands to reason that XWayland should be fixed: changing
the others would introduce massive inconsistencies depending on whether
your compositor is native or nested, which makes ~no sense.

The problem with fixing XWayland is that we don't have a particularly good
mechanism for these kinds of focus notifications, since it's really just
not designed to be nested. I've attached a couple of putative patches which
would do the right thing in XWayland; unfortunately, XWayland isn't playing
nice for me at the moment, so these are wildly untested.
IMHO they should not be different, therefore this patch is wrong. There
1. Revert it. The actual bug is that a client that already has focus
before a "compositor shortcut" will not get any changes to the keymap. Add
something to fix this instead. I recommend a redundant focus-in event.
2. Always send a blank key-down array in the focus-in event, since it is
possible that any key held down was the "shortcut" that caused the focus-in
event.
I'd like to see this reverted and XWayland fixed.

Sorry for the drive-by IRC review that may have gave the impression I
actually supported this overall change, rather than just commented on one
isolated question about whether or not focus_in should run bindings. I've
been meaning to get to this thread for a little while, but sometimes these
threads make me question my will to live.

Cheers,
Daniel
Giulio Camuffo
2014-11-13 09:43:44 UTC
Permalink
Post by Daniel Stone
Hi,
Sorry for the long absence from this thread.
Post by Pekka Paalanen
I don't get this behaviour. The xterm gets Ctrl+d, interprets it as
EOF, and causes it eventually exit.
That does not happen to me. My xterm seems to ignore the fact that 'd' is
held down until Ctrl is released, at which point it starts producing the
'd'.
However it does not matter.
The important point is that in both your and my case the client that has
just received focus sees the 'd' as being held down. The 'd' is on in the
key-down array attached to the focus-in event!!!!
Apparently if Ctrl+D was a "compositor shortcut" to close a window, then
this patch will cause the 'd' to not be there. But since Ctrl+D is handled
by a client to close the window, the client with the new focus *does* see
it. WHY IS THIS DIFFERENT??????
In a fairly shock twist of events, I actually agree with Bill here. Part of
the problem is wl_keyboard's, er, rather sparse documentation, for which my
many apologies. It's sitting on my TODO for after I clear out my patch
review backlog.
wl_keyboard::key is fairly obvious: a key was pressed, it was directed at
you, so please take any action required by this (printing keys, running
shortcuts). So, any key event received here will update the internal state,
and trigger actions.
wl_keyboard::enter, OTOH, is strictly advisory: you've got the focus now, oh
and by the way here's the state of the keyboard which changed whilst you
were away. Any key event received here will update internal state, but _not_
trigger actions.
Think about the case where pressing Alt on its own will focus the system
menu, but pressing Alt-F will raise the file menu directly. Let's say the
compositor has an Alt-F12 hotkey which triggers some action and then
immediately returns focus. We'll come back to the client with Alt held down,
and nothing else. When we come back, we do _not_ want the client to focus
the menu as if Alt was pressed normally. However, we _do_ want the client to
raise the file menu if F is then pressed (so, Alt+F). This exactly matches
the previous paragraph: update your internal state, but do not immediately
trigger any actions.
But this should not be affected by this patch. The key binding just
eats the F12, so the Alt is still being sent to the client.
Post by Daniel Stone
This also matches exactly the new semantics of notify_keyboard_focus_in(),
in Pekka's last mail above. Why should the compositor be different from any
other client, especially when the compositor _is_ a client itself?
If you think about it, the notify_keyboard_focus_in() semantics are the only
ones which makes sense. Say you trigger Exposay or something on a raw
Ctrl+Alt press, and switch VTs on Ctrl+Alt+Fx. The user presses Ctrl+Alt+F2
to focus the compositor on VT 2, and that binding is triggered on press.
We'll land on that compositor, which will see Ctrl+Alt+F2 down on its focus
event. Running keybindings would make it switch VTs over to itself in a
tight loop, so obviously we don't want to do that. But if we release F2 and
press F3, we want the Ctrl+Alt+F3 binding (switch to VT 3) to fire: so
again, we're using the focus events to update state, nothing else.
- notify_keyboard_focus_in runs bindings - as above, this is a bug
- compositor-drm does the right thing: in evdev_notify_keyboard_focus, it
gets the current state of keys down from evdev, and calls
notify_keyboard_focus_in, and not notify_key - this is the right thing to
do, modulo notify_keyboard_focus_in running bindings
- compositor-x11 also does the right thing: in the
XCB_FOCUS_IN/XCB_KEYMAP_NOTIFY handler, it pulls the list of
currently-pressed keys, and calls notify_keyboard_focus_in with them - same
caveat about notifykeyboard_focus_in running bindings
- compositor-wayland does the right thing: in the keyboard_enter handler,
it calls notify_keyboard_focus_in with the list of keys down - same caveat
- XWayland does the wrong thing: in the keyboard_enter handler, it queues
KeyPress events for every key listed as down
We can see exactly one outlier here, doing the exactly wrong thing, which is
XWayland. So it stands to reason that XWayland should be fixed: changing the
others would introduce massive inconsistencies depending on whether your
compositor is native or nested, which makes ~no sense.
I perfectly agree XWayland is broken here. But i don't see how this
patch breaks weston.
Post by Daniel Stone
The problem with fixing XWayland is that we don't have a particularly good
mechanism for these kinds of focus notifications, since it's really just not
designed to be nested. I've attached a couple of putative patches which
would do the right thing in XWayland; unfortunately, XWayland isn't playing
nice for me at the moment, so these are wildly untested.
IMHO they should not be different, therefore this patch is wrong. There
1. Revert it. The actual bug is that a client that already has focus
before a "compositor shortcut" will not get any changes to the keymap. Add
something to fix this instead. I recommend a redundant focus-in event.
2. Always send a blank key-down array in the focus-in event, since it is
possible that any key held down was the "shortcut" that caused the focus-in
event.
I'd like to see this reverted and XWayland fixed.
Sorry for the drive-by IRC review that may have gave the impression I
actually supported this overall change, rather than just commented on one
isolated question about whether or not focus_in should run bindings. I've
been meaning to get to this thread for a little while, but sometimes these
threads make me question my will to live.
Cheers,
Daniel
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.
But consider this case: There is a Alt+X compositor binding which
triggers something, without changing the focus. Then the client has a
Alt+X+Y binding. When pressing Alt+X the X is eaten by the compositor,
so trying to press also Y to trigger the client binding will have no
effect, because the client will actually see just Alt+Y. Without this
patch, if the Alt+X compositor binding was to instead switch the focus
to the client, the Alt+X+Y binding would work, because the client sees
the X too. Now it doesn't, as it doesn't without switching the focus.
This is where it is more consistent.


--
Giulio
Daniel Stone
2014-11-13 10:06:27 UTC
Permalink
Hi,
Post by Bill Spitzak
Post by Daniel Stone
Think about the case where pressing Alt on its own will focus the system
menu, but pressing Alt-F will raise the file menu directly. Let's say the
compositor has an Alt-F12 hotkey which triggers some action and then
immediately returns focus. We'll come back to the client with Alt held
down,
Post by Daniel Stone
and nothing else. When we come back, we do _not_ want the client to focus
the menu as if Alt was pressed normally. However, we _do_ want the
client to
Post by Daniel Stone
raise the file menu if F is then pressed (so, Alt+F). This exactly
matches
Post by Daniel Stone
the previous paragraph: update your internal state, but do not
immediately
Post by Daniel Stone
trigger any actions.
But this should not be affected by this patch. The key binding just
eats the F12, so the Alt is still being sent to the client.
So we've introduced an inconsistency. It happens to mostly kind of work for
modifiers, but that's luck of the draw.
Post by Bill Spitzak
We can see exactly one outlier here, doing the exactly wrong thing, which is
Post by Daniel Stone
XWayland. So it stands to reason that XWayland should be fixed: changing
the
Post by Daniel Stone
others would introduce massive inconsistencies depending on whether your
compositor is native or nested, which makes ~no sense.
I perfectly agree XWayland is broken here. But i don't see how this
patch breaks weston.
By introducing an inconsistency for, as far as I can see, no reason. That
it mostly happens to work is great, but that's luck rather than design.
Post by Bill Spitzak
Post by Daniel Stone
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.
But consider this case: There is a Alt+X compositor binding which
triggers something, without changing the focus. Then the client has a
Alt+X+Y binding. When pressing Alt+X the X is eaten by the compositor,
so trying to press also Y to trigger the client binding will have no
effect, because the client will actually see just Alt+Y. Without this
patch, if the Alt+X compositor binding was to instead switch the focus
to the client, the Alt+X+Y binding would work, because the client sees
the X too. Now it doesn't, as it doesn't without switching the focus.
This is where it is more consistent.
But this is just a client issue, and nothing in sending the full keys array
precludes this working.

If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers
the shortcut), then the client can use the keys array to notice this, and
ensure the shortcut is fired.

If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on
X before it arms the shortcut for Y.

In both cases, no problem. If we suppress X in the key-down array, we break
the first case, and in the second case, we send the client a release for a
key it's never seen. IOW, we introduce enormous inconsistencies in the key
book-keeping. As there's ~nothing more infuriating than broken key/button
state handling, and it's already relatively complicated code, I don't see
how any good can come out of making it easier to screw up for pretty much
no gain.

Given the above, and the possibility of making XWayland do the right thing,
I just can't see how this patch improves the status quo (pre-this-patch),
given the enormous downsides.

Cheers,
Daniel
Giulio Camuffo
2014-11-13 10:33:14 UTC
Permalink
Post by Daniel Stone
Hi,
Post by Giulio Camuffo
Post by Daniel Stone
Think about the case where pressing Alt on its own will focus the system
menu, but pressing Alt-F will raise the file menu directly. Let's say the
compositor has an Alt-F12 hotkey which triggers some action and then
immediately returns focus. We'll come back to the client with Alt held down,
and nothing else. When we come back, we do _not_ want the client to focus
the menu as if Alt was pressed normally. However, we _do_ want the client to
raise the file menu if F is then pressed (so, Alt+F). This exactly matches
the previous paragraph: update your internal state, but do not immediately
trigger any actions.
But this should not be affected by this patch. The key binding just
eats the F12, so the Alt is still being sent to the client.
So we've introduced an inconsistency. It happens to mostly kind of work for
modifiers, but that's luck of the draw.
Post by Giulio Camuffo
Post by Daniel Stone
We can see exactly one outlier here, doing the exactly wrong thing, which is
XWayland. So it stands to reason that XWayland should be fixed: changing the
others would introduce massive inconsistencies depending on whether your
compositor is native or nested, which makes ~no sense.
I perfectly agree XWayland is broken here. But i don't see how this
patch breaks weston.
By introducing an inconsistency for, as far as I can see, no reason. That it
mostly happens to work is great, but that's luck rather than design.
Post by Giulio Camuffo
Post by Daniel Stone
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.
But consider this case: There is a Alt+X compositor binding which
triggers something, without changing the focus. Then the client has a
Alt+X+Y binding. When pressing Alt+X the X is eaten by the compositor,
so trying to press also Y to trigger the client binding will have no
effect, because the client will actually see just Alt+Y. Without this
patch, if the Alt+X compositor binding was to instead switch the focus
to the client, the Alt+X+Y binding would work, because the client sees
the X too. Now it doesn't, as it doesn't without switching the focus.
This is where it is more consistent.
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers the
shortcut), then the client can use the keys array to notice this, and ensure
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on X
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
Post by Daniel Stone
In both cases, no problem. If we suppress X in the key-down array, we break
the first case, and in the second case, we send the client a release for a
key it's never seen. IOW, we introduce enormous inconsistencies in the key
Ok, i can see this may be a problem.
Post by Daniel Stone
book-keeping. As there's ~nothing more infuriating than broken key/button
state handling, and it's already relatively complicated code, I don't see
how any good can come out of making it easier to screw up for pretty much no
gain.
Given the above, and the possibility of making XWayland do the right thing,
I just can't see how this patch improves the status quo (pre-this-patch),
given the enormous downsides.
Cheers,
Daniel
Daniel Stone
2014-11-13 11:30:26 UTC
Permalink
Hi,
Post by Daniel Stone
Post by Daniel Stone
But this is just a client issue, and nothing in sending the full keys
array
Post by Daniel Stone
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers
the
Post by Daniel Stone
shortcut), then the client can use the keys array to notice this, and
ensure
Post by Daniel Stone
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing
X
Post by Daniel Stone
in the enter array but not a key event, that it must wait for a release
on X
Post by Daniel Stone
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
A problem we can solve by switching the focus. ;) I agree that it's
annoying to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's about
to get another enter event very shortly, but shouldn't redraw itself
insensitive or anything.

Alternately, we could bump the wl_keyboard version to just allow for
consecutive enter events and never send a leave in these temporal cases.

For older clients, we can just send leave/enter and they can suck it up.

Either way, it makes things more predictable and allows clients to work out
what's best.

Cheers,
Daniel
Jasper St. Pierre
2014-11-13 17:30:57 UTC
Permalink
Post by Daniel Stone
Hi,
Post by Daniel Stone
Post by Daniel Stone
But this is just a client issue, and nothing in sending the full keys
array
Post by Daniel Stone
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y
triggers the
Post by Daniel Stone
shortcut), then the client can use the keys array to notice this, and
ensure
Post by Daniel Stone
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from
seeing X
Post by Daniel Stone
in the enter array but not a key event, that it must wait for a release
on X
Post by Daniel Stone
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
A problem we can solve by switching the focus. ;) I agree that it's
annoying to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's about
to get another enter event very shortly, but shouldn't redraw itself
insensitive or anything.
wl_keyboard::leave should never affect the the window visibly. That's what
the state in xdg-shell is for.
Post by Daniel Stone
Alternately, we could bump the wl_keyboard version to just allow for
consecutive enter events and never send a leave in these temporal cases.
For older clients, we can just send leave/enter and they can suck it up.
Either way, it makes things more predictable and allows clients to work
out what's best.
Cheers,
Daniel
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
--
Jasper
Bill Spitzak
2014-11-13 21:08:03 UTC
Permalink
Post by Giulio Camuffo
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
I agree it is not consistent, but the proposed patch fixes the
consistency "backwards". It must be fixed instead by changing the
behavior when the focus does not change. The behavior when the focus
does change is correct.
Post by Giulio Camuffo
A problem we can solve by switching the focus. ;) I agree that it's
annoying to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's
about to get another enter event very shortly, but shouldn't redraw
itself insensitive or anything.
I don't think it has to be anywhere near as complicated. Instead the
compositor sends an extra focus-in event after handling the shortcut.

In the Alt+X compositor shortcut example:

When Alt is pushed the compositor records this in it's own keymap, but
also sends the event to the focus client, which records it too.

When X is pushed, the compositor updates it's map with the X down and
realizes that it got it's shortcut. It then handles it and does *not*
send the key-down event to the client. Instead it sends an extra
focus-in event to the client, with it's current keymap. The client then
updates it's keymap and knows that Alt and X are held down.

If Alt or X are released, the key-up events are sent to the client so it
can remove them from it's key map.
Giulio Camuffo
2014-11-18 16:54:04 UTC
Permalink
Hi,
Post by Giulio Camuffo
Post by Daniel Stone
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers the
shortcut), then the client can use the keys array to notice this, and ensure
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on X
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
A problem we can solve by switching the focus. ;) I agree that it's annoying
to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's about
to get another enter event very shortly, but shouldn't redraw itself
insensitive or anything.
Alternately, we could bump the wl_keyboard version to just allow for
consecutive enter events and never send a leave in these temporal cases.
Do we really need to bump the version? The description of
wl_keyboard.leave says " Notification that this seat's keyboard focus
is on a certain surface.", it doesn't say the leave and enter events
always go in pair, so it seems to me we can just send the new enters
without changing anything.


--
Giulio
For older clients, we can just send leave/enter and they can suck it up.
Either way, it makes things more predictable and allows clients to work out
what's best.
Cheers,
Daniel
Pekka Paalanen
2014-11-19 11:42:11 UTC
Permalink
On Tue, 18 Nov 2014 18:54:04 +0200
Post by Giulio Camuffo
Hi,
Post by Giulio Camuffo
Post by Daniel Stone
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers the
shortcut), then the client can use the keys array to notice this, and ensure
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on X
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
A problem we can solve by switching the focus. ;) I agree that it's annoying
to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's about
to get another enter event very shortly, but shouldn't redraw itself
insensitive or anything.
Alternately, we could bump the wl_keyboard version to just allow for
consecutive enter events and never send a leave in these temporal cases.
Do we really need to bump the version? The description of
wl_keyboard.leave says " Notification that this seat's keyboard focus
is on a certain surface.", it doesn't say the leave and enter events
always go in pair, so it seems to me we can just send the new enters
without changing anything.
We have lots of things we simply imply instead of having it written
down. But I also think we don't even need to consider this.

I don't see problem having the compositor itself steal the keyboard
focus when a hotkey goes down. It would cause a wl_keyboard.leave to
the client, and the keyboard focus would stay in the compositor as long
as the grab is active (hotkeys always install a grab to prevent key
events going to clients). When the grab ends, keyboard focus
can return, and we naturally get the wl_keyboard.enter with the list of
keys. I think this even allows both the compositor first handling the
hotkey-down, and then the app seeing the key already-down.

We even have one hotkey that won't release the grab on key-release: the
debug key. Instead, it will wait for another key to be pressed,
indefinitely IIRC.

Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.

I would favor the proper keyboard focus transitions I described above,
rather than changing the protocol behaviour. It's easier and can be
done in compositor only, without fixing all toolkits.

That would be half of the fix, the other half being in Xwayland, right?


Thanks,
pq
Giulio Camuffo
2014-11-19 12:31:35 UTC
Permalink
Post by Pekka Paalanen
On Tue, 18 Nov 2014 18:54:04 +0200
Post by Giulio Camuffo
Hi,
Post by Giulio Camuffo
Post by Daniel Stone
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers the
shortcut), then the client can use the keys array to notice this, and ensure
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on X
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
A problem we can solve by switching the focus. ;) I agree that it's annoying
to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's about
to get another enter event very shortly, but shouldn't redraw itself
insensitive or anything.
Alternately, we could bump the wl_keyboard version to just allow for
consecutive enter events and never send a leave in these temporal cases.
Do we really need to bump the version? The description of
wl_keyboard.leave says " Notification that this seat's keyboard focus
is on a certain surface.", it doesn't say the leave and enter events
always go in pair, so it seems to me we can just send the new enters
without changing anything.
We have lots of things we simply imply instead of having it written
down. But I also think we don't even need to consider this.
I don't see problem having the compositor itself steal the keyboard
focus when a hotkey goes down. It would cause a wl_keyboard.leave to
the client, and the keyboard focus would stay in the compositor as long
as the grab is active (hotkeys always install a grab to prevent key
events going to clients). When the grab ends, keyboard focus
can return, and we naturally get the wl_keyboard.enter with the list of
keys. I think this even allows both the compositor first handling the
hotkey-down, and then the app seeing the key already-down.
We even have one hotkey that won't release the grab on key-release: the
debug key. Instead, it will wait for another key to be pressed,
indefinitely IIRC.
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
The only problem with that is that apps using wl_shell can only use
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Post by Pekka Paalanen
I would favor the proper keyboard focus transitions I described above,
rather than changing the protocol behaviour. It's easier and can be
done in compositor only, without fixing all toolkits.
That would be half of the fix, the other half being in Xwayland, right?
Well, there is also the part about triggering the binding on key
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.


--
Giulio
Post by Pekka Paalanen
Thanks,
pq
Pekka Paalanen
2014-11-19 13:08:14 UTC
Permalink
On Wed, 19 Nov 2014 14:31:35 +0200
Post by Giulio Camuffo
Post by Pekka Paalanen
On Tue, 18 Nov 2014 18:54:04 +0200
Post by Giulio Camuffo
Hi,
Post by Giulio Camuffo
Post by Daniel Stone
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers
the
shortcut), then the client can use the keys array to notice this, and
ensure
the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release
on X
before it arms the shortcut for Y.
But no, because, when the focus isn't switched, there is no enter
event and no keys array. The client has no idea X was pressed, so it
can't possibly trigger the binding.
So without the patch this is not consistent. Depending on whether the
compositor binding switches the focus, the client binding works or it
doesn't work.
A problem we can solve by switching the focus. ;) I agree that it's annoying
to always do this for every hotkey, so we could introduce a new
wl_keyboard::leave_temporary which would inform the client that it's about
to get another enter event very shortly, but shouldn't redraw itself
insensitive or anything.
Alternately, we could bump the wl_keyboard version to just allow for
consecutive enter events and never send a leave in these temporal cases.
Do we really need to bump the version? The description of
wl_keyboard.leave says " Notification that this seat's keyboard focus
is on a certain surface.", it doesn't say the leave and enter events
always go in pair, so it seems to me we can just send the new enters
without changing anything.
We have lots of things we simply imply instead of having it written
down. But I also think we don't even need to consider this.
I don't see problem having the compositor itself steal the keyboard
focus when a hotkey goes down. It would cause a wl_keyboard.leave to
the client, and the keyboard focus would stay in the compositor as long
as the grab is active (hotkeys always install a grab to prevent key
events going to clients). When the grab ends, keyboard focus
can return, and we naturally get the wl_keyboard.enter with the list of
keys. I think this even allows both the compositor first handling the
hotkey-down, and then the app seeing the key already-down.
We even have one hotkey that won't release the grab on key-release: the
debug key. Instead, it will wait for another key to be pressed,
indefinitely IIRC.
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
The only problem with that is that apps using wl_shell can only use
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Yup. I think that's a reasonable thing to do. It's meant to be phased
out even though we can "never" drop it completely. (except mutter
already did, IIRC).

It's not *that* bad. At worst, it's a flicker. At best, might not even
be visible.

And it doesn't work at all if you don't have a physical keyboard to
begin with.
Post by Giulio Camuffo
Post by Pekka Paalanen
I would favor the proper keyboard focus transitions I described above,
rather than changing the protocol behaviour. It's easier and can be
done in compositor only, without fixing all toolkits.
That would be half of the fix, the other half being in Xwayland, right?
Well, there is also the part about triggering the binding on key
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.
That's a thing I didn't quite get yet... oh, wait, maybe I do now.
Yeah. A bit of extra events to send, but shouldn't hurt at all.

Mod down -> send key Mod down
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave

Then either

's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.

or

'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down

Something like this, is it?


Thanks,
pq
Bill Spitzak
2014-11-19 21:05:23 UTC
Permalink
Post by Pekka Paalanen
Post by Giulio Camuffo
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus"
will be set differently than the input device focus? I think you are
talking about "activation" which is DIFFERENT than keyboard focus.

Unless your goal is to make point-to-type impossible?
Post by Pekka Paalanen
Post by Giulio Camuffo
The only problem with that is that apps using wl_shell can only use
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Yup. I think that's a reasonable thing to do. It's meant to be phased
out even though we can "never" drop it completely. (except mutter
already did, IIRC).
It's not *that* bad. At worst, it's a flicker. At best, might not even
be visible.
The *title bar* will flicker. I think that is pretty bad.

I also think you are panicking about a non-existent problem. No client
is going to fail because it got two focus-in events in a row. Just send
two events irregardless of version, and stop adding needless complexity
to weston!

You also mentioned an "enter debug mode" shortcut. That must NOT send a
focus-out event, as it could change the state of the client being debugged!
Post by Pekka Paalanen
Post by Giulio Camuffo
Well, there is also the part about triggering the binding on key
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.
No! Users do NOT want this. Keys take action when you push them.
Otherwise the interface looks slow and drives users crazy.
Post by Pekka Paalanen
Mod down -> send key Mod down
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave
Then either
's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.
or
'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down
I think you meant the kbd.enter would include the Mod *and* the 's' key.

In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is
what users expect and what every toolkit in the world does on every
platform. There is nothing special about "compositor shortcuts" and they
should NEVER act differently than a client would that handles both
shortcuts. Here is correct behavior:

Mod down -> send key Mod down

's' down -> do the shortcut and take a screen shot, then send a
redundant focus-in event that has the Mod and 's' keys down.

'x' down -> send key 'x' down, which may trigger a client action.
Giulio Camuffo
2014-11-19 21:31:21 UTC
Permalink
Post by Pekka Paalanen
Post by Giulio Camuffo
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus" will be
set differently than the input device focus? I think you are talking about
"activation" which is DIFFERENT than keyboard focus.
Unless your goal is to make point-to-type impossible?
Post by Pekka Paalanen
Post by Giulio Camuffo
The only problem with that is that apps using wl_shell can only use
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Yup. I think that's a reasonable thing to do. It's meant to be phased
out even though we can "never" drop it completely. (except mutter
already did, IIRC).
It's not *that* bad. At worst, it's a flicker. At best, might not even
be visible.
The *title bar* will flicker. I think that is pretty bad.
I also think you are panicking about a non-existent problem. No client is
going to fail because it got two focus-in events in a row. Just send two
events irregardless of version, and stop adding needless complexity to
weston!
You also mentioned an "enter debug mode" shortcut. That must NOT send a
focus-out event, as it could change the state of the client being debugged!
Post by Pekka Paalanen
Post by Giulio Camuffo
Well, there is also the part about triggering the binding on key
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.
No! Users do NOT want this. Keys take action when you push them. Otherwise
the interface looks slow and drives users crazy.
Post by Pekka Paalanen
Mod down -> send key Mod down
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave
Then either
's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.
or
'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down
I think you meant the kbd.enter would include the Mod *and* the 's' key.
In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is what
users expect and what every toolkit in the world does on every platform.
There is nothing special about "compositor shortcuts" and they should NEVER
act differently than a client would that handles both shortcuts. Here is
As a user, i'm very happy my KDE session doesn't work that way. System
wide bindings must not have side effects depending on the client being
focused at the moment. That would make them very annoying to use.
Hitting F12 to show my yakuake window must not open chromium's debug
window or any other client thing. I want to open yakuake, i don't care
about what other clients would want to open in my face.
Mod down -> send key Mod down
's' down -> do the shortcut and take a screen shot, then send a redundant
focus-in event that has the Mod and 's' keys down.
'x' down -> send key 'x' down, which may trigger a client action.
Giulio Camuffo
2014-11-19 21:35:37 UTC
Permalink
Post by Giulio Camuffo
Post by Pekka Paalanen
Post by Giulio Camuffo
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus" will be
set differently than the input device focus? I think you are talking about
"activation" which is DIFFERENT than keyboard focus.
Unless your goal is to make point-to-type impossible?
Post by Pekka Paalanen
Post by Giulio Camuffo
The only problem with that is that apps using wl_shell can only use
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Yup. I think that's a reasonable thing to do. It's meant to be phased
out even though we can "never" drop it completely. (except mutter
already did, IIRC).
It's not *that* bad. At worst, it's a flicker. At best, might not even
be visible.
The *title bar* will flicker. I think that is pretty bad.
I also think you are panicking about a non-existent problem. No client is
going to fail because it got two focus-in events in a row. Just send two
events irregardless of version, and stop adding needless complexity to
weston!
You also mentioned an "enter debug mode" shortcut. That must NOT send a
focus-out event, as it could change the state of the client being debugged!
Post by Pekka Paalanen
Post by Giulio Camuffo
Well, there is also the part about triggering the binding on key
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.
No! Users do NOT want this. Keys take action when you push them. Otherwise
the interface looks slow and drives users crazy.
Post by Pekka Paalanen
Mod down -> send key Mod down
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave
Then either
's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.
or
'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down
I think you meant the kbd.enter would include the Mod *and* the 's' key.
In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is what
users expect and what every toolkit in the world does on every platform.
There is nothing special about "compositor shortcuts" and they should NEVER
act differently than a client would that handles both shortcuts. Here is
As a user, i'm very happy my KDE session doesn't work that way. System
wide bindings must not have side effects depending on the client being
focused at the moment. That would make them very annoying to use.
Hitting F12 to show my yakuake window must not open chromium's debug
window or any other client thing. I want to open yakuake, i don't care
about what other clients would want to open in my face.
Sorry, i just realized that i misread your mail. Discard my previous reply.
Post by Giulio Camuffo
Mod down -> send key Mod down
's' down -> do the shortcut and take a screen shot, then send a redundant
focus-in event that has the Mod and 's' keys down.
'x' down -> send key 'x' down, which may trigger a client action.
Daniel Stone
2014-11-20 08:05:58 UTC
Permalink
Hi,
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
Post by Giulio Camuffo
Post by Pekka Paalanen
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus" will
be set differently than the input device focus? I think you are talking
about "activation" which is DIFFERENT than keyboard focus.
Unless your goal is to make point-to-type impossible?
As far as I can tell from what the others are saying - which I'd totally
agree with - is that wl_keyboard's focus would be much more transient than
it is now, reflecting where keyboard events are _actually_ being delivered
_right now_. So if the compositor took a long-lived grab, e.g. for the lock
screen, you'd lose keyboard focus as you do today, as well as losing
xdg_shell focus. But if the compositor just took an ephemeral grab (e.g.
key binding), then while you would lose keyboard focus, you wouldn't lose
xdg_shell focus.

So think of keyboard focus as tightly bound to actual event delivery at
that very particular moment in time, and a hint that keys may be pressed
underneath you, so you could well come back with pressed keys you shouldn't
take action on, whereas shell focus is what you'd use to drive things like
rendering your decorations with (in)active highlights.
The only problem with that is that apps using wl_shell can only use
Post by Pekka Paalanen
Post by Giulio Camuffo
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Yup. I think that's a reasonable thing to do. It's meant to be phased
out even though we can "never" drop it completely. (except mutter
already did, IIRC).
It's not *that* bad. At worst, it's a flicker. At best, might not even
be visible.
The *title bar* will flicker. I think that is pretty bad.
I agree that it's not ideal.
I also think you are panicking about a non-existent problem. No client is
going to fail because it got two focus-in events in a row. Just send two
events irregardless of version, and stop adding needless complexity to
weston!
I can actually see them failing, tbh. Every time we change enter/leave
semantics under X11, we very subtly break quite a lot of clients, which
have more strict requirements on these events than you'd think/hope - I
spent years being bitterly disappointed (mostly on Peter's behalf). That we
strictly pair enter/leave events is IMO effectively part of the contract we
have with clients, and I guarantee you some are relying on a strict pairing.

I don't think 'but that's not part of the spec' is really an excuse either.
If we only allowed clients to rely on what was in the wl_keyboard spec,
no-one would ever be able to use keyboards at all. :\
You also mentioned an "enter debug mode" shortcut. That must NOT send a
focus-out event, as it could change the state of the client being debugged!
Heh. To be fair, these debug shortcuts are pretty ephemeral - things like
screenshots,
Well, there is also the part about triggering the binding on key
Post by Pekka Paalanen
Post by Giulio Camuffo
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.
No! Users do NOT want this. Keys take action when you push them. Otherwise
the interface looks slow and drives users crazy.
That sometimes conflicts with the goal of having ambiguous hotkeys. For
instance, if you press and release Mod on its own, you'll get Exposay mode;
Mod is, however, also used as the prefix for a million other hotkeys. So in
that case, we have to arm the shortcut when mod is pressed, disarm/release
it if any other keys were pressed, and then trigger on release.

Another classic example is Ctrl+Shift used to drive layout change (this is
the Windows default), when you still want Ctrl+Shift shortcuts.

I'd hope that this was restricted to modifiers, so we could just say as a
general rule that the compositor will drive modifier-only hotkeys (Exposay,
layout switch) on release, but all others on press. So if you have
ambiguous hotkey combinations which involve non-modifier keys, well, you
lose.
Mod down -> send key Mod down
Post by Pekka Paalanen
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave
Then either
's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.
or
'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down
I think you meant the kbd.enter would include the Mod *and* the 's' key.
Yes.
In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is what
users expect and what every toolkit in the world does on every platform.
There is nothing special about "compositor shortcuts" and they should NEVER
act differently than a client would that handles both shortcuts. Here is
I agree 100%. Again, think of nesting compositors: why should our
compositor behave completely different if it's nested? The more difficult
we make it to sensibly nest compositors, the more we're shooting ourselves
in the foot.

Cheers,
Dan
Daniel Stone
2014-11-20 08:14:45 UTC
Permalink
Hi,
Post by Daniel Stone
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
Post by Pekka Paalanen
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus" will
be set differently than the input device focus? I think you are talking
about "activation" which is DIFFERENT than keyboard focus.
Unless your goal is to make point-to-type impossible?
As far as I can tell from what the others are saying - which I'd totally
agree with - is that wl_keyboard's focus would be much more transient than
it is now, reflecting where keyboard events are _actually_ being delivered
_right now_. So if the compositor took a long-lived grab, e.g. for the lock
screen, you'd lose keyboard focus as you do today, as well as losing
xdg_shell focus. But if the compositor just took an ephemeral grab (e.g.
key binding), then while you would lose keyboard focus, you wouldn't lose
xdg_shell focus.
So think of keyboard focus as tightly bound to actual event delivery at
that very particular moment in time, and a hint that keys may be pressed
underneath you, so you could well come back with pressed keys you shouldn't
take action on, whereas shell focus is what you'd use to drive things like
rendering your decorations with (in)active highlights.
I should add that none of this precludes point-to-type/sloppy-focus. The
focus remains entirely at the discretion of the compositor, and this makes
it no more or less difficult than it is today: just adds another focus
annotation which better accounts for ephemeral focus changes.

Cheers,
Dan
Giulio Camuffo
2014-11-20 08:31:18 UTC
Permalink
Post by Daniel Stone
Hi,
Post by Pekka Paalanen
Post by Giulio Camuffo
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus" will
be set differently than the input device focus? I think you are talking
about "activation" which is DIFFERENT than keyboard focus.
Unless your goal is to make point-to-type impossible?
As far as I can tell from what the others are saying - which I'd totally
agree with - is that wl_keyboard's focus would be much more transient than
it is now, reflecting where keyboard events are _actually_ being delivered
_right now_. So if the compositor took a long-lived grab, e.g. for the lock
screen, you'd lose keyboard focus as you do today, as well as losing
xdg_shell focus. But if the compositor just took an ephemeral grab (e.g. key
binding), then while you would lose keyboard focus, you wouldn't lose
xdg_shell focus.
So think of keyboard focus as tightly bound to actual event delivery at that
very particular moment in time, and a hint that keys may be pressed
underneath you, so you could well come back with pressed keys you shouldn't
take action on, whereas shell focus is what you'd use to drive things like
rendering your decorations with (in)active highlights.
Post by Pekka Paalanen
Post by Giulio Camuffo
The only problem with that is that apps using wl_shell can only use
the keyboard focus as the window focus. Do we just say "too bad,
wl_shell is broken"?
Yup. I think that's a reasonable thing to do. It's meant to be phased
out even though we can "never" drop it completely. (except mutter
already did, IIRC).
It's not *that* bad. At worst, it's a flicker. At best, might not even
be visible.
The *title bar* will flicker. I think that is pretty bad.
I agree that it's not ideal.
I also think you are panicking about a non-existent problem. No client is
going to fail because it got two focus-in events in a row. Just send two
events irregardless of version, and stop adding needless complexity to
weston!
I can actually see them failing, tbh. Every time we change enter/leave
semantics under X11, we very subtly break quite a lot of clients, which have
more strict requirements on these events than you'd think/hope - I spent
years being bitterly disappointed (mostly on Peter's behalf). That we
strictly pair enter/leave events is IMO effectively part of the contract we
have with clients, and I guarantee you some are relying on a strict pairing.
I don't think 'but that's not part of the spec' is really an excuse either.
If we only allowed clients to rely on what was in the wl_keyboard spec,
no-one would ever be able to use keyboards at all. :\
You also mentioned an "enter debug mode" shortcut. That must NOT send a
focus-out event, as it could change the state of the client being debugged!
Heh. To be fair, these debug shortcuts are pretty ephemeral - things like
screenshots,
Post by Pekka Paalanen
Post by Giulio Camuffo
Well, there is also the part about triggering the binding on key
release instead of press, only if no other key was pressed in the mean
time. I hope to send the patch(es) for that one of these days.
No! Users do NOT want this. Keys take action when you push them. Otherwise
the interface looks slow and drives users crazy.
That sometimes conflicts with the goal of having ambiguous hotkeys. For
instance, if you press and release Mod on its own, you'll get Exposay mode;
Mod is, however, also used as the prefix for a million other hotkeys. So in
that case, we have to arm the shortcut when mod is pressed, disarm/release
it if any other keys were pressed, and then trigger on release.
Another classic example is Ctrl+Shift used to drive layout change (this is
the Windows default), when you still want Ctrl+Shift shortcuts.
I'd hope that this was restricted to modifiers, so we could just say as a
general rule that the compositor will drive modifier-only hotkeys (Exposay,
layout switch) on release, but all others on press. So if you have ambiguous
hotkey combinations which involve non-modifier keys, well, you lose.
Post by Pekka Paalanen
Mod down -> send key Mod down
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave
Then either
's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.
or
'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down
I think you meant the kbd.enter would include the Mod *and* the 's' key.
Yes.
In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is what
users expect and what every toolkit in the world does on every platform.
There is nothing special about "compositor shortcuts" and they should NEVER
act differently than a client would that handles both shortcuts. Here is
I agree 100%. Again, think of nesting compositors: why should our compositor
behave completely different if it's nested? The more difficult we make it to
sensibly nest compositors, the more we're shooting ourselves in the foot.
Ok, now you got me confused. Wasn't this what you said would be the
correct way to handle bindings?
Post by Daniel Stone
Cheers,
Dan
Pekka Paalanen
2014-11-21 08:57:32 UTC
Permalink
On Thu, 20 Nov 2014 10:31:18 +0200
Post by Giulio Camuffo
Post by Daniel Stone
Hi,
Post by Pekka Paalanen
Post by Pekka Paalanen
Just like Jasper said, it is a mistake to use wl_keyboard focus for
window focus, xdg_shell has a separate mechanism for window focus.
Window focus is a shell concept IMO, anyway.
Can you explain when (except to fix this bug) then "xdg_shell focus" will
be set differently than the input device focus? I think you are talking
about "activation" which is DIFFERENT than keyboard focus.
Unless your goal is to make point-to-type impossible?
As far as I can tell from what the others are saying - which I'd totally
agree with - is that wl_keyboard's focus would be much more transient than
it is now, reflecting where keyboard events are _actually_ being delivered
_right now_. So if the compositor took a long-lived grab, e.g. for the lock
screen, you'd lose keyboard focus as you do today, as well as losing
xdg_shell focus. But if the compositor just took an ephemeral grab (e.g. key
binding), then while you would lose keyboard focus, you wouldn't lose
xdg_shell focus.
So think of keyboard focus as tightly bound to actual event delivery at that
very particular moment in time, and a hint that keys may be pressed
underneath you, so you could well come back with pressed keys you shouldn't
take action on, whereas shell focus is what you'd use to drive things like
rendering your decorations with (in)active highlights.
Exactly.
Post by Giulio Camuffo
Post by Daniel Stone
You also mentioned an "enter debug mode" shortcut. That must NOT send a
focus-out event, as it could change the state of the client being debugged!
By the very same reasoning, nothing about the debug key combo should be
sent to any client - except that's not possible. Right now the debug
key works by first hitting Mod+Shift+Space, then hitting the actual
debug key (a letter key usually). At least the Mod and Shift will
somehow end up in some client.
Post by Giulio Camuffo
Post by Daniel Stone
Heh. To be fair, these debug shortcuts are pretty ephemeral - things like
screenshots,
Our debug things include tinting all GL-composited stuff green, drawing
the triangle mesh edges, toggling cursor and overlay planes of DRM, etc.
Post by Giulio Camuffo
Post by Daniel Stone
Post by Pekka Paalanen
Mod down -> send key Mod down
's' down -> oooh, hotkey, steal kbd focus, send kbd.leave
Then either
's' up -> do screenshot, restore kbd focus, send kbd.enter with Mod
down.
or
'x' down -> not our hotkey after all, restore kbd focus, send kbd.enter
with Mod down, send key 'x' down
I think you meant the kbd.enter would include the Mod *and* the 's' key.
Yes.
Whoops, yes indeed.
Post by Giulio Camuffo
Post by Daniel Stone
In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is what
users expect and what every toolkit in the world does on every platform.
There is nothing special about "compositor shortcuts" and they should NEVER
act differently than a client would that handles both shortcuts. Here is
Well, I just personally hate triggering unwanted side-actions when
reaching for a single final action. Maybe that's just me.

If the client was handling both shortcuts, it could do either way.
Post by Giulio Camuffo
Post by Daniel Stone
I agree 100%. Again, think of nesting compositors: why should our compositor
behave completely different if it's nested? The more difficult we make it to
sensibly nest compositors, the more we're shooting ourselves in the foot.
Ok, now you got me confused. Wasn't this what you said would be the
correct way to handle bindings?
Yeah, I'm confused too.

I tried to think of a stack of:
evdev <- Weston <- nested (compositor) <- app
and the different cases of how Weston behaves towards nested vs. OS
(e.g. VT-switching) behaves towards Weston, and I'm not sure I see what
nested would need to do differently from weston with what I thought.

So how should it work?

I would still assume, that if I press Mod+s to take a screenshot, I
wouldn't want the 's' to appear on the terminal that happened to have
the keyboard focus when Mod went down. If the terminal had a Mod+s
shortcut too, should it run or not?

That is, if all of Weston, nested, and app happened to have the same
shortcut, should all three fire, or only Weston's?


Thanks,
pq
Daniel Stone
2014-11-21 18:34:32 UTC
Permalink
Hi,
Post by Pekka Paalanen
On Thu, 20 Nov 2014 10:31:18 +0200
Post by Giulio Camuffo
Post by Bill Spitzak
You also mentioned an "enter debug mode" shortcut. That must NOT send a
Post by Bill Spitzak
focus-out event, as it could change the state of the client being
debugged!
By the very same reasoning, nothing about the debug key combo should be
sent to any client - except that's not possible. Right now the debug
key works by first hitting Mod+Shift+Space, then hitting the actual
debug key (a letter key usually). At least the Mod and Shift will
somehow end up in some client.
Shrug, so be it. Not a lot we can do about it, and this is also the case on
other OSes.
Post by Pekka Paalanen
Post by Giulio Camuffo
Post by Bill Spitzak
Heh. To be fair, these debug shortcuts are pretty ephemeral - things
like
Post by Giulio Camuffo
Post by Bill Spitzak
screenshots,
Our debug things include tinting all GL-composited stuff green, drawing
the triangle mesh edges, toggling cursor and overlay planes of DRM, etc.
I know, I use them all the time. :) I mean ephemeral in terms of keyboard
focus - the grab is active for one press, and focus then reverts to the
client - rather than ephemeral in terms of effect.
Post by Pekka Paalanen
Post by Giulio Camuffo
Post by Bill Spitzak
I agree 100%. Again, think of nesting compositors: why should our
compositor
Post by Giulio Camuffo
Post by Bill Spitzak
behave completely different if it's nested? The more difficult we make
it to
Post by Giulio Camuffo
Post by Bill Spitzak
sensibly nest compositors, the more we're shooting ourselves in the
foot.
Post by Giulio Camuffo
Ok, now you got me confused. Wasn't this what you said would be the
correct way to handle bindings?
Yeah, I'm confused too.
evdev <- Weston <- nested (compositor) <- app
and the different cases of how Weston behaves towards nested vs. OS
(e.g. VT-switching) behaves towards Weston, and I'm not sure I see what
nested would need to do differently from weston with what I thought.
I tried to write out the case that made me nervous in my head, but
couldn't. I'm not sure if that's because it's not actually a real problem,
or I'm just far too tired.
Post by Pekka Paalanen
So how should it work?
I would still assume, that if I press Mod+s to take a screenshot, I
wouldn't want the 's' to appear on the terminal that happened to have
the keyboard focus when Mod went down. If the terminal had a Mod+s
shortcut too, should it run or not?
That is, if all of Weston, nested, and app happened to have the same
shortcut, should all three fire, or only Weston's?
Only Weston.

All three need to obey the same rule:
- when triggering a hotkey, steal the focus and do _not_ send key events
- when the grab is ended, send the focus back to the client with the
true, correct, and full keyboard state in enter
- on an enter event, update internal state but do _not_ trigger actions
or hotkeys because of it

Cheers,
Daniel
Pekka Paalanen
2014-11-24 14:08:51 UTC
Permalink
On Fri, 21 Nov 2014 18:34:32 +0000
Post by Daniel Stone
Hi,
Post by Giulio Camuffo
Post by Giulio Camuffo
Post by Daniel Stone
I agree 100%. Again, think of nesting compositors: why should our
compositor
Post by Giulio Camuffo
Post by Daniel Stone
behave completely different if it's nested? The more difficult we make
it to
Post by Giulio Camuffo
Post by Daniel Stone
sensibly nest compositors, the more we're shooting ourselves in the
foot.
Post by Giulio Camuffo
Ok, now you got me confused. Wasn't this what you said would be the
correct way to handle bindings?
Yeah, I'm confused too.
evdev <- Weston <- nested (compositor) <- app
and the different cases of how Weston behaves towards nested vs. OS
(e.g. VT-switching) behaves towards Weston, and I'm not sure I see what
nested would need to do differently from weston with what I thought.
I tried to write out the case that made me nervous in my head, but
couldn't. I'm not sure if that's because it's not actually a real problem,
or I'm just far too tired.
Post by Giulio Camuffo
So how should it work?
I would still assume, that if I press Mod+s to take a screenshot, I
wouldn't want the 's' to appear on the terminal that happened to have
the keyboard focus when Mod went down. If the terminal had a Mod+s
shortcut too, should it run or not?
That is, if all of Weston, nested, and app happened to have the same
shortcut, should all three fire, or only Weston's?
Only Weston.
- when triggering a hotkey, steal the focus and do _not_ send key events
- when the grab is ended, send the focus back to the client with the
true, correct, and full keyboard state in enter
- on an enter event, update internal state but do _not_ trigger actions
or hotkeys because of it
Makes perfect sense, this and your other replies. "Steal focus" means
sending wl_keyboard.leave to the current focus, right?


Thanks,
pq

Daniel Stone
2014-11-21 18:30:02 UTC
Permalink
Hi,
Post by Pekka Paalanen
Post by Daniel Stone
Post by Bill Spitzak
No! Users do NOT want this. Keys take action when you push them.
Otherwise
Post by Daniel Stone
Post by Bill Spitzak
the interface looks slow and drives users crazy.
That sometimes conflicts with the goal of having ambiguous hotkeys. For
instance, if you press and release Mod on its own, you'll get Exposay
mode;
Post by Daniel Stone
Mod is, however, also used as the prefix for a million other hotkeys. So
in
Post by Daniel Stone
that case, we have to arm the shortcut when mod is pressed,
disarm/release
Post by Daniel Stone
it if any other keys were pressed, and then trigger on release.
Another classic example is Ctrl+Shift used to drive layout change (this
is
Post by Daniel Stone
the Windows default), when you still want Ctrl+Shift shortcuts.
I'd hope that this was restricted to modifiers, so we could just say as a
general rule that the compositor will drive modifier-only hotkeys
(Exposay,
Post by Daniel Stone
layout switch) on release, but all others on press. So if you have
ambiguous
Post by Daniel Stone
hotkey combinations which involve non-modifier keys, well, you lose.
[the above is salient]
Post by Pekka Paalanen
Post by Daniel Stone
Post by Bill Spitzak
In any case, DO NOT DO THIS!!!! What the above sequence should do is a
screenshot, followed by the action of Mod+s+x in the client. This is
what
Post by Daniel Stone
Post by Bill Spitzak
users expect and what every toolkit in the world does on every platform.
There is nothing special about "compositor shortcuts" and they should
NEVER
Post by Daniel Stone
Post by Bill Spitzak
act differently than a client would that handles both shortcuts. Here is
I agree 100%. Again, think of nesting compositors: why should our
compositor
Post by Daniel Stone
behave completely different if it's nested? The more difficult we make
it to
Post by Daniel Stone
sensibly nest compositors, the more we're shooting ourselves in the foot.
Ok, now you got me confused. Wasn't this what you said would be the
correct way to handle bindings?
Sorry, was kind of mixing theoretical and practical a little bit here.

The only case where you need action-on-release rather than on press, is
where you have ambiguous/overlapping hotkeys. For instance, Ctrl+Shift
switches keyboard layouts, Ctrl+Shift+Q closes Chrome. When you have these
kind of overlaps, you need to wait until release to disambiguate.

As in the first quoted section, I think it's not really reasonable to guard
against the possibility of Mod+S taking a screenshot in the compositor, but
the client having a shortcut for Mod+S+X. So I think we can take the easy
way out of saying:
- for any hotkeys which are _solely_ combinations of modifiers and _not_
non-modifier keys (e.g. Exposay as it is today), then we only take action
on release
- for all other hotkeys (i.e. including non-modifier keys), then we take
action on press
- if a client has a hotkey combination which conflicts with the above
(e.g. the Mod+S vs. Mod+S+X case), then that's their problem, because it's
a stupid hotkey combination anyway

Does that help?

Cheers,
Daniel
Bill Spitzak
2014-11-21 19:45:24 UTC
Permalink
Post by Daniel Stone
As in the first quoted section, I think it's not really reasonable to
guard against the possibility of Mod+S taking a screenshot in the
compositor, but the client having a shortcut for Mod+S+X. So I think we
- for any hotkeys which are _solely_ combinations of modifiers and
_not_ non-modifier keys (e.g. Exposay as it is today), then we only take
action on release
- for all other hotkeys (i.e. including non-modifier keys), then we
take action on press
- if a client has a hotkey combination which conflicts with the above
(e.g. the Mod+S vs. Mod+S+X case), then that's their problem, because
it's a stupid hotkey combination anyway
That seems correct to me. The point is that this sort of action is
irregardless of whether it is a "server shortcut" or not. If a client
had Ctrl+Shift do something and Ctrl+Shift+X do something else, then it
will trigger the first action on the release of Shift. The second action
it would trigger on the press of 'X'. The logic in the compositor is
identical. Nothing special is done about a key up or down until it
figures out it triggered a shortcut, then that up or down event is not
sent to the client.

I suppose in theory a client could have Ctrl+X and Ctrl+X+Y as shortcuts
and it then treats Ctrl+X as a modifier. And this seems to have led to
the concern that the server has to be able to do this, without the
ability to query other programs whether there is a conflicting key
combination. However I don't think there are any clients that do this,
for the simple reason that it will make the Ctrl+X shortcut look broken
or slow and contrary to user expectations. Since clients with a full
ability to implement this do not do this, there is no reason to try to
do it in this case where it is actually not possible.
Bill Spitzak
2014-11-21 20:10:46 UTC
Permalink
Post by Daniel Stone
As in the first quoted section, I think it's not really reasonable to
guard against the possibility of Mod+S taking a screenshot in the
compositor, but the client having a shortcut for Mod+S+X. So I think we
- for any hotkeys which are _solely_ combinations of modifiers and
_not_ non-modifier keys (e.g. Exposay as it is today), then we only take
action on release
I did think of one annoying conflict. My initial worry seems to be ok
provided clients are well-behaved, but a more obscure problem with key
release order does exist. I will try to describe it:

Imagine the server has Shift+Alt as a shortcut, and a client just has
Shift as a shortcut. Both of these adhere to convention and do not
trigger the event unless they see the *up* event.

Then the following happens:

1. Shift down -> sent to client
2. Alt down -> sent to client
3. Alt up -> triggers shortcut, send updated keymap to client
4. Shift up -> ??? this should not trigger event in client

At first I thought this might be a big problem, but I now think it is ok
if the shift-up is sent to the client. This should not trigger the shift
event because the Alt key was pressed and the client saw that. If the
client did respond to the shift-up then it can be considered a client bug.

Then I realized that steps 3 and 4 above (the release events) could be
swapped. This should still trigger the server shortcut and avoid
triggering the client shortcut. This is only going to happen if
everybody agrees on which up event is the triggering one. Unfortunately
there are different implementations:

1. The first up is the trigger
2. The up corresponding to the last down is the trigger
3. The last up (ie when all modifiers are released) is the trigger
4. others I did not think of...

It looks like all wayland clients are going to have to agree on the
rules. Otherwise the compositor will not "eat" the event that the client
uses as the trigger. I get the impression that #2 is the only scheme
that will work, but I have not been able to convince myself of this yet.
It may also be useful to try existing software, especially on Windows
where this use of modifiers as triggers is more common, to see if there
is any standard.
Bill Spitzak
2014-11-20 20:37:29 UTC
Permalink
Post by Daniel Stone
As far as I can tell from what the others are saying - which I'd totally
agree with - is that wl_keyboard's focus would be much more transient
than it is now, reflecting where keyboard events are _actually_ being
delivered _right now_. So if the compositor took a long-lived grab, e.g.
for the lock screen, you'd lose keyboard focus as you do today, as well
as losing xdg_shell focus. But if the compositor just took an ephemeral
grab (e.g. key binding), then while you would lose keyboard focus, you
wouldn't lose xdg_shell focus.
So think of keyboard focus as tightly bound to actual event delivery at
that very particular moment in time, and a hint that keys may be pressed
underneath you, so you could well come back with pressed keys you
shouldn't take action on, whereas shell focus is what you'd use to drive
things like rendering your decorations with (in)active highlights.
So basically you have two events that the client must not respond to!

One of them is now the "the key map changed" event. And the other is "a
useless event that must be sent before the key map changed event".

I don't need a "hint that keys may be pressed underneath you". I KNOW
"keys were pressed underneath me" when I get a focus-in event with the
key map changed!

I would get rid of this pointless complexity and restore these events to
their original purpose, which was to instruct clients to update their
display to show that they are receiving keyboard focus. And I would
allow the redundant one, because I think your attempts to "pair" events
are making things worse. And I have real experience in this due to
Post by Daniel Stone
I can actually see them failing, tbh. Every time we change enter/leave
semantics under X11, we very subtly break quite a lot of clients, which
have more strict requirements on these events than you'd think/hope - I
spent years being bitterly disappointed (mostly on Peter's behalf). That
we strictly pair enter/leave events is IMO effectively part of the
contract we have with clients, and I guarantee you some are relying on a
strict pairing.
You are WRONG here. ALL the problems I have had with X11 changes are due
to you guys enforcing useless pairing of events.

In particular I must now peek ahead in the event queue on every key up
in order to detect if it is a fake key-up due to a repeat key. That
change broke hundreds of applications and toolkits including the
software I was working on (it basically broke every program using keys
as modifiers). A very useful program called Amazon Paint had to be
abandoned because of this and forced all work to be moved from Unix to
Windows and Photoshop used instead, at tremendous cost and immense harm
to the future of Linux. So don't you dare claim you are trying to not
break clients. Somebody panicked about "mispairing of events" and went
nuts and broke far more software and caused more damage than they could
ever have imagined.

In addition in all versions of X I have to peek ahead on all exit events
to see if it is due to an enter into another window that my client owns,
to avoid blinking in my redrawing. This is stupid and would be trivial
to fix by sending the enter event first, or not sending the exit event.
But because somebody seems to think some perverse piece of software will
crash because the events are not "paired", both the client and server
software have to be made much more complicated!

Sorry, please stop this. Try writing some clients. The pairing is
absolutely useless, incredibly TRIVIAL for a client to work around, and
complicates every layer of your software needlessly. Stop it.
Post by Daniel Stone
No! Users do NOT want this. Keys take action when you push them.
Otherwise the interface looks slow and drives users crazy.
That sometimes conflicts with the goal of having ambiguous hotkeys. For
instance, if you press and release Mod on its own, you'll get Exposay
mode; Mod is, however, also used as the prefix for a million other
hotkeys. So in that case, we have to arm the shortcut when mod is
pressed, disarm/release it if any other keys were pressed, and then
trigger on release.
Another classic example is Ctrl+Shift used to drive layout change (this
is the Windows default), when you still want Ctrl+Shift shortcuts.
I'd hope that this was restricted to modifiers, so we could just say as
a general rule that the compositor will drive modifier-only hotkeys
(Exposay, layout switch) on release, but all others on press. So if you
have ambiguous hotkey combinations which involve non-modifier keys,
well, you lose.
Yes restricting this behavior to modifiers makes sense. This does match
what clients do and therefore will inter-operate. The problem was the
previous example was talking about a non-modifier. Those should not act
different than any client would do.

So here is an example where "Mod" is a server shortcut:

Mod down -> send key Mod down

Mod up -> execute the shortcut, send redundant focus-in with keymap
with mod key removed. Notice that no UP event is sent.

Here is what happens if "Mod+x" is a shortcut handled by the client:

Mod down -> send key Mod down

'x' down -> send key 'x' down (client may do something with it)

Mod up -> not a shortcut, send Mod up to client

Here is what happens if "Mod+s" is a shortcut handled by server:

Mod down -> send key Mod down

's' down -> execute the shortcut, send redundant focus-in with keymap
with 's' key added.

's' up -> send 's' up to client

What I have done in software supporting user-definable keybindings is at
a low level it produces messages for modifiers when they are released
with no other changes since they were pressed. It produces messages for
all other keys on down. This allows all the shortcuts, including the
shift keys, to be in the same table. This exactly matches what event is
eaten by the shortcut-handling server (the down events for normal keys
and the up events for modifiers).
Daniel Stone
2014-11-21 18:59:17 UTC
Permalink
Right.
Post by Bill Spitzak
Post by Daniel Stone
As far as I can tell from what the others are saying - which I'd totally
agree with - is that wl_keyboard's focus would be much more transient
than it is now, reflecting where keyboard events are _actually_ being
delivered _right now_. So if the compositor took a long-lived grab, e.g.
for the lock screen, you'd lose keyboard focus as you do today, as well
as losing xdg_shell focus. But if the compositor just took an ephemeral
grab (e.g. key binding), then while you would lose keyboard focus, you
wouldn't lose xdg_shell focus.
So think of keyboard focus as tightly bound to actual event delivery at
that very particular moment in time, and a hint that keys may be pressed
underneath you, so you could well come back with pressed keys you
shouldn't take action on, whereas shell focus is what you'd use to drive
things like rendering your decorations with (in)active highlights.
So basically you have two events that the client must not respond to!
No. What on earth makes you think that?
Post by Bill Spitzak
One of them is now the "the key map changed" event.
Which you respond to by updating your internal state. Not useless.
Post by Bill Spitzak
And the other is "a useless event that must be sent before the key map
changed event".
Which you respond to by ceasing _all_ actions related to your current
keyboard state, such as cancelling key repeat. So, also not useless.
Post by Bill Spitzak
I don't need a "hint that keys may be pressed underneath you". I KNOW
"keys were pressed underneath me" when I get a focus-in event with the key
map changed!
And in the meantime, you've just repeated whatever keys were down when the
focus left for the entire time. Sounds great, where do I sign up?
Post by Bill Spitzak
I would get rid of this pointless complexity and restore these events to
their original purpose, which was to instruct clients to update their
display to show that they are receiving keyboard focus. And I would allow
the redundant one, because I think your attempts to "pair" events are
making things worse. And I have real experience in this due to changes that
It's not pointless, so no.
Post by Bill Spitzak
I can actually see them failing, tbh. Every time we change enter/leave
Post by Daniel Stone
semantics under X11, we very subtly break quite a lot of clients, which
have more strict requirements on these events than you'd think/hope - I
spent years being bitterly disappointed (mostly on Peter's behalf). That
we strictly pair enter/leave events is IMO effectively part of the
contract we have with clients, and I guarantee you some are relying on a
strict pairing.
You are WRONG here. ALL the problems I have had with X11 changes are due
to you guys enforcing useless pairing of events.
I assume by 'you guys', you mean 'the guys who wrote the X11 spec whilst I
was less than a year old'. And since the spec requires it, enforcing it is
required for compilance with the spec, therefore it is not useless. Even if
it didn't have any other use, which it does. But let's continue.
Post by Bill Spitzak
In particular I must now peek ahead in the event queue on every key up in
order to detect if it is a fake key-up due to a repeat key. That change
broke hundreds of applications and toolkits including the software I was
working on (it basically broke every program using keys as modifiers). A
very useful program called Amazon Paint had to be abandoned because of this
and forced all work to be moved from Unix to Windows and Photoshop used
instead, at tremendous cost and immense harm to the future of Linux. So
don't you dare claim you are trying to not break clients. Somebody panicked
about "mispairing of events" and went nuts and broke far more software and
caused more damage than they could ever have imagined.
I don't know where this came from, but it's both massively unrelated, and
incorrect.

The way you detect repeat in _core_ X11, is by a stream of paired KeyPress
and KeyRelease events. Look it up. Clients rely on this, and conformance
relies on it too. I don't know if you've heard of an extension called the X
Keyboard Extension (obscure name, I know), but since its inclusion in
X11R6.1 in 1996, the XkbSetDetectableAutorepeat function has allowed you to
request key repeat be sent as a stream of press-press-press-press-release
events, rather than press-release-press-release-press-release. For bonus
points, it's even part of the core libX11.so.6 API/ABI. And it works
perfectly, and every real toolkit I've seen has made good use of it.

So I don't know who took the decision to not use a toolkit but instead
write their own, to write a toolkit without seemingly looking at any prior
art, or to apparently abandon an entire product on the basis that peeking
ahead in the event stream was more difficult than five minutes on Google,
but this is a person who should so comprehensively not be in charge of a
software product that whatever they were running would've been doomed
regardless.

This is not something which has ever changed in X11, ever. It's not a
recent development that someone panicked and changed the X server to do it;
it's the same behaviour we've had since 1987, in which time people have
worked out how to deal with it. Or most people, anyway.

Onwards.
Post by Bill Spitzak
In addition in all versions of X I have to peek ahead on all exit events
to see if it is due to an enter into another window that my client owns, to
avoid blinking in my redrawing. This is stupid and would be trivial to fix
by sending the enter event first, or not sending the exit event. But
because somebody seems to think some perverse piece of software will crash
because the events are not "paired", both the client and server software
have to be made much more complicated!
That 'somebody' would again be the people who wrote the spec that people
rely on. How do we know that people rely on it? Because some pretty fiddly
bugs crept in after MPX, and a _lot_ of people complained to RHEL that
their apps broke, and Peter had to fix it. This probably went on for about
five years before all of them were properly stomped out.

I agree it would be nice to have a notification that it's going to a
surface owned by the same client, mind. It's something I've got on my todo
list to fix, but your combination of furious, insulting, condescending, and
(perhaps worst of all) totally flat-out incorrect, is not really motivating
me to fix it.
Post by Bill Spitzak
Sorry, please stop this. Try writing some clients. The pairing is
absolutely useless, incredibly TRIVIAL for a client to work around, and
complicates every layer of your software needlessly. Stop it.
I'm ignoring your jump from 'it has effects I don't like' to 'it has no
useful effects', because it's ridiculous.

I'm also ignoring your request to 'write some clients', because I've done
it for years now (three toolkits, three EGL stacks, a browser, a media
framework, so on, so forth). On the other hand, you prove almost every
single thread that you have never ever written a single client in your
life. Not only do you not understand any of the fundamentals of the Wayland
protocol - things which are obvious to anyone who's ever written any client
more complicated than wl_display_connect(); exit(); - but apparently you
don't particularly understand how X works either.
Post by Bill Spitzak
I'd hope that this was restricted to modifiers, so we could just say as
Post by Daniel Stone
a general rule that the compositor will drive modifier-only hotkeys
(Exposay, layout switch) on release, but all others on press. So if you
have ambiguous hotkey combinations which involve non-modifier keys,
well, you lose.
Yes restricting this behavior to modifiers makes sense. This does match
what clients do and therefore will inter-operate.
Good.
Post by Bill Spitzak
The problem was the previous example was talking about a non-modifier.
Those should not act different than any client would do.
[snip]
These are all correct.
Post by Bill Spitzak
What I have done in software supporting user-definable keybindings is at a
low level it produces messages for modifiers when they are released with no
other changes since they were pressed. It produces messages for all other
keys on down. This allows all the shortcuts, including the shift keys, to
be in the same table. This exactly matches what event is eaten by the
shortcut-handling server (the down events for normal keys and the up events
for modifiers).
Thanks for this totally useless information.

I didn't want to reply to this mail, since not only does it contribute
absolutely nothing of value to a thread which is already borderline
impossible to get through, but is incredibly rude and insulting. On the
other hand, it was so full of absolute fiction that I felt compelled to,
lest someone stumble on it in the archives one day and think any of it (bar
the small bit at the end) made even the smallest amount of sense.

Have a great weekend.
Bill Spitzak
2014-11-21 21:11:40 UTC
Permalink
Post by Bill Spitzak
And the other is "a useless event that must be sent before the key
map changed event".
Which you respond to by ceasing _all_ actions related to your current
keyboard state, such as cancelling key repeat. So, also not useless.
You are right, it is the "stop repeating keys" event. I didn't think of
that. Depending on exactly what keystrokes the server is consuming, it
will have to send this, and right away, rather than deferring it to
immediately before the enter event, as I was thinking of.

I personally feel it is a mistake to have the client do repeat rather
than the compositor, because complexities of timing will not be
identical between clients, and because it makes it really difficult to
do remote display on a system that does the repeat itself. However this
event would still be necessary to cancel "key held down for this long"
timeouts.
Post by Bill Spitzak
The way you detect repeat in _core_ X11, is by a stream of paired
KeyPress and KeyRelease events. Look it up. Clients rely on this, and
conformance relies on it too. I don't know if you've heard of an
extension called the X Keyboard Extension (obscure name, I know), but
since its inclusion in X11R6.1 in 1996, the XkbSetDetectableAutorepeat
function has allowed you to request key repeat be sent as a stream of
press-press-press-press-release events, rather than
press-release-press-release-press-release. For bonus points, it's even
part of the core libX11.so.6 API/ABI. And it works perfectly, and every
real toolkit I've seen has made good use of it.
The "old" keyboard api (I think this is what you meant by core) in X
sent repeated press events. This was certainly true for IRIX and I
believe it was true for the first versions on Linux, because the errors
would be obvious if it was different. Forms, XForms, FLTK, Tk and
whatever Amazon paint used assumed it worked this way. Prisms and Alias
Wavefront did too but they fixed them pretty quick.

You can blame it on the documentation that I never saw
XkbSetDetectableAutorepeat. The books we were using for reference
predate the change. Because of the enter+exit problem, the toolkit had
peek-ahead code for the event stream already, so it was pretty easy to
modify this. I suppose if X had done the enter+exit events correctly the
peek-ahead code would not have existed and I might have searched harder
and found this call.

Also it looks like this function is allowed to fail and return false.
This makes it pretty useless as the look-ahead solution has to be
implemented anyway as a fallback. Due to the need to minimize code paths
to reduce chances of bugs I certainly would not have used this call at
all and used the fallback always. But I also suspect this is
documentation over-kill and it actually always works: it should not have
been defined as returning a failure code. This sort of mistake is what
leads to misuse on non-use of documented API.

There are also indications that various bugs make it return true but not
work anyway: https://bugs.freedesktop.org/show_bug.cgi?id=22515.
Encountering such a bug would also certainly have stopped us using the call.
Post by Bill Spitzak
So I don't know who took the decision to not use a toolkit but instead
write their own, to write a toolkit without seemingly looking at any
prior art,
You may be unaware of the conditions of things in 1996 but the
alternative to writing your own toolkit was to use Motif. I can assure
you that the decision was not hard.

Looking at Motif source code is pretty near useless as it is obfuscated
beyond belief (it also would not have contained this call for obvious
reasons). I based our code on the source for Forms and Tk and GLUT and
samples from the web and from reading the blue X11 reference books. GTK
was the Gimp Toolkit then and considered part of Gimp (in fact Stallman
sent me letters because they were considering using FLTK as the base
toolkit, I screwed up on getting the necessary permissions so this did
not happen). The K desktop had just appeared, so I guess a rudimentary
version of Qt was available, but we never looked at that either.
Post by Bill Spitzak
or to apparently abandon an entire product on the basis that
peeking ahead in the event stream was more difficult than five minutes
on Google
The product was closed source and they were not maintaining the Unix
version. And Google did not exist in 1996.
Post by Bill Spitzak
This is not something which has ever changed in X11, ever. It's not a
recent development that someone panicked and changed the X server to do
it; it's the same behaviour we've had since 1987, in which time people
have worked out how to deal with it. Or most people, anyway.
No I have absolute proof that on Irix and early Linux it did not work
this way. The documentation for XkbSetDetectableAutorepeat here
http://linux.die.net/man/3/xkbsetdetectableautorepeat seems to say that
earlier X servers did not work this way. Perhaps the X11 standard
claimed it worked this way (I am not going to read the book to try to
find any statement one way or another), but the implementations did not,
and the implementations win over the documentation all the time.

Anyway I'm sorry about being mad, but the experience certainly did piss
me off at that time, enough that I am still pretty upset about it 20
years later. I had (and still have) great hopes for Wayland, but I am
disturbed to see some of the same mistakes from then being repeated.
Daniel Stone
2014-11-21 21:44:16 UTC
Permalink
Hi,
Post by Bill Spitzak
Post by Daniel Stone
Which you respond to by ceasing _all_ actions related to your current
keyboard state, such as cancelling key repeat. So, also not useless.
You are right, it is the "stop repeating keys" event. I didn't think of
that. Depending on exactly what keystrokes the server is consuming, it will
have to send this, and right away, rather than deferring it to immediately
before the enter event, as I was thinking of.
I personally feel it is a mistake to have the client do repeat rather than
the compositor, because complexities of timing will not be identical
between clients, and because it makes it really difficult to do remote
display on a system that does the repeat itself. However this event would
still be necessary to cancel "key held down for this long" timeouts.
I don't see how it makes it difficult to do remote display, and also
wl_keyboard does send the repeat rate and delay through. No reason to wake
the compositor up when the client could equally do it on its own.

Either way, it's a done deal now: for better or worse, that's how
wl_keyboard works. It's what we promise clients, so we can't be changing it
from under them.
Post by Bill Spitzak
The way you detect repeat in _core_ X11, is by a stream of paired
Post by Daniel Stone
KeyPress and KeyRelease events. Look it up. Clients rely on this, and
conformance relies on it too. I don't know if you've heard of an
extension called the X Keyboard Extension (obscure name, I know), but
since its inclusion in X11R6.1 in 1996, the XkbSetDetectableAutorepeat
function has allowed you to request key repeat be sent as a stream of
press-press-press-press-release events, rather than
press-release-press-release-press-release. For bonus points, it's even
part of the core libX11.so.6 API/ABI. And it works perfectly, and every
real toolkit I've seen has made good use of it.
The "old" keyboard api (I think this is what you meant by core) in X sent
repeated press events. This was certainly true for IRIX and I believe it
was true for the first versions on Linux, because the errors would be
obvious if it was different. Forms, XForms, FLTK, Tk and whatever Amazon
paint used assumed it worked this way. Prisms and Alias Wavefront did too
but they fixed them pretty quick.
Sorry, but this isn't true. If it was, XkbSetDetectableAutorepeat would
literally have no reason at all to exist. Its sole reason is to change the
repeat behaviour of the server from press-release-press-release to
press-press-release. See section 4.1.2 of the XKB spec.
Post by Bill Spitzak
Also it looks like this function is allowed to fail and return false. This
makes it pretty useless as the look-ahead solution has to be implemented
anyway as a fallback. Due to the need to minimize code paths to reduce
chances of bugs I certainly would not have used this call at all and used
the fallback always. But I also suspect this is documentation over-kill and
it actually always works: it should not have been defined as returning a
failure code. This sort of mistake is what leads to misuse on non-use of
documented API.
Yes, if the XKB extension isn't supported, then you have to cope with the
lack of that. Again, press-release-press-release predates XKB, to the core
protocol.
Post by Bill Spitzak
There are also indications that various bugs make it return true but not
work anyway: https://bugs.freedesktop.org/show_bug.cgi?id=22515.
Encountering such a bug would also certainly have stopped us using the call.
I fixed that bug within literally two weeks of being filed! That's like
saying that you can't rely on Wayland to ever work because one version of
Weston had a segfault once (ho ho).

Anyway I'm sorry about being mad, but the experience certainly did piss me
Post by Bill Spitzak
off at that time, enough that I am still pretty upset about it 20 years
later. I had (and still have) great hopes for Wayland, but I am disturbed
to see some of the same mistakes from then being repeated.
I agree with you that enter/leave could be improved here, with a maybe-NULL
surface pointer which would be set if focus was transferring to another
surface from the same client. Like I said, that's actually been somewhere
on my TODO for a while, pending some time to get back to Wayland work.

I don't think this thread is the place to discuss that though; it's more
than run its course. So, rather than muddy the waters any more, how about
we take it to a separate thread if we want to carry this on - but ideally
one with some patches behind it too.

Let's call this EOT for now and move on with our lives. It's Friday night,
after all.

Cheers,
Daniel
Peter Hutterer
2014-11-17 07:54:33 UTC
Permalink
Post by Daniel Stone
Hi,
Post by Bill Spitzak
Post by Daniel Stone
Think about the case where pressing Alt on its own will focus the system
menu, but pressing Alt-F will raise the file menu directly. Let's say the
compositor has an Alt-F12 hotkey which triggers some action and then
immediately returns focus. We'll come back to the client with Alt held
down,
Post by Daniel Stone
and nothing else. When we come back, we do _not_ want the client to focus
the menu as if Alt was pressed normally. However, we _do_ want the
client to
Post by Daniel Stone
raise the file menu if F is then pressed (so, Alt+F). This exactly
matches
Post by Daniel Stone
the previous paragraph: update your internal state, but do not
immediately
Post by Daniel Stone
trigger any actions.
But this should not be affected by this patch. The key binding just
eats the F12, so the Alt is still being sent to the client.
So we've introduced an inconsistency. It happens to mostly kind of work for
modifiers, but that's luck of the draw.
Post by Bill Spitzak
We can see exactly one outlier here, doing the exactly wrong thing, which is
Post by Daniel Stone
XWayland. So it stands to reason that XWayland should be fixed: changing
the
Post by Daniel Stone
others would introduce massive inconsistencies depending on whether your
compositor is native or nested, which makes ~no sense.
I perfectly agree XWayland is broken here. But i don't see how this
patch breaks weston.
By introducing an inconsistency for, as far as I can see, no reason. That
it mostly happens to work is great, but that's luck rather than design.
Post by Bill Spitzak
Post by Daniel Stone
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.
But consider this case: There is a Alt+X compositor binding which
triggers something, without changing the focus. Then the client has a
Alt+X+Y binding. When pressing Alt+X the X is eaten by the compositor,
so trying to press also Y to trigger the client binding will have no
effect, because the client will actually see just Alt+Y. Without this
patch, if the Alt+X compositor binding was to instead switch the focus
to the client, the Alt+X+Y binding would work, because the client sees
the X too. Now it doesn't, as it doesn't without switching the focus.
This is where it is more consistent.
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers
the shortcut), then the client can use the keys array to notice this, and
ensure the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on
X before it arms the shortcut for Y.
In both cases, no problem. If we suppress X in the key-down array, we break
the first case, and in the second case, we send the client a release for a
key it's never seen. IOW, we introduce enormous inconsistencies in the key
book-keeping. As there's ~nothing more infuriating than broken key/button
state handling, and it's already relatively complicated code, I don't see
how any good can come out of making it easier to screw up for pretty much
no gain.
Given the above, and the possibility of making XWayland do the right thing,
I just can't see how this patch improves the status quo (pre-this-patch),
given the enormous downsides.
fwiw, I'm on Daniel's side here. Swallowing the key event doesn't work, it
just looks like it does for a limited set of use-cases.

Cheers,
Peter
Pekka Paalanen
2014-11-19 11:58:14 UTC
Permalink
On Mon, 17 Nov 2014 17:54:33 +1000
Post by Peter Hutterer
Post by Daniel Stone
Hi,
Post by Bill Spitzak
Post by Daniel Stone
Think about the case where pressing Alt on its own will focus the system
menu, but pressing Alt-F will raise the file menu directly. Let's say the
compositor has an Alt-F12 hotkey which triggers some action and then
immediately returns focus. We'll come back to the client with Alt held
down,
Post by Daniel Stone
and nothing else. When we come back, we do _not_ want the client to focus
the menu as if Alt was pressed normally. However, we _do_ want the
client to
Post by Daniel Stone
raise the file menu if F is then pressed (so, Alt+F). This exactly
matches
Post by Daniel Stone
the previous paragraph: update your internal state, but do not
immediately
Post by Daniel Stone
trigger any actions.
But this should not be affected by this patch. The key binding just
eats the F12, so the Alt is still being sent to the client.
So we've introduced an inconsistency. It happens to mostly kind of work for
modifiers, but that's luck of the draw.
Post by Bill Spitzak
We can see exactly one outlier here, doing the exactly wrong thing, which is
Post by Daniel Stone
XWayland. So it stands to reason that XWayland should be fixed: changing
the
Post by Daniel Stone
others would introduce massive inconsistencies depending on whether your
compositor is native or nested, which makes ~no sense.
I perfectly agree XWayland is broken here. But i don't see how this
patch breaks weston.
By introducing an inconsistency for, as far as I can see, no reason. That
it mostly happens to work is great, but that's luck rather than design.
Post by Bill Spitzak
Post by Daniel Stone
I think the fix makes Weston act _less_ consistently, especially when you
consider how it behaves when it's nested. If Weston nested under Wayland
behaves differently to DRM, that's proof that our model is broken.
But consider this case: There is a Alt+X compositor binding which
triggers something, without changing the focus. Then the client has a
Alt+X+Y binding. When pressing Alt+X the X is eaten by the compositor,
so trying to press also Y to trigger the client binding will have no
effect, because the client will actually see just Alt+Y. Without this
patch, if the Alt+X compositor binding was to instead switch the focus
to the client, the Alt+X+Y binding would work, because the client sees
the X too. Now it doesn't, as it doesn't without switching the focus.
This is where it is more consistent.
But this is just a client issue, and nothing in sending the full keys array
precludes this working.
If Alt+X is a modifier (i.e. any time Alt+X is held, pressing Y triggers
the shortcut), then the client can use the keys array to notice this, and
ensure the shortcut is fired.
If Alt+(X+Y) is a cumulative shortcut, then the client knows from seeing X
in the enter array but not a key event, that it must wait for a release on
X before it arms the shortcut for Y.
In both cases, no problem. If we suppress X in the key-down array, we break
the first case, and in the second case, we send the client a release for a
key it's never seen. IOW, we introduce enormous inconsistencies in the key
book-keeping. As there's ~nothing more infuriating than broken key/button
state handling, and it's already relatively complicated code, I don't see
how any good can come out of making it easier to screw up for pretty much
no gain.
Given the above, and the possibility of making XWayland do the right thing,
I just can't see how this patch improves the status quo (pre-this-patch),
given the enormous downsides.
fwiw, I'm on Daniel's side here. Swallowing the key event doesn't work, it
just looks like it does for a limited set of use-cases.
commit 86b5396d896a747495721d9c00670a039b704d18
Author: Pekka Paalanen <***@collabora.co.uk>
Date: Wed Nov 19 13:43:32 2014 +0200

Revert "input: don't send to clients key events eaten by bindings"

Pushed.

Thanks,
pq
Giulio Camuffo
2014-11-11 09:22:51 UTC
Permalink
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.
---

v4: - initialize and release the eaten_keys array
- check the eaten_keys array is empty before updating the keymap
(i'm am not completely sure this is correct to be honest, but i think so :) )

src/bindings.c | 7 +++++--
src/compositor.h | 3 ++-
src/input.c | 57 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 48 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;

struct wl_array keys;
+ struct wl_array eaten_keys;

struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..7819a41 100644
--- a/src/input.c
+++ b/src/input.c
@@ -1307,6 +1307,26 @@ update_keymap(struct weston_seat *seat)
}
#endif

+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1335,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;

if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,28 +1347,26 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;

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;
}

+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);

if (keyboard->pending_keymap &&
@@ -1430,6 +1450,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }

seat->modifier_state = 0;
--
2.1.3
Giulio Camuffo
2014-11-11 09:23:40 UTC
Permalink
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.
---

v4: - initialize and release the eaten_keys array
- check the eaten_keys array is empty before updating the keymap
(i'm am not completely sure this is correct to be honest, but i think so :) )

src/bindings.c | 7 ++++--
src/compositor.h | 3 ++-
src/input.c | 67 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/src/bindings.c b/src/bindings.c
index 5e24725..369c81a 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 eaten;

/* 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;

struct wl_array keys;
+ struct wl_array eaten_keys;

struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..5cb2703 100644
--- a/src/input.c
+++ b/src/input.c
@@ -526,6 +526,7 @@ weston_keyboard_create(void)
wl_list_init(&keyboard->focus_resource_listener.link);
keyboard->focus_resource_listener.notify = keyboard_focus_resource_destroyed;
wl_array_init(&keyboard->keys);
+ wl_array_init(&keyboard->eaten_keys);
keyboard->default_grab.interface = &default_keyboard_grab_interface;
keyboard->default_grab.keyboard = keyboard;
keyboard->grab = &keyboard->default_grab;
@@ -552,6 +553,7 @@ weston_keyboard_destroy(struct weston_keyboard *keyboard)
#endif

wl_array_release(&keyboard->keys);
+ wl_array_release(&keyboard->eaten_keys);
wl_list_remove(&keyboard->focus_resource_listener.link);
free(keyboard);
}
@@ -1307,6 +1309,26 @@ update_keymap(struct weston_seat *seat)
}
#endif

+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1337,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;

if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,32 +1349,31 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;

if (grab == &keyboard->default_grab ||
grab == &keyboard->input_method_grab) {
- weston_compositor_run_key_binding(compositor, seat, time, key,
- state);
+ eaten = weston_compositor_run_key_binding(compositor, seat,
+ time, key, state);
grab = keyboard->grab;
}

+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);

if (keyboard->pending_keymap &&
- keyboard->keys.size == 0)
+ keyboard->keys.size == 0 &&
+ keyboard->eaten_keys.size == 0)
update_keymap(seat);

if (update_state == STATE_UPDATE_AUTOMATIC) {
@@ -1430,6 +1453,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }

seat->modifier_state = 0;

@@ -2083,7 +2111,8 @@ weston_seat_update_keymap(struct weston_seat *seat, struct xkb_keymap *keymap)
xkb_keymap_unref(seat->keyboard->pending_keymap);
seat->keyboard->pending_keymap = xkb_keymap_ref(keymap);

- if (seat->keyboard->keys.size == 0)
+ if (seat->keyboard->keys.size == 0 &&
+ seat->keyboard->eaten_keys.size == 0)
update_keymap(seat);
#endif
}
--
2.1.3
Pekka Paalanen
2014-11-11 13:10:52 UTC
Permalink
On Tue, 11 Nov 2014 11:23:40 +0200
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.
---
v4: - initialize and release the eaten_keys array
- check the eaten_keys array is empty before updating the keymap
(i'm am not completely sure this is correct to be honest, but i think so :) )
src/bindings.c | 7 ++++--
src/compositor.h | 3 ++-
src/input.c | 67 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 55 insertions(+), 22 deletions(-)
diff --git a/src/bindings.c b/src/bindings.c
index 5e24725..369c81a 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 eaten;
/* 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;
struct wl_array keys;
+ struct wl_array eaten_keys;
struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..5cb2703 100644
--- a/src/input.c
+++ b/src/input.c
@@ -526,6 +526,7 @@ weston_keyboard_create(void)
wl_list_init(&keyboard->focus_resource_listener.link);
keyboard->focus_resource_listener.notify = keyboard_focus_resource_destroyed;
wl_array_init(&keyboard->keys);
+ wl_array_init(&keyboard->eaten_keys);
keyboard->default_grab.interface = &default_keyboard_grab_interface;
keyboard->default_grab.keyboard = keyboard;
keyboard->grab = &keyboard->default_grab;
@@ -552,6 +553,7 @@ weston_keyboard_destroy(struct weston_keyboard *keyboard)
#endif
wl_array_release(&keyboard->keys);
+ wl_array_release(&keyboard->eaten_keys);
wl_list_remove(&keyboard->focus_resource_listener.link);
free(keyboard);
}
@@ -1307,6 +1309,26 @@ update_keymap(struct weston_seat *seat)
}
#endif
+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1337,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,32 +1349,31 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;
if (grab == &keyboard->default_grab ||
grab == &keyboard->input_method_grab) {
- weston_compositor_run_key_binding(compositor, seat, time, key,
- state);
+ eaten = weston_compositor_run_key_binding(compositor, seat,
+ time, key, state);
grab = keyboard->grab;
}
+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);
if (keyboard->pending_keymap &&
- keyboard->keys.size == 0)
+ keyboard->keys.size == 0 &&
+ keyboard->eaten_keys.size == 0)
update_keymap(seat);
if (update_state == STATE_UPDATE_AUTOMATIC) {
@@ -1430,6 +1453,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }
seat->modifier_state = 0;
@@ -2083,7 +2111,8 @@ weston_seat_update_keymap(struct weston_seat *seat, struct xkb_keymap *keymap)
xkb_keymap_unref(seat->keyboard->pending_keymap);
seat->keyboard->pending_keymap = xkb_keymap_ref(keymap);
- if (seat->keyboard->keys.size == 0)
+ if (seat->keyboard->keys.size == 0 &&
+ seat->keyboard->eaten_keys.size == 0)
update_keymap(seat);
#endif
}
Looks good enough to me, pushed.

I do wonder about notify_keyboard_focus_in(), though. That one is also
running key bindings, so should it too be filtering eaten keys?

Actually, should focus_in be running key bindings at all?


Thanks,
pq
Giulio Camuffo
2014-11-11 14:10:22 UTC
Permalink
Post by Pekka Paalanen
On Tue, 11 Nov 2014 11:23:40 +0200
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.
---
v4: - initialize and release the eaten_keys array
- check the eaten_keys array is empty before updating the keymap
(i'm am not completely sure this is correct to be honest, but i think so :) )
src/bindings.c | 7 ++++--
src/compositor.h | 3 ++-
src/input.c | 67 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 55 insertions(+), 22 deletions(-)
diff --git a/src/bindings.c b/src/bindings.c
index 5e24725..369c81a 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 eaten;
/* 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 27ea693..55f73db 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -473,6 +473,7 @@ struct weston_keyboard {
uint32_t grab_time;
struct wl_array keys;
+ struct wl_array eaten_keys;
struct {
uint32_t mods_depressed;
@@ -1144,7 +1145,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 b504d06..5cb2703 100644
--- a/src/input.c
+++ b/src/input.c
@@ -526,6 +526,7 @@ weston_keyboard_create(void)
wl_list_init(&keyboard->focus_resource_listener.link);
keyboard->focus_resource_listener.notify = keyboard_focus_resource_destroyed;
wl_array_init(&keyboard->keys);
+ wl_array_init(&keyboard->eaten_keys);
keyboard->default_grab.interface = &default_keyboard_grab_interface;
keyboard->default_grab.keyboard = keyboard;
keyboard->grab = &keyboard->default_grab;
@@ -552,6 +553,7 @@ weston_keyboard_destroy(struct weston_keyboard *keyboard)
#endif
wl_array_release(&keyboard->keys);
+ wl_array_release(&keyboard->eaten_keys);
wl_list_remove(&keyboard->focus_resource_listener.link);
free(keyboard);
}
@@ -1307,6 +1309,26 @@ update_keymap(struct weston_seat *seat)
}
#endif
+/* remove the key from the array if it is being released,
+ * else return -1 and do nothing */
+static int
+remove_key(uint32_t key, enum wl_keyboard_key_state state,
+ struct wl_array *array)
+{
+ uint32_t *k, *end;
+ end = array->data + array->size;
+ for (k = array->data; k < end; k++) {
+ if (*k == key) {
+ /* Ignore server-generated repeats. */
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
+ return -1;
+ *k = *--end;
+ }
+ }
+ array->size = (void *) end - array->data;
+ return 0;
+}
+
WL_EXPORT void
notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
enum wl_keyboard_key_state state,
@@ -1315,7 +1337,9 @@ notify_key(struct weston_seat *seat, uint32_t time, uint32_t key,
struct weston_compositor *compositor = seat->compositor;
struct weston_keyboard *keyboard = seat->keyboard;
struct weston_keyboard_grab *grab = keyboard->grab;
- uint32_t *k, *end;
+ uint32_t *k;
+ int eaten = 0;
+ struct wl_array *array;
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
weston_compositor_idle_inhibit(compositor);
@@ -1325,32 +1349,31 @@ 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;
- }
+ /* Ignore server-generated repeats, so return if remove_key()
+ * returns -1, signaling the key is in the array already. */
+ if (remove_key(key, state, &keyboard->keys) == -1)
+ return;
+ if (remove_key(key, state, &keyboard->eaten_keys) == -1)
+ return;
if (grab == &keyboard->default_grab ||
grab == &keyboard->input_method_grab) {
- weston_compositor_run_key_binding(compositor, seat, time, key,
- state);
+ eaten = weston_compositor_run_key_binding(compositor, seat,
+ time, key, state);
grab = keyboard->grab;
}
+ array = eaten == 0 ? &keyboard->keys : &keyboard->eaten_keys;
+ if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+ k = wl_array_add(array, sizeof *k);
+ *k = key;
+ }
+
grab->interface->key(grab, time, key, state);
if (keyboard->pending_keymap &&
- keyboard->keys.size == 0)
+ keyboard->keys.size == 0 &&
+ keyboard->eaten_keys.size == 0)
update_keymap(seat);
if (update_state == STATE_UPDATE_AUTOMATIC) {
@@ -1430,6 +1453,11 @@ notify_keyboard_focus_out(struct weston_seat *seat)
update_modifier_state(seat, serial, *k,
WL_KEYBOARD_KEY_STATE_RELEASED);
}
+ wl_array_for_each(k, &keyboard->eaten_keys) {
+ weston_compositor_idle_release(compositor);
+ /* No need to update the modifier state here, modifiers
+ * can't be in the eaten_keys array */
+ }
seat->modifier_state = 0;
@@ -2083,7 +2111,8 @@ weston_seat_update_keymap(struct weston_seat *seat, struct xkb_keymap *keymap)
xkb_keymap_unref(seat->keyboard->pending_keymap);
seat->keyboard->pending_keymap = xkb_keymap_ref(keymap);
- if (seat->keyboard->keys.size == 0)
+ if (seat->keyboard->keys.size == 0 &&
+ seat->keyboard->eaten_keys.size == 0)
update_keymap(seat);
#endif
}
Looks good enough to me, pushed.
I do wonder about notify_keyboard_focus_in(), though. That one is also
running key bindings, so should it too be filtering eaten keys?
Actually, should focus_in be running key bindings at all?
I've been wondering that too, I don't think it makes much sense. I can
make a follow up patch if we have an agreement.


Thanks,
Giulio
Post by Pekka Paalanen
Thanks,
pq
Pekka Paalanen
2014-11-12 11:18:21 UTC
Permalink
On Tue, 11 Nov 2014 16:10:22 +0200
Post by Giulio Camuffo
Post by Pekka Paalanen
On Tue, 11 Nov 2014 11:23:40 +0200
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.
---
v4: - initialize and release the eaten_keys array
- check the eaten_keys array is empty before updating the keymap
(i'm am not completely sure this is correct to be honest, but i think so :) )
src/bindings.c | 7 ++++--
src/compositor.h | 3 ++-
src/input.c | 67 ++++++++++++++++++++++++++++++++++++++++----------------
3 files changed, 55 insertions(+), 22 deletions(-)
...
Post by Giulio Camuffo
Post by Pekka Paalanen
I do wonder about notify_keyboard_focus_in(), though. That one is also
running key bindings, so should it too be filtering eaten keys?
Actually, should focus_in be running key bindings at all?
I've been wondering that too, I don't think it makes much sense. I can
make a follow up patch if we have an agreement.
I seem to have an agreement:

< pq> I do wonder if notify_keyboard_focus_in() running the keybindings is a sane thing to do...
< daniels> pq: no, i don't think it is
< daniels> pq: update modifiers definitely, running bindings no

Go for it. :-)


Thanks,
pq
Continue reading on narkive:
Loading...