[Spice-devel] [spice-gtk] RFC Sync only on focus change
Pavel Grunt
pgrunt at redhat.com
Mon May 23 08:04:53 UTC 2016
Hi Frediano,
On Fri, 2016-05-20 at 15:03 +0100, Frediano Ziglio 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).
>
> This patch is actually not complete but more an RFC:
> - only X11 is currently supported and there are some
> embedded constants which should be changed even for X11
> (from commit 063c1b9c0627c87eb7f5369c4a6b9776a22e5c7d);
> I think to move the function in a separate file (you will
> understand the reason with Windows...);
> - 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;
> - there are some possible changes in behavior for
> keymap_modifiers_changed;
> - 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.
> return true;
>
> + if (!d->disable_inputs)
> + spice_gtk_session_set_focus(d->gtk_session, FALSE);
> +
> release_keys(display);
> update_keyboard_focus(display, false);
I cannot see it as improvement - it can be confusing, imo would be better to not
do the sync at all. But if you think it is worth to continue (I don't know how
easy is to implement "led controling" for wayland and Windows) - what do you
think about an additional sync when agent connects (&& session has focus) ?
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> src/spice-gtk-session-priv.h | 2 +-
> src/spice-gtk-session.c | 104 +++++++++++++++++++++++++++++++++++++++---
> -
> src/spice-widget.c | 5 ++-
> 3 files changed, 101 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 bbcbeeb..476098e 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -66,6 +66,8 @@ struct _SpiceGtkSessionPrivate {
> gboolean keyboard_has_focus;
> gboolean mouse_has_pointer;
> gboolean sync_modifiers;
> + /** the session has a window with the focus */
> + gboolean has_focus;
> };
>
> /**
> @@ -103,6 +105,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;
> +static void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self,
> + gboolean force,
> + keyboard_sync_dir dir);
>
> /* ------------------------------------------------------------------ */
> /* gobject glue */
> @@ -179,9 +188,45 @@ static guint32 get_keyboard_lock_modifiers(void)
> return modifiers;
> }
>
> +static void set_keyboard_lock_modifiers(guint32 modifiers)
> +{
> +#ifdef HAVE_X11_XKBLIB_H
> + // TODO fixed ??
> + enum {
> + XKB_CAPS_LOCK = 2,
> + XKB_NUM_LOCK = 0x10,
> + XKB_SCROLL_LOCK = 0 // TODO
> + };
> + Display *x_display = NULL;
> + int x_modifiers = 0;
> +
> + GdkScreen *screen = gdk_screen_get_default();
> + if (!GDK_IS_X11_DISPLAY(gdk_screen_get_display(screen))) {
> + SPICE_DEBUG("FIXME: gtk backend is not X11");
> + return;
> + }
> +
> + x_display = GDK_SCREEN_XDISPLAY(screen);
> +
> + if ((modifiers & SPICE_INPUTS_CAPS_LOCK) != 0) {
> + x_modifiers |= XKB_CAPS_LOCK;
> + }
> + if ((modifiers & SPICE_INPUTS_NUM_LOCK) != 0) {
> + x_modifiers |= XKB_NUM_LOCK;
> + }
> + if ((modifiers & SPICE_INPUTS_SCROLL_LOCK) != 0) {
> + x_modifiers |= XKB_SCROLL_LOCK;
> + }
> + XkbLockModifiers(x_display, XkbUseCoreKbd,
> XKB_CAPS_LOCK|XKB_NUM_LOCK|XKB_SCROLL_LOCK, x_modifiers);
> +#else
> + g_warning("set_keyboard_lock_modifiers not implemented");
> +#endif // HAVE_X11_XKBLIB_H
> +}
> +
> static void
> spice_gtk_session_sync_keyboard_modifiers_for_channel(SpiceGtkSession *self,
> SpiceInputs
> Channel* inputs,
> - gboolean
> force)
> + gboolean
> force,
> + keyboard_sy
> nc_dir dir)
> {
> gint guest_modifiers = 0, client_modifiers = 0;
>
> @@ -192,28 +237,43 @@ static void
> spice_gtk_session_sync_keyboard_modifiers_for_channel(SpiceGtkSessio
> return;
> }
>
> + // if client synchronization is enabled this is done
> + // by widget
> + // TODO check if possible to do client sync
> + if (!force) {
> + return;
> + }
> +
> 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;
> + }
force is always TRUE here
> +
> + 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);
> }
> }
>
> +/* 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)
> @@ -1066,7 +1126,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);
> }
> }
>
> @@ -1226,15 +1287,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);
> @@ -1250,6 +1312,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 cbca5dc..07c0f79 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -1681,7 +1681,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);
> @@ -1708,6 +1708,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);
> +
This deserves some more comments, also it is a SpiceDisplay property - what will
happen when one SpiceDisplay has disable_inputs = TRUE and another FALSE ?
> release_keys(display);
> update_keyboard_focus(display, false);
In general I think we are making the sync stuff much more complicated.
Would not be easier / more correct to disable the sync when the guest doesn't
want to sync ?
Pavel
More information about the Spice-devel
mailing list