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

[Daniel Stone]

Hi,
Sorry for the long absence from this thread.

On 12 November 2014 20:06, Bill Spitzak <spitzak at gmail.com> wrote:

> On 11/12/2014 03:15 AM, Pekka Paalanen wrote:
>>
>> 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
> 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.


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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141113/d520bb57/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-XKB-Split-filter-execution-into-a-separate-function.patch
Type: text/x-patch
Size: 7254 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141113/d520bb57/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Input-Add-KeyFocusIn-internal-event.patch
Type: text/x-patch
Size: 10225 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141113/d520bb57/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-XWayland-Use-FocusIn-events-for-keyboard-enter.patch
Type: text/x-patch
Size: 1742 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141113/d520bb57/attachment-0005.bin>