[Spice-devel] [PATCH spice-gtk 2/2] gtk-session: always sync modifiers for client events

[Jonathon Jongsma]

On Fri, 2014-04-04 at 12:47 +0200, Marc-André Lureau wrote:
> The channel state is not synchronous.
> 
> It may happen that we want to set and unset quickly a modifier, but the
> guest modifier state hasn't been updated yet, and will still be seen as
> unset, preventing the last unset change.
> ---
>  gtk/spice-gtk-session.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/gtk/spice-gtk-session.c b/gtk/spice-gtk-session.c
> index 922614d..9e74cbc 100644
> --- a/gtk/spice-gtk-session.c
> +++ b/gtk/spice-gtk-session.c
> @@ -156,32 +156,35 @@ static guint32 get_keyboard_lock_modifiers(void)
>  }
>  
>  static void spice_gtk_session_sync_keyboard_modifiers_for_channel(SpiceGtkSession *self,
> -                                                                  SpiceInputsChannel* inputs)
> +                                                                  SpiceInputsChannel* inputs,
> +                                                                  gboolean force)
>  {
>      gint guest_modifiers = 0, client_modifiers = 0;
>  
>      g_return_if_fail(SPICE_IS_INPUTS_CHANNEL(inputs));
>  
>      g_object_get(inputs, "key-modifiers", &guest_modifiers, NULL);
> -
>      client_modifiers = get_keyboard_lock_modifiers();
> -    CHANNEL_DEBUG(inputs, "client_modifiers:0x%x, guest_modifiers:0x%x",
> -                  client_modifiers, guest_modifiers);
>  
> -    if (client_modifiers != guest_modifiers)
> +    if (force || client_modifiers != guest_modifiers) {
> +        CHANNEL_DEBUG(inputs, "client_modifiers:0x%x, guest_modifiers:0x%x",
> +                      client_modifiers, guest_modifiers);
>          spice_inputs_set_key_locks(inputs, client_modifiers);
> +    }
>  }
>  
>  static void keymap_modifiers_changed(GdkKeymap *keymap, gpointer data)
>  {
>      SpiceGtkSession *self = data;
> +
>      spice_gtk_session_sync_keyboard_modifiers(self);
>  }
>  
>  static void guest_modifiers_changed(SpiceInputsChannel *inputs, gpointer data)
>  {
>      SpiceGtkSession *self = data;
> -    spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs);
> +
> +    spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs, FALSE);
>  }
>  
>  static void spice_gtk_session_init(SpiceGtkSession *self)
> @@ -965,7 +968,7 @@ static void channel_new(SpiceSession *session, SpiceChannel *channel,
>      if (SPICE_IS_INPUTS_CHANNEL(channel)) {
>          spice_g_signal_connect_object(channel, "inputs-modifiers",
>                                        G_CALLBACK(guest_modifiers_changed), self, 0);
> -        spice_gtk_session_sync_keyboard_modifiers_for_channel(self, SPICE_INPUTS_CHANNEL(channel));
> +        spice_gtk_session_sync_keyboard_modifiers_for_channel(self, SPICE_INPUTS_CHANNEL(channel), TRUE);
>      }
>  }
>  
> @@ -1132,7 +1135,7 @@ void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self)
>      for (l = channels; l != NULL; l = l->next) {
>          if (SPICE_IS_INPUTS_CHANNEL(l->data)) {
>              SpiceInputsChannel *inputs = SPICE_INPUTS_CHANNEL(l->data);
> -            spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs);
> +            spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs, TRUE);
>          }
>      }
>  }


Hmm, I'm not sure about this one. The problem is that
GdkKeymap::state-changed will fire whenever *any* modifier is changed,
even ones we don't care about (e.g. shift, ctrl, alt, etc).  This patch
makes us send down messages in response to all of those keypresses. That
doesn't seem like a great idea. But this prompted me to think about the
issue in a bit more depth.  Allow me to back up and explain a little
bit.

The root problem with the modifier synchronization was that there were
only a few discrete points that modifiers get synchronized:
 - when the channel is created
 - when the window is focused

The first patch of this series (building on longguang's original
suggestion) fixed the case where the guest changed the modifiers behind
our back. We now reset them back to match the client state by listening
for the 'inputs-modifiers' signal.

This second patch (the GdkKeymap::state-changed signal handler) was not
strictly related to the reported bug, but it seemed to be a cleaner way
to handle synchronization on the client side. Instead of just
synchronizing at the discrete moment when the widget gets focus, we
synchronize whenever there is a change in the client modifier state.

However, I now wonder if there may be unintended consequences of adding
this feature. I didn't notice any problems while testing the patch, but
in thinking about it some more and doing a little testing, here are some
potential issues:

        - if the guest display widget is focused and the user presses
        Caps lock, we will send a keypress message (which will cause the
        capslock modifier to toggle on the guest) and then we will
        immediately send a modifers message. So we'll send duplicate
        messages for the same event. In practice I hadn't noticed any
        problems because of this, but I wonder if this is partly what
        caused the issue you observed?
        
        - if the capslock on the client is mapped to something else
        (e.g. 'control'), and the capslock key is pressed while the
        display widget is focused, we will send a keypress event to the
        guest, which will cause the guest's capslock state to change.
        However, the client's capslock state will not change, so when we
        receive the 'inputs-modifiers' message from the guest, we will
        send down the old value again.  On the other hand, this is a
        corner case because it requires a non-standard keyboard layout,
        and the alternative (not overriding the guest modifiers state)
        results in an out-of-sync modifiers state between the guest and
        client. So I'm not sure what the right answer is here.

thoughts?

Jonathon