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

[Peter Hutterer]

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.

Cheers,
   Peter