Finally found some time to work on this patchset.
>
> Hi
>
> On Wed, Jun 8, 2016 at 1:10 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > Limit the virtual keystrokes sent to the remote machine.
> > The modifiers are synced only when the application receive or lose
> > the focus. This reduce a lot the possible virtual keystrokes sent
> > to the guest to synchronize the modifiers.
> > This affect the situations where modifiers are configured
> > differently in client and guest.
> > When the application receive the focus the synchronization is
> > attempted from client to guest while when the application lose
> > focus is attempted guest to client (basically is moved following
> > user moving).
>
> Why not change guest to client immediately when it has the focus? If
> not, wouldn't this make keyboard leds not synchronized with the guest?
>
Can happen if guest is not behaving like client think.
In the worst case the leds are inverted. This as client should invert
leds when modifiers are pressed.
> > This patch is actually not complete but more an RFC:
> > - only X11 is currently supported;
>
> I guess you have win32 support now
>
Fixed.
> > - what happen with multimonitors? I don't think this patch
> > it's causing regressions anyway;
Not a regression, removed
> > - instead of calling spice_inputs_set_key_locks to send
> > a message to spice-server which will insert the keystrokes
> > based on spice-server knowledge of the modifiers use
> > client knowledge, this will also help when some more knowledge
> > of the guest status will be implemented in the guest;
>
> I don't understand what you are talking about here, is this still
> relevant with this series?
>
No, removed, was a future change.
> > - there are some possible changes in behavior for
> > keymap_modifiers_changed;
>
> Shouldn't we revert this commit? I fail to understand how it fits with
> all your changes. I hope Jonathon can look at it.
>
> commit d06b256710cf91aec1275785d8cd65283581f544
> Author: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> Date: Mon Mar 31 11:07:53 2014 -0500
>
> Use GdkKeymap to listen for keyboard modifier changes
>
No, for no X11/Windows this is working.
>
> > - one possible regression is that if you are using virt-viewer
> > and the guest is booted it's possible the boot process will switch
> > modifiers status. Honestly I consider this more of an improvement
> > than a regression.
>
> fair enough, hopefully users will agree with you
>
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > src/spice-gtk-session-priv.h | 2 +-
> > src/spice-gtk-session.c | 68
> > ++++++++++++++++++++++++++++++++++++++------
> > src/spice-widget.c | 5 +++-
> > 3 files changed, 65 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/spice-gtk-session-priv.h b/src/spice-gtk-session-priv.h
> > index d7fe313..abf90d6 100644
> > --- a/src/spice-gtk-session-priv.h
> > +++ b/src/spice-gtk-session-priv.h
> > @@ -38,13 +38,13 @@ struct _SpiceGtkSessionClass
> > void spice_gtk_session_request_auto_usbredir(SpiceGtkSession *self,
> > gboolean state);
> > gboolean spice_gtk_session_get_read_only(SpiceGtkSession *self);
> > -void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self);
> > void spice_gtk_session_set_pointer_grabbed(SpiceGtkSession *self, gboolean
> > grabbed);
> > gboolean spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self);
> > void spice_gtk_session_set_keyboard_has_focus(SpiceGtkSession *self,
> > gboolean keyboard_has_focus);
> > void spice_gtk_session_set_mouse_has_pointer(SpiceGtkSession *self,
> > gboolean mouse_has_pointer);
> > gboolean spice_gtk_session_get_keyboard_has_focus(SpiceGtkSession *self);
> > gboolean spice_gtk_session_get_mouse_has_pointer(SpiceGtkSession *self);
> > +void spice_gtk_session_set_focus(SpiceGtkSession *self, gboolean focus);
> >
> > G_END_DECLS
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b2028d9..84be84c 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -67,6 +67,8 @@ struct _SpiceGtkSessionPrivate {
> > gboolean keyboard_has_focus;
> > gboolean mouse_has_pointer;
> > gboolean sync_modifiers;
> > + /** the session has a window with the focus */
>
> spice-gtk-session.c:70: Error: SpiceClientGtk: Skipping invalid
> GTK-Doc comment block:
> /** the session has a window with the focus */
> ^
>
> Imho has_focus is already pretty clear by itself.
updated
>
> > + gboolean has_focus;
> > };
> >
> > /**
> > @@ -104,6 +106,13 @@ static void channel_new(SpiceSession *session,
> > SpiceChannel *channel,
> > static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
> > gpointer user_data);
> > static gboolean read_only(SpiceGtkSession *self);
> > +typedef enum {
> > + from_guest_to_client,
> > + from_client_to_guest
> > +} keyboard_sync_dir;
>
> Please use uppercase for enum and constants
>
Removed, no more necessary
> > +static void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession
> > *self,
> > + gboolean force,
> > + keyboard_sync_dir
> > dir);
> >
> > /* ------------------------------------------------------------------ */
> > /* gobject glue */
> > @@ -125,7 +134,8 @@ enum {
> >
> > static void
> > spice_gtk_session_sync_keyboard_modifiers_for_channel(SpiceGtkSession
> > *self,
> > SpiceInputsChannel*
> > inputs,
> > - gboolean
> > force)
> > + gboolean
> > force,
> > +
> > keyboard_sync_dir
> > dir)
> > {
> > guint32 guest_modifiers = 0, client_modifiers = 0;
> >
> > @@ -136,28 +146,42 @@ static void
> > spice_gtk_session_sync_keyboard_modifiers_for_channel(SpiceGtkSessio
> > return;
> > }
> >
> > + // if client synchronization is enabled this is done
> > + // by widget
>
> I find this comment confusing. What do you mean?
>
updated
> > + if (!force && can_set_keyboard_lock_modifiers()) {
> > + return;
> > + }
> > +
>
> can_set_keyboard_lock_modifiers() is only client side check, why not
> sync the guest?
>
New code should be more clear, the confusion came from the old
comment and the wrong place of handling guest -> client sync.
>
> > g_object_get(inputs, "key-modifiers", &guest_modifiers, NULL);
> > client_modifiers = get_keyboard_lock_modifiers();
> >
> > - if (force || client_modifiers != guest_modifiers) {
> > + if (!force && client_modifiers == guest_modifiers) {
> > + return;
> > + }
> > +
> > + if (dir == from_client_to_guest) {
> > CHANNEL_DEBUG(inputs, "client_modifiers:0x%x,
> > guest_modifiers:0x%x",
> > client_modifiers, guest_modifiers);
> > spice_inputs_set_key_locks(inputs, client_modifiers);
> > + } else {
> > + set_keyboard_lock_modifiers(guest_modifiers);
> > }
>
> Looks like this is the wrong place to put this call, since it will be
> called for each input channels, you should move it down to
> spice_gtk_session_sync_keyboard_modifiers() and somehow combine the
> modifiers state of the various inputs or take the first channel. But
> that brings new problems or limitations...
>
Changed. Yes, was the wrong place.
> I would like to understand why the client locks should follow the
> guest locks. To me, the VM should behave like an application, and thus
> it is pretty bad if it starts changing your keyboard status. I would
> rather have an indicator if necessary (like the inputs_modifiers() in
> spicy), and make the best for the VM to follow my client keyboard
> state instead.
>
A VM is not an application, VM handle locks in its own way.
This is future work, VM cannot (currently) follow client state, the current
code (client/server) is just trying to do something "smart".
....
Following new patchset.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel