Hey,
ping
(not sure if in-reply-to will work, so here's a link too
http://lists.x.org/archives/xorg-devel/2014-July/043080.html)
I pinged this patch for two reasons:
1) it fixes a bug https://bugzilla.gnome.org/show_bug.cgi?id=752165
2) [for wayland-devel] The order of modifier event and the key press is
not still settled in the protocol (or I can't find it anywhere). If it
is, sorry for spam. (I found it only in the commit message that
introduced the event
http://cgit.freedesktop.org/wayland/wayland/commit/?id=9a1705c5f5e877d4e68bd0e7eb858f517375ba3f
)
comment for the patch below
Regards,
Marek
> From tiagomatos at gmail.com Tue Jul 15 06:57:20 2014
> From: tiagomatos at gmail.com (Rui Matos)
> Date: Tue, 15 Jul 2014 15:57:20 +0200
> Subject: [PATCH] xwayland-input: Always set the xkb group index on
modifiers
> events
> Message-ID: <[email protected]>
>
> While we have keyboard focus, the server's xkb code is already locking
> and latching modifiers appropriately while processing keyboard
> events.
>
> Since there is no guaranteed order between wl_keyboard key and
> modifiers events, if we got the modifiers event with a locked or
> latched modifier and then process the key press event for that
> modifier we would wrongly unlock/unlatch. To prevent this, we ignore
> locked and latched modifiers while any of our surfaces has keyboard
> focus.
>
> But we always need to set the xkb group index since this might be
> triggered programatically by the wayland compositor at any time.
> ---
>
> Note that xwayland's xkb state handling needs quite a bit of work to
> make it reliable. Basically, if the wl_keyboard.modifiers event comes
> in after the wl_keyboard.enter event we won't update the locked and
> latched modifiers properly. Right now this just happens to work with
> mutter.
>
> I'm working on a proper fix for this which requires API changes to the
> core xkb code in order for us to be able to tell it to ignore modifier
> state processing on key events and instead always set that state from
> wl_keyboard.modifiers events like any other wayland client.
>
> Extra API will also be needed to tell the xkb request processing code
> to return errors and/or silently drop X client requests that try to
> change any xkb state and keymaps.
>
> Anyway, that wouldn't be acceptable for the upcoming 1.16 release so
> I'm just proposing this targetted fix for the group index to be
> honored which I'd like to have working on the next GNOME release.
>
> hw/xwayland/xwayland-input.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 990cb82..9bdf9c3 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -420,12 +420,6 @@ keyboard_handle_modifiers(void *data, struct
wl_keyboard *keyboard,
> xkbStateNotify sn;
> CARD16 changed;
>
> - /* We don't need any of this while we have keyboard focus since
> - the regular key event processing already takes care of setting
> - our internal state correctly. */
> - if (xwl_seat->keyboard_focus)
> - return;
> -
> for (dev = inputInfo.devices; dev; dev = dev->next) {
> if (dev != xwl_seat->keyboard &&
> dev != GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD))
> @@ -434,10 +428,12 @@ keyboard_handle_modifiers(void *data, struct
wl_keyboard *keyboard,
> old_state = dev->key->xkbInfo->state;
> new_state = &dev->key->xkbInfo->state;
>
> + if (!xwl_seat->keyboard_focus) {
> + new_state->locked_mods = mods_locked & XkbAllModifiersMask;
> + XkbLatchModifiers(dev, XkbAllModifiersMask,
> + mods_latched & XkbAllModifiersMask);
> + }
I believe here it would deserve a comment why we care about the focus
and why the group is updated unconditionally.
Reviewed-by: Marek Chalupa <[email protected]>
> new_state->locked_group = group & XkbAllGroupsMask;
> - new_state->locked_mods = mods_locked & XkbAllModifiersMask;
> - XkbLatchModifiers(dev, XkbAllModifiersMask,
> - mods_latched & XkbAllModifiersMask);
>
> XkbComputeDerivedState(dev->key->xkbInfo);
>
> --
> 1.9.0
-------------------------------------------------------------------------
> From daniel at fooishbar.org Fri Jul 18 05:42:48 2014
> From: daniel at fooishbar.org (Daniel Stone)
> Date: Fri, 18 Jul 2014 13:42:48 +0100
> Subject: [PATCH] xwayland-input: Always set the xkb group index on
modifiers events
> In-Reply-To: <[email protected]>
> References: <[email protected]>
> Message-ID:
<CAPj87rO8OvpgzF=Vsp9CtcALSLZJ-ONEpFbeC=jrt7hqqaq...@mail.gmail.com>
>
> Hi,
>
> On 15 July 2014 14:57, Rui Matos <tiagomatos at gmail.com> wrote:
>
> > While we have keyboard focus, the server's xkb code is already locking
> > and latching modifiers appropriately while processing keyboard
> > events.
> >
> > Since there is no guaranteed order between wl_keyboard key and
> > modifiers events, if we got the modifiers event with a locked or
> > latched modifier and then process the key press event for that
> > modifier we would wrongly unlock/unlatch. To prevent this, we ignore
> > locked and latched modifiers while any of our surfaces has keyboard
> > focus.
> >
>
> Wow, I really wish I'd encoded that properly in the wl_keyboard spec.
>
> Sending modifiers before key is _seriously_ broken, and no-one should
ever
> do it. There are corner cases it completely and horrifically breaks.
>
> X11 actually got this right for once: the state sent with the key
event is
> the state that applied _immediately before the press_, i.e. unaffected by
> the press itself. Where it slipped up was not updating you with the state
> immediately after the press as well, but you cannot interpret the press
> with the state the press itself created.
>
> I'm very much tempted to merge Jonny's repeat rate patches which take
us to
> a new wl_keyboard version, and including in that version a
requirement that
> this ordering be observed.
>
> If Mutter sends modifiers before key, _please_ reverse that.
>
>
> > But we always need to set the xkb group index since this might be
> > triggered programatically by the wayland compositor at any time.
> > ---
> >
> > Note that xwayland's xkb state handling needs quite a bit of work to
> > make it reliable. Basically, if the wl_keyboard.modifiers event comes
> > in after the wl_keyboard.enter event we won't update the locked and
> > latched modifiers properly. Right now this just happens to work with
> > mutter.
> >
> > I'm working on a proper fix for this which requires API changes to the
> > core xkb code in order for us to be able to tell it to ignore modifier
> > state processing on key events and instead always set that state from
> > wl_keyboard.modifiers events like any other wayland client.
> >
> > Extra API will also be needed to tell the xkb request processing code
> > to return errors and/or silently drop X client requests that try to
> > change any xkb state and keymaps.
> >
> > Anyway, that wouldn't be acceptable for the upcoming 1.16 release so
> > I'm just proposing this targetted fix for the group index to be
> > honored which I'd like to have working on the next GNOME release.
> >
>
> Agreed to all of the above, but, wouldn't we be better off waiting for a
> modifiers event straight after enter? Although that's another thing which
> really needs encoding into the protocol ... oh well.
>
> Reviewed-by: Daniel Stone <daniels at collabora.com>
>
> Cheers,
> Daniel
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
