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

Submitted by Victor Toso on April 9, 2019, 8:12 a.m.

Details

Message ID 20190409081225.1037-1-victortoso@redhat.com
State New
Headers show
Series "gtk-session: do not sync modifiers when focused" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso April 9, 2019, 8:12 a.m.
From: Olivier Fourdan <ofourdan@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@redhat.com>
Signed-off-by: Olivier Fourdan <ofourdan@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(+)

Patch hide | download patch | download mbox

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;
+
     spice_gtk_session_sync_keyboard_modifiers(self);
 }
 

Comments


> 
> Hi,
> 
> On Tue, Apr 09, 2019 at 08:12:25AM +0000, Victor Toso wrote:
> > From: Olivier Fourdan <ofourdan@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@redhat.com>
> > Signed-off-by: Olivier Fourdan <ofourdan@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@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