[PATCH 3/3] XWayland: Use FocusIn events for keyboard enter

[Peter Hutterer]

On 29/11/2014 03:09 , Daniel Stone wrote:
> Hi,
>
> On 28 November 2014 at 06:09, Peter Hutterer <peter.hutterer at who-t.net
> <mailto:peter.hutterer at who-t.net>> wrote:
>
>     On Tue, Nov 25, 2014 at 09:30:59PM +0000, Daniel Stone wrote:
>      > On 25 November 2014 at 02:38, Keith Packard <keithp at keithp.com
>     <mailto:keithp at keithp.com>> wrote:
>      > > Yeah, I'm fine with figuring out how to make it perfect; if the
>     above
>      > > patch will suffice to get modifiers handled correctly for now,
>     I'd like
>      > > to go with that for this release and pend the perfect solution
>     until
>      > > after 1.17; just too many changes outside of hw/xwayland to make me
>      > > happy at this point in the release cycle.
>      >
>      > I think they're all pretty harmless as they're confined to new
>     codepaths
>      > which were not previously triggerable, and are still not triggerable
>      > outside of Xwayland.
>
>     that's the theory :) if you forgot to add one check somewhere, you
>     may now
>     get weird events. I can't seem to find a 1.17 release schedule - Keith,
>     where're we at?
>
>
> Sure, but the same is true of the suggested global state. Forget to
> check it and you've got a bug.

yeah, I'm not a fan of the global state either.

>     > > In particular, I can imagine passing a new boolean in, or setting some
>     > > device state, that muted event generation and avoided having to check
>     > > for ET_KeyFocusIn in a bunch of places now and in the future. That would
>     > > also encourage us to actually deal with this case in Xephyr and Xnest.
>     >
>     > How is that better? You've shifted some local state (what's the event
>     > type?) into global state (is this thing set?). The penalties for missing a
>     > check are exactly the same for both, except the intention is much more
>     > obvious with an explicit event. The event type also has the benefit of
>     > being able to be posted through the queue, rather than OsBlockSignals ->
>     > ProcessInputEvents -> mangle magic global flag -> QueueKeyboardEvents ->
>     > ProcessKeyboardEvents -> OsUnblockSignals. I'd much rather take the hit of
>     > tiny extra complexity in the core (really, it's not much at all) rather
>     > than having all the users get it wrong, or simply not bother because it's
>     > too hard. Much like they do today.
>
>     You could add that state to the internal event, add a fake_event
>     field to
>     the DeviceEvent and check that where necessary, otherwise let it pass
>     through as normal event. That way the penalty for forgetting a check
>     only
>     matters where you actually care about it.
>
>     That should also reduce the risk of getting odd events.
>
>
> So to be clear, it would be ET_KeyPress, but DeviceEvent grows an extra
> field that tells you that really it should be suppressed?

correct

> I don't think that makes too much difference from above (again, if you
> miss a check, you're sending something out incorrectly; it also makes it
> less easy to tell at a glance what's going on), but if that's what you'd
> prefer, then fair enough.

It'll give us odd key events in the bad case, but it reduces the 
maintenance burden since we only need to check for the field where we 
care about it. All the other changes in your patch with if type == 
ET_KeyPress || type == ET_FocusIn would go away, leaving only two or 
three instances where we need that extra check. I think that alone 
should be worth it.

Cheers,
   Peter

>
>      > If you're implacably opposed, then I'll ack the minimal change
>     (once we can
>      > be certain that the focus is indeed NULL whilst posting those
>     events) for
>      > 1.17, but I'll also request a revert immediately after master
>     reopens, and
>      > resubmit this patchset. Your patch does just happen to work, but
>     it's an
>      > extra whack of magic bonghits right where we least need them.
>      >
>      > Cheers,
>      > Daniel
>
>