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

[Peter Hutterer]

On Tue, Nov 25, 2014 at 09:30:59PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 25 November 2014 at 02:38, Keith Packard <keithp at keithp.com> wrote:
> 
> > Daniel Stone <daniel at fooishbar.org> writes:
> > > Because that's still a bunch of work,
> >
> > I think it's just:
> >
> > -    wl_array_for_each(k, &xwl_seat->keys)
> > -        QueueKeyboardEvents(xwl_seat->keyboard, KeyPress, *k + 8, &mask);
> > +    wl_array_for_each(k, &xwl_seat->keys) {
> > +        if (xwl_seat->keyboard->key->xkbInfo->desc->map->modmap[*k+8] !=
> > 0)
> > +            QueueKeyboardEvents(xwl_seat->keyboard, KeyPress, *k + 8,
> > &mask);
> > +    }
> >  }
> >
> 
> Is it? You have to make sure to the focus is NULL at this point, otherwise
> you send surprise events to windows that don't want it. And if the focus is
> NULL, it has pretty much the same effect as my patch; it's just that it's
> stripped of context when you're trying to debug things at lower levels.
> 
> Think of the case where you enter with Alt down. If the user presses F, you
> want the file menu to trigger, but when the user releases Alt without
> pressing anything, you don't want to activate the menubar.
> 
> I can also see the case for passing all keys along on enter: after all,
> that's exactly what KeymapNotify does. If you think this is fine, would you
> also accept a patch to do this on every focus change inside normal X11? And
> if not, why not ... ?
> 
> 
> > > This patch isn't the end of the road; in the end, it should be
> > > entirely possible to achieve perfect nesting wrt KeymapNotify events
> > > with XWayland, Xephyr, XQuartz, and any combination thereof. This is a
> > > relatively start in that direction.
> >
> > 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?
 
> > 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.

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