Fair enough, my patch isn't exactly the same as yours, but sync_keyboard_lock_modifiers() could certainly be changed to check whether the keyboard modifiers had changed before sending down a new value.
But there's still a problem with this approach. There can be multiple SpiceDisplay widgets associated with each input channel object (and in virt-viewer, there are regularly 4 display objects associated with each input channel). So I don't think that the SpiceDisplay is the right object to be responsible for this functionality. Otherwise when the inputs-modifiers signal is triggered, 4 handlers will be triggered in sequence, and they will all send messages to the spice server. In my testing, the act of sending rapid-fire modifier updates messages to the spice server seemed to cause problems. So if we do want this functionality, it should probably live somewhere other than inside SpiceDisplay so that we can guarantee that the handler will only be triggered once.
Jonathon
----- Original Message -----
> From: "longguang.yue" <bigclouds@xxxxxxx>
> To: "Jonathon Jongsma" <jjongsma@xxxxxxxxxx>
> Cc: spice-devel@xxxxxxxxxxxxxxxxxxxxx
> Sent: Thursday, March 27, 2014 8:41:07 PM
> Subject: Re:Re: 0001-a patch for syncing keyboard lock status
>
> that is not enough.
> in that case server and client will keep syncing lock status all the time.
>
>
> we have to judge if two ends are identical already.
> my patch avoid modifying old functions, but for the sake of brevity, we have
> to modify old functions.
> sync_keyboard_lock_modifiers and where it emits inputs-modifiers sginal.
>
> thanks
>
>
>
>
>
>
> At 2014-03-28 00:31:32,"Jonathon Jongsma" <jjongsma@xxxxxxxxxx> wrote:
> >Thank you for the additional details. I can indeed reproduce the issue
> >here. However, my comments below still stand. You can essentially replace
> >your entire patch with the following and it should behave exactly the same
> >(and has the advantage of also building on windows):
> >
> >
> >diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> >index f5adf66..9d1db6e 100644
> >--- a/gtk/spice-widget.c
> >+++ b/gtk/spice-widget.c
> >@@ -2412,6 +2412,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, "inputs-modifiers",
> >G_CALLBACK(sync_keyboard_lock_modifiers), display);
> > spice_channel_connect(channel);
> > sync_keyboard_lock_modifiers(display);
> > return;
> >
> >
> >However, I still need to look into the details a bit more to see if this is
> >really the right way to handle this particular bug.
> >
> >Jonathon
> >
> >
> >
> >----- Original Message -----
> >> From: "longguang.yue" <bigclouds@xxxxxxx>
> >> To: "Jonathon Jongsma" <jjongsma@xxxxxxxxxx>
> >> Cc: spice-devel@xxxxxxxxxxxxxxxxxxxxx
> >> Sent: Thursday, March 27, 2014 9:05:19 AM
> >> Subject: Re:Re: 0001-a patch for syncing keyboard lock
> >> status
> >>
> >> my comment tells the reason.
> >> there are only one or two oppotunity to sync lock status.
> >> when channel_new and windows get focus. but some use cases do not
> >> encounter
> >> the latter one, because they use spice
> >> as desktop(maxmized when launched), no get-focus event will be catched, so
> >> if
> >> after input channel created but lock status is not identical , in this
> >> case
> >> lock status will never been corrected. you have to shutdown remote-viewer,
> >> and start it again.
> >> like you press keyboard during your computer start up, all the keyboard
> >> press event will be dropped . because OS is not ready and can not
> >> responde to keyboard interrupt. so there is a gap, you really press
> >> keyboard, but the event is dropped.
> >>
> >> reproduct steps: require spice window maxmized when connect to vm.
> >> 1. start vm
> >> 2.launch remote-viewer (window maxmized) right after start vm. let
> >> suppose
> >> CAPLK is down
> >> 3.press CAPLK(CAPLK=1)
> >> 4. wait vm finish start up . login in vm and edit a file, you will see all
> >> characters is small-capital-contrary.
> >>
> >> i am not sure if you can reproduct it, the key point is that there is a
> >> gap
> >> allow you to change lock status, but the changing is after channel
> >> connection and before vm can process interrupt.
> >> sync_keyboard_lock_modifiers does not make sure that lock status is
> >> identical
> >> after vm started.
> >>
> >> thanks
> >>
> >>
> >>
> >> At 2014-03-27 05:15:27,"Jonathon Jongsma" <jjongsma@xxxxxxxxxx> wrote:
> >> >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@xxxxxxxxx>
> >> >> 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
> >> >
> >> >_______________________________________________
> >> >Spice-devel mailing list
> >> >Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> >> >http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel