[Spice-devel] 0001-a patch for syncing keyboard lock status
Jonathon Jongsma
jjongsma at redhat.com
Thu Mar 27 09:31:32 PDT 2014
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 at 163.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>
> Cc: spice-devel at lists.freedesktop.org
> Sent: Thursday, March 27, 2014 9:05:19 AM
> Subject: Re:Re: [Spice-devel] 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 at redhat.com> 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 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
> >
> >_______________________________________________
> >Spice-devel mailing list
> >Spice-devel at lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list