[xserver] xwayland: Process queued events before making wayland mods effective

Since xwayland's initial commit we have had a check to not process
wayland modifier events while one of our surfaces has keyboard focus
since the normal xkb event processing keeps our internal modifier
state up to date and if we use the modifiers we get from the
compositor we mess up that state.

This was slightly changed in commit
10e9116b3f709bec6d6a50446c1341441a0564e4 to allow the xkb group to be
set from the wayland event while we have focus in case the compositor
triggers a group switch.

There's a better solution to the original problem though. Processing
queued events before overriding the xkb state with the compositor's
allows those events to be sent properly modified to X clients while
any further events will be modified with the wayland modifiers as
intended.

This allows us to fully take in the wayland modifiers, including
depressed ones, which fixes an issue where we wouldn't be aware of
already pressed modifiers on enter.

Signed-off-by: Rui Matos <tiagomatos@gmail.com>
---

Fixes https://bugzilla.gnome.org/show_bug.cgi?id=767475 . I'm not 100%
sure there aren't unintended side-effects but it seems to be working
fine in actual use for a couple of weeks.

 hw/xwayland/xwayland-input.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Hi

----- Original Message -----
> Since xwayland's initial commit we have had a check to not process
> wayland modifier events while one of our surfaces has keyboard focus
> since the normal xkb event processing keeps our internal modifier
> state up to date and if we use the modifiers we get from the
> compositor we mess up that state.
> 
> This was slightly changed in commit
> 10e9116b3f709bec6d6a50446c1341441a0564e4 to allow the xkb group to be
> set from the wayland event while we have focus in case the compositor
> triggers a group switch.
> 
> There's a better solution to the original problem though. Processing
> queued events before overriding the xkb state with the compositor's
> allows those events to be sent properly modified to X clients while
> any further events will be modified with the wayland modifiers as
> intended.
> 
> This allows us to fully take in the wayland modifiers, including
> depressed ones, which fixes an issue where we wouldn't be aware of
> already pressed modifiers on enter.
> 
> Signed-off-by: Rui Matos <tiagomatos@gmail.com>
> ---
> 
> Fixes https://bugzilla.gnome.org/show_bug.cgi?id=767475 . I'm not 100%
> sure there aren't unintended side-effects but it seems to be working
> fine in actual use for a couple of weeks.
> 
>  hw/xwayland/xwayland-input.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 4642efe..7aae7ba 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -503,6 +503,8 @@ keyboard_handle_modifiers(void *data, struct wl_keyboard
> *keyboard,
>      xkbStateNotify sn;
>      CARD16 changed;
>  
> +    mieqProcessInputEvents();
> +
>      for (dev = inputInfo.devices; dev; dev = dev->next) {
>          if (dev != xwl_seat->keyboard &&
>              dev != GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD))
> @@ -511,12 +513,11 @@ 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);
> -        }
>          new_state->locked_group = group & XkbAllGroupsMask;
> +        new_state->base_mods = mods_depressed & XkbAllModifiersMask;
> +        new_state->locked_mods = mods_locked & XkbAllModifiersMask;
> +        XkbLatchModifiers(dev, XkbAllModifiersMask,
> +                          mods_latched & XkbAllModifiersMask);
>  
>          XkbComputeDerivedState(dev->key->xkbInfo);
>  

FWIW I have been using this patch for a few weeks and it works fine as far as I can tell, and it doesn't seem to have any ill effect on Weston either, so:

Tested-by: Olivier Fourdan <ofourdan@redhat.com>

Cheers,
Olivier

Hi,

On 27 June 2016 at 03:48, Rui Matos <tiagomatos@gmail.com> wrote:
> Since xwayland's initial commit we have had a check to not process
> wayland modifier events while one of our surfaces has keyboard focus
> since the normal xkb event processing keeps our internal modifier
> state up to date and if we use the modifiers we get from the
> compositor we mess up that state.
>
> This was slightly changed in commit
> 10e9116b3f709bec6d6a50446c1341441a0564e4 to allow the xkb group to be
> set from the wayland event while we have focus in case the compositor
> triggers a group switch.
>
> There's a better solution to the original problem though. Processing
> queued events before overriding the xkb state with the compositor's
> allows those events to be sent properly modified to X clients while
> any further events will be modified with the wayland modifiers as
> intended.
>
> This allows us to fully take in the wayland modifiers, including
> depressed ones, which fixes an issue where we wouldn't be aware of
> already pressed modifiers on enter.

This is a good solution, though a better solution would arguably be to
add an event we send down the mi event queue, which carried the
modifier information and was processed with that. But, this works, so:
Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

It seems this patch has fallen through the cracks, can someone push it please?

Rui

On Mon, Jun 27, 2016 at 9:55 AM, Daniel Stone <daniel@fooishbar.org> wrote:
> This is a good solution, though a better solution would arguably be to
> add an event we send down the mi event queue, which carried the
> modifier information and was processed with that. But, this works, so:
> Reviewed-by: Daniel Stone <daniels@collabora.com>