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

[Giulio Camuffo]

2014-11-13 12:06 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:
> Hi,
>
> On Thursday, November 13, 2014, Giulio Camuffo <giuliocamuffo at gmail.com>
> wrote:
>>
>> 2014-11-13 11:18 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:
>> > 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.
>
>> > 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.
>
>>
>> > 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.

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

> 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