On 29/11/2014 03:09 , Daniel Stone wrote:
Hi,
On 28 November 2014 at 06:09, Peter Hutterer <[email protected]
<mailto:[email protected]>> wrote:
On Tue, Nov 25, 2014 at 09:30:59PM +0000, Daniel Stone wrote:
> On 25 November 2014 at 02:38, Keith Packard <[email protected]
<mailto:[email protected]>> 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
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
