[PATCH] xwayland-input: Always set the xkb group index on modifiers events

[Marek Chalupa]

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: <1405432640-16113-1-git-send-email-tiagomatos at gmail.com>
 >
 > 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 <mchqwerty at gmail.com>

 >          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: <1405432640-16113-1-git-send-email-tiagomatos at gmail.com>
 > References: <1405432640-16113-1-git-send-email-tiagomatos at gmail.com>
 > Message-ID: 
<CAPj87rO8OvpgzF=Vsp9CtcALSLZJ-ONEpFbeC=jRT7HqQAq6zg at 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