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

[Daniel Stone]

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.


> 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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20141125/54e41a13/attachment.html>