[Spice-devel] [spice-gtk] gtk-session: do not sync modifiers when focused

[Frediano Ziglio]

> 
> Hi,
> 
> On Tue, Apr 09, 2019 at 08:12:25AM +0000, Victor Toso wrote:
> > From: Olivier Fourdan <ofourdan at redhat.com>
> > 
> > Spice gtk-session would try to synchronize the modifiers state whenever
> > the keymap changes, but doing so is inherently racy.
> > 
> > While the there is a keyboard grab in effect, all key events are
> > forwarded to the guest, hence all modifiers key press get processed by
> > the kernel on the guest.
> > 
> > Trying to synchronize the modifiers will generate additional key press/
> > release events which will result in the opposite effect and effectively
> > desynchronize the modifier states in the guest.
> > 
> > Synchronizing modifiers from the host should therefore be limited to
> > focus change, as actual press/release events might have occurred without
> > the guest knowing. Otherwise, no need to synchronize the modifiers.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires at redhat.com>
> > Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> > ---
> > 
> > Merge request spice-gtk!6 or
> >     https://gitlab.freedesktop.org/spice/spice-gtk/merge_requests/6
> > 
> >  src/spice-gtk-session.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..65442b2 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -163,6 +163,13 @@ static void keymap_modifiers_changed(GdkKeymap
> > *keymap, gpointer data)
> >  {
> >      SpiceGtkSession *self = data;
> >  
> > +    /* set_key_locks() is inherently racy, but no need to sync modifiers
> > +     * if we have focus as the regular keypress/keyrelease will have set
> > +     * the expected modifiers state in the guest.
> > +     */
> > +    if (self->priv->keyboard_has_focus)
> > +      return;
> > +
> 
> Doing a quick test, one can see that on keyboard_focus, before
> this patch we had (NumLock on Wayland)
> 
> | spice-gtk-session.c:157 inputs-3:0: client_modifiers:0x2,
> | guest_modifiers:0x2
> | spice-widget.c:1646 0:0 key_event press: keycode: 77  state: 16  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode:  scancode 69
> | spice-gtk-session.c:157 inputs-3:0: client_modifiers:0x0,
> | guest_modifiers:0x2
> | spice-widget.c:1646 0:0 key_event release: keycode: 77  state: 16  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode: release scancode 69
> 
> and
> 
> | spice-gtk-session.c:157 inputs-3:0: client_modifiers:0x2,
> | guest_modifiers:0x0
> | spice-widget.c:1646 0:0 key_event press: keycode: 77  state: 0  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode:  scancode 69
> | spice-gtk-session.c:157 inputs-3:0: client_modifiers:0x2,
> | guest_modifiers:0x2
> | spice-widget.c:1646 0:0 key_event release: keycode: 77  state: 16  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode: release scancode 69
> | spice-gtk-session.c:157 inputs-3:0: client_modifiers:0x2,
> | guest_modifiers:0x0
> 
> With this patch, becomes:
> 
> | spice-widget.c:1644 0:0 key_event press: keycode: 77  state: 16  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode:  scancode 69
> | spice-widget.c:1644 0:0 key_event release: keycode: 77  state: 16  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode: release scancode 69
> 
> and
> 
> | spice-widget.c:1644 0:0 key_event press: keycode: 77  state: 0  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode:  scancode 69
> | spice-widget.c:1644 0:0 key_event release: keycode: 77  state: 16  group 0
> | modifier 1
> | spice-util.c:270 spice_make_scancode: release scancode 69
> 
> So, keypress alone is enough indeed when we have focus. Without focus,
> this is still needed to keep the modifiers synced.
> 
> Many thanks for the investigation, explanation and fixing it!
> 
> Acked-by: Victor Toso <victortoso at redhat.com>
> 

I agree on the patch however to me it looks partial.

When the server send the modifiers the client check its state
and still does the sync.
In my configuration my client does not change the caps lock state
as the caps is used for other purposes. The caps in my case get
ignored or it bounce back depending on how long I press the caps.

Frediano