[Spice-devel] [PATCH 1/2] Ensure keyboard modifiers are synchronized properly
Jonathon Jongsma
jjongsma at redhat.com
Wed Apr 2 09:57:53 PDT 2014
----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau at gmail.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>
> Cc: spice-devel at freedesktop.org
> Sent: Wednesday, April 2, 2014 11:42:41 AM
> Subject: Re: [Spice-devel] [PATCH 1/2] Ensure keyboard modifiers are synchronized properly
>
> On Wed, Apr 2, 2014 at 5:58 PM, Jonathon Jongsma <jjongsma at redhat.com>wrote:
>
> > In certain circumstances, the keyboard modifiers get out-of-sync between
> > the
> > guest and the client. This is easy to reproduce with the following steps:
> > - launch virt-viewer with a guest that is not running
> > - start the guest
> > - while guest is booting, enable CAPS LOCK on the client
> > - after guest finishes booting, it will set its modifiers to a default
> > value
> > (e.g. numlock on, capslock off)
> > - now capslock is OFF in the guest, but ON in the client
> > - toggle caps lock
> > - now capslock is ON in the guest, but OFF in the client
> >
> > This fix consists of two parts. The first is that we register a signal
> > handler
> > for the InputsChannel::inputs-modifiers signal to detect when the guest has
> > changed its modifiers. The signal handler simply overrides the guests
> > modifiers
> > and sets them back to the value from the client.
> >
> > In order to avoid sending this message down to the guest multiple times,
> > I've
> > introduce a singleton SpiceKeyboardStateMonitor object that is responsible
> > for
> > synchronizing keyboard modifier state. Unfortunately it can't be done
> > directly
> > within the InputsChannel object since it depends on Gdk.
> >
>
> I think you should have done it in gtk-session, which is a singleton for
> the session and is gtk-aware.
Good point, I forgot about that object.
> Why having a gobject with a property if the object is all private?
Indeed, it's a bit pointless at the moment. Part of the reason I made it a property was because I was thinking that it might not be desirable behavior in all situations, and we could add a commandline switch (to spice_get_option_group()) to enable/disable that behavior. But maybe that's not useful. I can hard-code it if you prefer.
>
>
> > ---
> > gtk/Makefile.am | 2 +
> > gtk/spice-keyboard-state-monitor.c | 223
> > +++++++++++++++++++++++++++++++++++++
> > gtk/spice-keyboard-state-monitor.h | 56 ++++++++++
> > gtk/spice-widget.c | 95 +---------------
> > 4 files changed, 287 insertions(+), 89 deletions(-)
> > create mode 100644 gtk/spice-keyboard-state-monitor.c
> > create mode 100644 gtk/spice-keyboard-state-monitor.h
> >
> > diff --git a/gtk/Makefile.am b/gtk/Makefile.am
> > index 2e38cce..b8f42a9 100644
> > --- a/gtk/Makefile.am
> > +++ b/gtk/Makefile.am
> > @@ -126,6 +126,8 @@ SPICE_GTK_SOURCES_COMMON = \
> > spice-util-priv.h \
> > spice-gtk-session.c \
> > spice-gtk-session-priv.h \
> > + spice-keyboard-state-monitor.c \
> > + spice-keyboard-state-monitor.h \
> > spice-widget.c \
> > spice-widget-priv.h \
> > vncdisplaykeymap.c \
> > diff --git a/gtk/spice-keyboard-state-monitor.c
> > b/gtk/spice-keyboard-state-monitor.c
> > new file mode 100644
> > index 0000000..161dc43
> > --- /dev/null
> > +++ b/gtk/spice-keyboard-state-monitor.c
> > @@ -0,0 +1,223 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > +/*
> > + Copyright (C) 2014 Red Hat, Inc.
> > +
> > + This library is free software; you can redistribute it and/or
> > + modify it under the terms of the GNU Lesser General Public
> > + License as published by the Free Software Foundation; either
> > + version 2.1 of the License, or (at your option) any later version.
> > +
> > + This library is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + Lesser General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with this library; if not, see <
> > http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include "config.h"
> > +#endif
> > +
> > +#if HAVE_X11_XKBLIB_H
> > +#include <X11/XKBlib.h>
> > +#include <gdk/gdkx.h>
> > +#endif
> > +#ifdef GDK_WINDOWING_X11
> > +#include <X11/Xlib.h>
> > +#include <gdk/gdkx.h>
> > +#endif
> > +#ifdef WIN32
> > +#include <windows.h>
> > +#include <gdk/gdkwin32.h>
> > +#ifndef MAPVK_VK_TO_VSC /* may be undefined in older mingw-headers */
> > +#define MAPVK_VK_TO_VSC 0
> > +#endif
> > +#endif
> > +
> > +#include "channel-inputs.h"
> > +#include "spice-keyboard-state-monitor.h"
> > +
> > +#include <gdk/gdk.h>
> > +
> > +G_DEFINE_TYPE(SpiceKeyboardStateMonitor, spice_keyboard_state_monitor,
> > G_TYPE_OBJECT)
> > +
> > +#define KEYBOARD_STATE_MONITOR_PRIVATE(o) \
> > + (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > SPICE_TYPE_KEYBOARD_STATE_MONITOR, SpiceKeyboardStateMonitorPrivate))
> > +
> > + struct _SpiceKeyboardStateMonitorPrivate
> > +{
> > + GList *inputs_channels;
> > + gboolean override_guest_modifiers;
> > +};
> > +
> > +enum {
> > + PROP_0,
> > + PROP_OVERRIDE_GUEST_MODIFIERS
> > +};
> > +
> > +static void spice_keyboard_state_monitor_finalize(GObject *object)
> > +{
> > + SpiceKeyboardStateMonitor *self =
> > SPICE_KEYBOARD_STATE_MONITOR(object);
> > + g_list_free(self->priv->inputs_channels);
> > +
> > G_OBJECT_CLASS(spice_keyboard_state_monitor_parent_class)->finalize(object);
> > +}
> > +
> > +static void spice_keyboard_state_monitor_get_property(GObject *object,
> > + guint prop_id,
> > + GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + SpiceKeyboardStateMonitor *self =
> > SPICE_KEYBOARD_STATE_MONITOR(object);
> > + switch (prop_id) {
> > + case PROP_OVERRIDE_GUEST_MODIFIERS:
> > + g_value_set_boolean(value,
> > self->priv->override_guest_modifiers);
> > + break;
> > + default:
> > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > + break;
> > + }
> > +}
> > +
> > +static void spice_keyboard_state_monitor_set_property(GObject *object,
> > + guint prop_id,
> > + const GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + SpiceKeyboardStateMonitor *self =
> > SPICE_KEYBOARD_STATE_MONITOR(object);
> > + switch (prop_id) {
> > + case PROP_OVERRIDE_GUEST_MODIFIERS:
> > + self->priv->override_guest_modifiers =
> > g_value_get_boolean(value);
> > + break;
> > + default:
> > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > + break;
> > + }
> > +}
> > +
> > +static void
> > spice_keyboard_state_monitor_class_init(SpiceKeyboardStateMonitorClass
> > *klass)
> > +{
> > + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > +
> > + g_type_class_add_private(klass, sizeof
> > (SpiceKeyboardStateMonitorPrivate));
> > +
> > + object_class->get_property =
> > spice_keyboard_state_monitor_get_property;
> > + object_class->set_property =
> > spice_keyboard_state_monitor_set_property;
> > + object_class->finalize = spice_keyboard_state_monitor_finalize;
> > +
> > + g_object_class_install_property(object_class,
> > PROP_OVERRIDE_GUEST_MODIFIERS,
> > +
> > g_param_spec_boolean("override-guest-modifiers",
> > + "Override guest
> > modifiers",
> > + "Whether guest
> > modifiers should be overriidden by client modifiers",
> > + TRUE,
> > +
> > G_PARAM_READWRITE |
> > +
> > G_PARAM_CONSTRUCT |
> > +
> > G_PARAM_STATIC_STRINGS));
> > +}
> > +
> > +static guint32 get_keyboard_lock_modifiers(void)
> > +{
> > + guint32 modifiers = 0;
> > +#if HAVE_X11_XKBLIB_H
> > + Display *x_display = NULL;
> > + XKeyboardState keyboard_state;
> > +
> > + 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 0;
> > + }
> > +
> > + x_display = GDK_SCREEN_XDISPLAY(screen);
> > + XGetKeyboardControl(x_display, &keyboard_state);
> > +
> > + if (keyboard_state.led_mask & 0x01) {
> > + modifiers |= SPICE_INPUTS_CAPS_LOCK;
> > + }
> > + if (keyboard_state.led_mask & 0x02) {
> > + modifiers |= SPICE_INPUTS_NUM_LOCK;
> > + }
> > + if (keyboard_state.led_mask & 0x04) {
> > + modifiers |= SPICE_INPUTS_SCROLL_LOCK;
> > + }
> > +#elif defined(win32)
> > + if (GetKeyState(VK_CAPITAL) & 1) {
> > + modifiers |= SPICE_INPUTS_CAPS_LOCK;
> > + }
> > + if (GetKeyState(VK_NUMLOCK) & 1) {
> > + modifiers |= SPICE_INPUTS_NUM_LOCK;
> > + }
> > + if (GetKeyState(VK_SCROLL) & 1) {
> > + modifiers |= SPICE_INPUTS_SCROLL_LOCK;
> > + }
> > +#else
> > + g_warning("get_keyboard_lock_modifiers not implemented");
> > +#endif // HAVE_X11_XKBLIB_H
> > + return modifiers;
> > +}
> > +
> > +static void spice_keyboard_state_monitor_init(SpiceKeyboardStateMonitor
> > *self)
> > +{
> > + self->priv = KEYBOARD_STATE_MONITOR_PRIVATE(self);
> > +}
> > +
> > +static gpointer spice_keyboard_state_monitor_new(gpointer data)
> > +{
> > + return g_object_new(SPICE_TYPE_KEYBOARD_STATE_MONITOR, NULL);
> > +}
> > +
> > +SpiceKeyboardStateMonitor* spice_keyboard_state_monitor_get_default(void)
> > +{
> > + static GOnce once = G_ONCE_INIT;
> > + g_once(&once, spice_keyboard_state_monitor_new, NULL);
> > + return once.retval;
> > +}
> > +
> > +static void
> > spice_keyboard_state_monitor_sync_one(SpiceKeyboardStateMonitor *self,
> > + SpiceInputsChannel*
> > inputs)
> > +{
> > + gint guest_modifiers = 0, client_modifiers = 0;
> > +
> > + g_return_if_fail(SPICE_IS_INPUTS_CHANNEL(inputs));
> > +
> > + g_object_get(inputs, "key-modifiers", &guest_modifiers, NULL);
> > +
> > + client_modifiers = get_keyboard_lock_modifiers();
> > + g_debug("%s: input:%p client_modifiers:0x%x, guest_modifiers:0x%x",
> > + G_STRFUNC, inputs, client_modifiers, guest_modifiers);
> > +
> > + if (client_modifiers != guest_modifiers)
> > + spice_inputs_set_key_locks(inputs, client_modifiers);
> > +}
> > +
> > +void
> > spice_keyboard_state_monitor_sync_modifiers(SpiceKeyboardStateMonitor
> > *self)
> > +{
> > + g_debug("%s: modifiers", G_STRFUNC);
> > + GList *l = self->priv->inputs_channels;
> > + for (; l; l = l->next) {
> > + SpiceInputsChannel *inputs = SPICE_INPUTS_CHANNEL(l->data);
> > + spice_keyboard_state_monitor_sync_one(self, inputs);
> > + }
> > +}
> > +
> > +static void guest_modifiers_changed(SpiceInputsChannel *inputs, gpointer
> > data)
> > +{
> > + SpiceKeyboardStateMonitor *self = data;
> > + if (self->priv->override_guest_modifiers)
> > + spice_keyboard_state_monitor_sync_one(self, inputs);
> > +}
> > +
> > +void
> > spice_keyboard_state_monitor_register_inputs_channel(SpiceKeyboardStateMonitor
> > *self,
> > +
> > SpiceInputsChannel *inputs)
> > +{
> > + /* avoid registering the same channel multiple times */
> > + if (g_list_find(self->priv->inputs_channels, inputs) != NULL)
> > + return;
> > +
> > + spice_g_signal_connect_object(inputs, "inputs-modifiers",
> > + G_CALLBACK(guest_modifiers_changed),
> > self, 0);
> > + self->priv->inputs_channels =
> > g_list_append(self->priv->inputs_channels,
> > + inputs);
> > +}
> > +
> > diff --git a/gtk/spice-keyboard-state-monitor.h
> > b/gtk/spice-keyboard-state-monitor.h
> > new file mode 100644
> > index 0000000..2c2033a
> > --- /dev/null
> > +++ b/gtk/spice-keyboard-state-monitor.h
> > @@ -0,0 +1,56 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > +/*
> > + Copyright (C) 2014 Red Hat, Inc.
> > +
> > + This library is free software; you can redistribute it and/or
> > + modify it under the terms of the GNU Lesser General Public
> > + License as published by the Free Software Foundation; either
> > + version 2.1 of the License, or (at your option) any later version.
> > +
> > + This library is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + Lesser General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with this library; if not, see <
> > http://www.gnu.org/licenses/>.
> > +*/
> > +#ifndef __SPICE_KEYBOARD_STATE_MONITOR_H__
> > +#define __SPICE_KEYBOARD_STATE_MONITOR_H__
> > +
> > +#include <glib-object.h>
> > +#include "channel-inputs.h"
> > +
> > +G_BEGIN_DECLS
> > +
> > +#define SPICE_TYPE_KEYBOARD_STATE_MONITOR
> > spice_keyboard_state_monitor_get_type()
> > +#define SPICE_KEYBOARD_STATE_MONITOR(obj)
> > (G_TYPE_CHECK_INSTANCE_CAST((obj), SPICE_TYPE_KEYBOARD_STATE_MONITOR,
> > SpiceKeyboardStateMonitor))
> > +#define SPICE_KEYBOARD_STATE_MONITOR_CLASS(klass)
> > (G_TYPE_CHECK_CLASS_CAST((klass), SPICE_TYPE_KEYBOARD_STATE_MONITOR,
> > SpiceKeyboardStateMonitorClass))
> > +#define SPICE_IS_KEYBOARD_STATE_MONITOR(obj)
> > (G_TYPE_CHECK_INSTANCE_TYPE((obj), SPICE_TYPE_KEYBOARD_STATE_MONITOR))
> > +#define SPICE_IS_KEYBOARD_STATE_MONITOR_CLASS(klass)
> > (G_TYPE_CHECK_CLASS_TYPE((klass), SPICE_TYPE_KEYBOARD_STATE_MONITOR))
> > +#define SPICE_KEYBOARD_STATE_MONITOR_GET_CLASS(obj)
> > (G_TYPE_INSTANCE_GET_CLASS((obj), SPICE_TYPE_KEYBOARD_STATE_MONITOR,
> > SpiceKeyboardStateMonitorClass))
> > +
> > +typedef struct _SpiceKeyboardStateMonitor SpiceKeyboardStateMonitor;
> > +typedef struct _SpiceKeyboardStateMonitorClass
> > SpiceKeyboardStateMonitorClass;
> > +typedef struct _SpiceKeyboardStateMonitorPrivate
> > SpiceKeyboardStateMonitorPrivate;
> > +
> > +struct _SpiceKeyboardStateMonitor
> > +{
> > + GObject parent;
> > + SpiceKeyboardStateMonitorPrivate *priv;
> > +};
> > +
> > +struct _SpiceKeyboardStateMonitorClass
> > +{
> > + GObjectClass parent_class;
> > +};
> > +
> > +GType spice_keyboard_state_monitor_get_type(void) G_GNUC_CONST;
> > +
> > +SpiceKeyboardStateMonitor *spice_keyboard_state_monitor_get_default(void);
> > +void
> > spice_keyboard_state_monitor_register_inputs_channel(SpiceKeyboardStateMonitor
> > *self, SpiceInputsChannel *inputs);
> > +void
> > spice_keyboard_state_monitor_sync_modifiers(SpiceKeyboardStateMonitor
> > *self);
> > +
> > +G_END_DECLS
> > +
> > +#endif /* __SPICE_KEYBOARD_STATE_MONITOR_H__ */
> > diff --git a/gtk/spice-widget.c b/gtk/spice-widget.c
> > index 0e4a0de..dc9198c 100644
> > --- a/gtk/spice-widget.c
> > +++ b/gtk/spice-widget.c
> > @@ -38,6 +38,7 @@
> > #include "spice-widget.h"
> > #include "spice-widget-priv.h"
> > #include "spice-gtk-session-priv.h"
> > +#include "spice-keyboard-state-monitor.h"
> > #include "vncdisplaykeymap.h"
> >
> > #include "glib-compat.h"
> > @@ -131,7 +132,6 @@ static void try_mouse_ungrab(SpiceDisplay *display);
> > static void recalc_geometry(GtkWidget *widget);
> > static void channel_new(SpiceSession *s, SpiceChannel *channel, gpointer
> > data);
> > static void channel_destroy(SpiceSession *s, SpiceChannel *channel,
> > gpointer data);
> > -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);
> > @@ -1457,7 +1457,8 @@ static gboolean focus_in_event(GtkWidget *widget,
> > GdkEventFocus *focus G_GNUC_UN
> > return true;
> >
> > release_keys(display);
> > - sync_keyboard_lock_modifiers(display);
> > + if (!d->disable_inputs)
> > +
> > spice_keyboard_state_monitor_sync_modifiers(spice_keyboard_state_monitor_get_default());
> > update_keyboard_focus(display, true);
> > try_keyboard_grab(display);
> > update_display(display);
> > @@ -2411,7 +2412,9 @@ static void channel_new(SpiceSession *s,
> > SpiceChannel *channel, gpointer data)
> > if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> > d->inputs = SPICE_INPUTS_CHANNEL(channel);
> > spice_channel_connect(channel);
> > - sync_keyboard_lock_modifiers(display);
> > +
> > spice_keyboard_state_monitor_register_inputs_channel(spice_keyboard_state_monitor_get_default(),
> > d->inputs);
> > + if (!d->disable_inputs)
> > +
> > spice_keyboard_state_monitor_sync_modifiers(spice_keyboard_state_monitor_get_default());
> > return;
> > }
> >
> > @@ -2600,89 +2603,3 @@ GdkPixbuf *spice_display_get_pixbuf(SpiceDisplay
> > *display)
> > return pixbuf;
> > }
> >
> > -#if HAVE_X11_XKBLIB_H
> > -static guint32 get_keyboard_lock_modifiers(Display *x_display)
> > -{
> > - XKeyboardState keyboard_state;
> > - guint32 modifiers = 0;
> > -
> > - XGetKeyboardControl(x_display, &keyboard_state);
> > -
> > - if (keyboard_state.led_mask & 0x01) {
> > - modifiers |= SPICE_INPUTS_CAPS_LOCK;
> > - }
> > - if (keyboard_state.led_mask & 0x02) {
> > - modifiers |= SPICE_INPUTS_NUM_LOCK;
> > - }
> > - if (keyboard_state.led_mask & 0x04) {
> > - modifiers |= SPICE_INPUTS_SCROLL_LOCK;
> > - }
> > - return modifiers;
> > -}
> > -
> > -static void sync_keyboard_lock_modifiers(SpiceDisplay *display)
> > -{
> > - Display *x_display;
> > - SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> > - guint32 modifiers;
> > - GdkWindow *w;
> > -
> > - if (d->disable_inputs)
> > - return;
> > -
> > - w = gtk_widget_get_parent_window(GTK_WIDGET(display));
> > - if (w == NULL) /* it can happen if the display is not yet shown */
> > - return;
> > -
> > - if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
> > - SPICE_DEBUG("FIXME: gtk backend is not X11");
> > - return;
> > - }
> > -
> > - x_display = GDK_WINDOW_XDISPLAY(w);
> > - modifiers = get_keyboard_lock_modifiers(x_display);
> > - if (d->inputs)
> > - spice_inputs_set_key_locks(d->inputs, modifiers);
> > -}
> > -
> > -#elif defined (WIN32)
> > -static guint32 get_keyboard_lock_modifiers(void)
> > -{
> > - guint32 modifiers = 0;
> > -
> > - if (GetKeyState(VK_CAPITAL) & 1) {
> > - modifiers |= SPICE_INPUTS_CAPS_LOCK;
> > - }
> > - if (GetKeyState(VK_NUMLOCK) & 1) {
> > - modifiers |= SPICE_INPUTS_NUM_LOCK;
> > - }
> > - if (GetKeyState(VK_SCROLL) & 1) {
> > - modifiers |= SPICE_INPUTS_SCROLL_LOCK;
> > - }
> > -
> > - return modifiers;
> > -}
> > -
> > -static void sync_keyboard_lock_modifiers(SpiceDisplay *display)
> > -{
> > - SpiceDisplayPrivate *d = SPICE_DISPLAY_GET_PRIVATE(display);
> > - guint32 modifiers;
> > - GdkWindow *w;
> > -
> > - if (d->disable_inputs)
> > - return;
> > -
> > - w = gtk_widget_get_parent_window(GTK_WIDGET(display));
> > - if (w == NULL) /* it can happen if the display is not yet shown */
> > - return;
> > -
> > - modifiers = get_keyboard_lock_modifiers();
> > - if (d->inputs)
> > - spice_inputs_set_key_locks(d->inputs, modifiers);
> > -}
> > -#else
> > -static void sync_keyboard_lock_modifiers(SpiceDisplay *display)
> > -{
> > - g_warning("sync_keyboard_lock_modifiers not implemented");
> > -}
> > -#endif // HAVE_X11_XKBLIB_H
> > --
> > 1.9.0
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>
>
>
> --
> Marc-André Lureau
>
More information about the Spice-devel
mailing list