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?
> This patch is actually not complete but more an RFC:
> - only X11 is currently supported;
I guess you have win32 support now
> - what happen with multimonitors? I don't think this patch
> it's causing regressions anyway;
> - 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?
> - 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
> - 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.
> + 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
> +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?
> + if (!force && can_set_keyboard_lock_modifiers()) {
> + return;
> + }
> +
can_set_keyboard_lock_modifiers() is only client side check, why not
sync the guest?
> 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...
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.
> }
>
> +/* Keyboard state changed in the client, try to sync
> + */
> static void keymap_modifiers_changed(GdkKeymap *keymap, gpointer data)
> {
> SpiceGtkSession *self = data;
>
> - spice_gtk_session_sync_keyboard_modifiers(self);
> + spice_gtk_session_sync_keyboard_modifiers(self, FALSE, from_client_to_guest);
> }
>
> static void guest_modifiers_changed(SpiceInputsChannel *inputs, gpointer data)
> {
> SpiceGtkSession *self = data;
>
> - spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs, FALSE);
> + spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs, FALSE, from_client_to_guest);
> }
>
> static void spice_gtk_session_init(SpiceGtkSession *self)
> @@ -1010,7 +1034,8 @@ static void channel_new(SpiceSession *session, SpiceChannel *channel,
> if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> spice_g_signal_connect_object(channel, "inputs-modifiers",
> G_CALLBACK(guest_modifiers_changed), self, 0);
> - spice_gtk_session_sync_keyboard_modifiers_for_channel(self, SPICE_INPUTS_CHANNEL(channel), TRUE);
> + spice_gtk_session_sync_keyboard_modifiers_for_channel(self, SPICE_INPUTS_CHANNEL(channel), TRUE,
> + from_client_to_guest);
> }
> }
>
> @@ -1170,15 +1195,16 @@ void spice_gtk_session_paste_from_guest(SpiceGtkSession *self)
> s->clip_hasdata[selection] = FALSE;
> }
>
> -G_GNUC_INTERNAL
> -void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self)
> +static void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self,
> + gboolean force,
> + keyboard_sync_dir dir)
> {
> GList *l = NULL, *channels = spice_session_get_channels(self->priv->session);
>
> for (l = channels; l != NULL; l = l->next) {
> if (SPICE_IS_INPUTS_CHANNEL(l->data)) {
> SpiceInputsChannel *inputs = SPICE_INPUTS_CHANNEL(l->data);
> - spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs, TRUE);
> + spice_gtk_session_sync_keyboard_modifiers_for_channel(self, inputs, force, dir);
> }
> }
> g_list_free(channels);
> @@ -1194,6 +1220,32 @@ void spice_gtk_session_set_pointer_grabbed(SpiceGtkSession *self, gboolean grabb
> }
>
> G_GNUC_INTERNAL
> +void spice_gtk_session_set_focus(SpiceGtkSession *self, gboolean focus)
> +{
> + g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> +
> + /* TODO detect switch between monitors */
> +
> + /* not changed ? */
> + if (self->priv->has_focus == focus)
> + return;
> +
> + if (focus) {
> + /* User switched to the viewer, try to syncronize the keyboard
> + * modifiers.
> + * This should be called before setting has_focus
> + */
> + spice_gtk_session_sync_keyboard_modifiers(self, TRUE, from_client_to_guest);
> + } else {
> + /* User swicthed to another application, syncronize the client
> + * keyboard */
> + spice_gtk_session_sync_keyboard_modifiers(self, TRUE, from_guest_to_client);
> + }
> +
> + self->priv->has_focus = focus;
> +}
> +
> +G_GNUC_INTERNAL
> gboolean spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self)
> {
> g_return_val_if_fail(SPICE_IS_GTK_SESSION(self), FALSE);
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index a77fdf3..10638ac 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -1682,7 +1682,7 @@ static gboolean focus_in_event(GtkWidget *widget, GdkEventFocus *focus G_GNUC_UN
>
> release_keys(display);
> if (!d->disable_inputs)
> - spice_gtk_session_sync_keyboard_modifiers(d->gtk_session);
> + spice_gtk_session_set_focus(d->gtk_session, TRUE);
> if (d->keyboard_grab_released)
> memset(d->activeseq, 0, sizeof(gboolean) * d->grabseq->nkeysyms);
> update_keyboard_focus(display, true);
> @@ -1717,6 +1717,9 @@ static gboolean focus_out_event(GtkWidget *widget, GdkEventFocus *focus G_GNUC_U
> if (d->keyboard_grab_active)
> return true;
>
> + if (!d->disable_inputs)
> + spice_gtk_session_set_focus(d->gtk_session, FALSE);
> +
> release_keys(display);
> update_keyboard_focus(display, false);
>
> --
> 2.7.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel