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