[Spice-devel] [spice-gtk] RFC Sync only on focus change
Frediano Ziglio
fziglio at redhat.com
Mon May 23 09:28:57 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) ?
>
Running my tests on these stuff and attempting to fix various combination
of caps lock I disabled the syncing. Obviously if you disable sync there are
no weird behavior however was quite frustrating moving between windows (on
client machine) and finding different settings.
How important it is? We have to take into account that:
- Qemu implement synchronization (VNC);
- we (both spice-server and spice-gtk) implement synchronization;
- there have been quite a lot of bugs opened for caps lock problems
(like not in sync when machine boots).
Considering all that effort I would say that modifiers synchronization is quite
important for a lot of people.
I personally don't use caps lock that much, the number of upper cases insertions
are quite low and shift is usually the best approach but I had some experience
where I can say caps lock is really used daily. Many banks (that is for us
big customers) have still old systems which work on upper cases input.
There are jobs where people are not that computer friendly and simply having
to press Shift-A to get a capital A is much harder to press 3 keys (and
really I saw with my eyes!).
We just throw keys expecting everything to work. In a lot of cases the caps
lock key does the caps lock so usually works even if you disable sync but
this is a quite unfair consideration. Try to disable all syncs, set VMs
in different ways and type in different virt-viewers typing sometimes all upper
case and sometimes all lower cases (using caps of course!)
About the additional sync I don't get your idea. Which direction (guest ->
client or client -> guest) ?
I'm implementing setting modifiers in Windows. In X11 was quite easy actually,
I suppose wayland coders were quite familiar with X11 so I suppose they brought
similar knowledge into wayland.
About "led controlling". One problem here is that we use leds to understand
OS/user idea about modifiers. The problem could be that OS/applications
don't sync the two. This for instance happens currently using physical
terminals on different Linux, caps led is never enabled even if caps lock
is on in the terminal. That's one reason you are working on patches to send
the real caps lock state using the agent.
> >
> > 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
>
Yes, not when the TODO above will (if) be implemented.
> > +
> > + 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 ?
>
Actually my question is why can be set differently for different display.
I mean, if a guest have multiple monitors does it make sense that input
is disabled using one monitor and enabled in another?
I think the disable inputs should be more "global" like stored in SpiceGtkSession.
Or actually I didn't get the usage of this property.
> > 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
>
>
Actually we don't have a way to understand "the guest doesn't want to sync".
Too many variable. What if the user open an application like a game that does
not care about caps lock behavior but just uses raw keys?
What if one terminal/session in the guest just hung?
Frediano
More information about the Spice-devel
mailing list