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

[Pekka Paalanen]

On Mon, 17 Nov 2014 17:54:33 +1000
Peter Hutterer <peter.hutterer at who-t.net> wrote:

> On Thu, Nov 13, 2014 at 10:06:27AM +0000, Daniel Stone wrote:
> > 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
> > > <javascript:;>>:
> > > > 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.
> > 
> > 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 <pekka.paalanen at 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