Re: [Spice-devel] 0001-a patch for syncing keyboard lock status
How about this alternative approach? It introduces a new central keyboard monitor object which can keep the registered input channels in sync with the client keyboard state. This object has an 'override-guest-modifiers' property which controls whether it will connect to the 'inputs-modifiers' signal on the inputs channels and override detected changes on the guest with the current state of the client modifiers. This is the core functionality introduced by the previous patch, but it is done here in a way that guarantees that the signal handler is only triggered once. This property is enabled by default. [PATCH 1/2] Ensure keyboard modifiers are synchronized properly [PATCH 2/2] Use GdkKeymap to listen for keyboard modifier changes ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] 0001-a patch for syncing keyboard lock status
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 bigclo...@163.com
To: Jonathon Jongsma jjong...@redhat.com
Cc: spice-devel@lists.freedesktop.org
Sent: Thursday, March 27, 2014 8:41:07 PM
Subject: Re:Re: [Spice-devel] 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 jjong...@redhat.com 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 bigclo...@163.com
To: Jonathon Jongsma jjong...@redhat.com
Cc: spice-devel@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 jjong...@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@gmail.com
Date: Wed, 26 Mar 2014
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 jjong...@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@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
Re: [Spice-devel] 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 jjong...@redhat.com 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 bigclo...@163.com
To: Jonathon Jongsma jjong...@redhat.com
Cc: spice-devel@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 jjong...@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@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
[Spice-devel] 0001-a patch for syncing keyboard lock status
thanks 0001-there-is-only-few-oppotunity-one-or-two-to-sync-keyb.patch Description: Binary data ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] 0001-a patch for syncing keyboard lock status
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@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);
