[Spice-devel] 0001-a patch for syncing keyboard lock status

[Jonathon Jongsma]

Hello,

Thanks for the patch.  First, a general comment: it's helpful if you
write a few more details in the commit message. I can't tell exactly
what bug you are trying to fix, what conditions trigger the bug, or what
the expected behavior is.  Knowing these things will make it easier to
review the patch.

More detailed comments below.


On Wed, 2014-03-26 at 23:10 +0800, bigclouds wrote:


> From cdfcb3083825836e69f3e8d9ef18323117e43015 Mon Sep 17 00:00:00 2001
> From: "longguang.yue" <longguang.yue at gmail.com>
> Date: Wed, 26 Mar 2014 22:28:55 +0800
> Subject: [PATCH] there is only few oppotunity (one or two) to sync keybloard
>  lock status, in case lock status is not corrected after
>  that it will never be synced if window is maximized
> 
> ---
>  gtk/channel-inputs.c | 15 ++++++++++++++-
>  gtk/channel-inputs.h |  1 +
>  gtk/spice-widget.c   | 28 +++++++++++++++++++++++++++-
>  3 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/gtk/channel-inputs.c b/gtk/channel-inputs.c
> index 8a726e0..1a3e584 100644
> --- a/gtk/channel-inputs.c
> +++ b/gtk/channel-inputs.c
> @@ -63,7 +63,7 @@ enum {
>  /* Signals */
>  enum {
>      SPICE_INPUTS_MODIFIERS,
> -
> +	SPICE_DISPLAY_MODIFIER_SYNC,
>      SPICE_INPUTS_LAST_SIGNAL,
>  };
>  
> @@ -142,6 +142,17 @@ static void spice_inputs_channel_class_init(SpiceInputsChannelClass *klass)
>                       g_cclosure_marshal_VOID__VOID,
>                       G_TYPE_NONE,
>                       0);
> +	/*sync led status in case they are not identical after channel connection*/
> +	signals[SPICE_DISPLAY_MODIFIER_SYNC] =
> +         g_signal_new("modifier-sync",
> +                      G_OBJECT_CLASS_TYPE(gobject_class),
> +                      G_SIGNAL_RUN_FIRST|G_SIGNAL_ACTION,
> +                      G_STRUCT_OFFSET(SpiceInputsChannelClass, modifier_sync),
> +                      NULL, NULL,
> +                      g_cclosure_marshal_VOID__UINT,
> +                      G_TYPE_NONE,
> +                      1,
> +                      G_TYPE_INT);
>  
>      g_type_class_add_private(klass, sizeof(SpiceInputsChannelPrivate));
>      channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
> @@ -262,6 +273,8 @@ static void inputs_handle_modifiers(SpiceChannel *channel, SpiceMsgIn *in)
>  
>      c->modifiers = modifiers->modifiers;
>      emit_main_context(channel, SPICE_INPUTS_MODIFIERS);
> +	SPICE_DEBUG("get modifier key=0x%x, channelObject=%p", modifiers->modifiers, channel);

Incorrect indentation: use spaces instead of tabs

> +    g_signal_emit(channel, signals[SPICE_DISPLAY_MODIFIER_SYNC], 0, modifiers->modifiers);

This code is running within a coroutine context, and emitting the
signal within the coroutine context is not necessarily safe. Note that
just before your additions, we emit another signal, but we emit it in
the main context (by calling emit_main_context()) rather than within
the coroutine context. 

But more fundamentally, I'm struggling to see why this new signal is
necessary, since we're already emitting the 'inputs-modifiers' signal in
the exact same place.  Why couldn't you simply use that signal instead?


> 
>  }
>  
>  /* coroutine context */
> diff --git a/gtk/channel-inputs.h b/gtk/channel-inputs.h
> index 3179a76..83fce65 100644
> --- a/gtk/channel-inputs.h
> +++ b/gtk/channel-inputs.h
> @@ -64,6 +64,7 @@ struct _SpiceInputsChannelClass {
>  
>      /* signals */
>      void (*inputs_modifiers)(SpiceChannel *channel);
> +	void (*modifier_sync)(SpiceChannel *channel, uint32_t v);
> 
> 

improper indentation again, but this function pointer is probably not
necessary as mentioned above.

>  
>      /*< private >*/
>      /* Do not add fields to this struct */
> diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> index 0e4a0de..40dfbfb 100644
> --- a/gtk/spice-widget.c
> +++ b/gtk/spice-widget.c
> @@ -135,7 +135,7 @@ static void sync_keyboard_lock_modifiers(SpiceDisplay *display);
>  static void cursor_invalidate(SpiceDisplay *display);
>  static void update_area(SpiceDisplay *display, gint x, gint y, gint width, gint height);
>  static void release_keys(SpiceDisplay *display);
> -
> +static void modifier_sync(SpiceChannel *channel, uint32_t  v, SpiceDisplay *display);
>  /* ---------------------------------------------------------------- */
>  
>  static void spice_display_get_property(GObject    *object,
> @@ -2410,6 +2410,7 @@ static void channel_new(SpiceSession *s, SpiceChannel *channel, gpointer data)
>  
>      if (SPICE_IS_INPUTS_CHANNEL(channel)) {
>          d->inputs = SPICE_INPUTS_CHANNEL(channel);
> +		g_signal_connect(channel, "modifier-sync", G_CALLBACK(modifier_sync), display);

So what you're doing here is connecting to a signal that is emitted
whenever the guest reports that its modifiers have changed. In response
to this signal, you call a function which then resets the modifiers of
the guest to be equal to the modifiers on the host. Is that what you
intended?


> 
>          spice_channel_connect(channel);
>          sync_keyboard_lock_modifiers(display);
>          return;
> @@ -2686,3 +2687,28 @@ static void sync_keyboard_lock_modifiers(SpiceDisplay *display)
>      g_warning("sync_keyboard_lock_modifiers not implemented");
>  }
>  #endif // HAVE_X11_XKBLIB_H
> +
> +static void modifier_sync(SpiceChannel *channel, uint32_t  v, SpiceDisplay *display)
> +{
> +    Display *x_display;
> +    SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> +    guint32 modifiers;
> +    GdkWindow *w;
> + 
> +    if (d->disable_inputs)
> +        return;
> + 
> +    w = gtk_widget_get_parent_window(GTK_WIDGET(display));
> +    if (w == NULL) /* it can happen if the display is not yet shown */
> +        return;
> + 
> +    x_display = GDK_WINDOW_XDISPLAY(w);
> +    #if HAVE_X11_XKBLIB_H
> +        modifiers = get_keyboard_lock_modifiers(x_display);
> +    #elif defined (WIN32)
> +        modifiers = get_keyboard_lock_modifiers();
> +    #endif
> +    SPICE_DEBUG("modifier_sync 0x%x, 0x%x", modifiers, v);
> +    if(modifiers != v && d->inputs)
> +      spice_inputs_set_key_locks(d->inputs, modifiers);
> +}
> \ No newline at end of file
> -- 
> 1.7.11.msysgit.1


This function looks almost identical to sync_keyboard_lock_modifiers(),
the main difference is that it won't work on windows (e.g. Display*
doesn't exist, etc).  Is there a reason that you didn't simply use the
existing function instead?

Jonathon