[Spice-devel] 0001-a patch for syncing keyboard lock status
longguang.yue
bigclouds at 163.com
Thu Mar 27 07:05:19 PDT 2014
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140327/b3dcfafa/attachment-0001.html>
More information about the Spice-devel
mailing list