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

[Giulio Camuffo]

2014-11-13 11:18 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:
> 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.

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.

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

I perfectly agree XWayland is broken here. But i don't see how this
patch breaks weston.

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

> 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