----- Original Message ----- > From: "Marc-André Lureau" <marcandre.lureau@xxxxxxxxx> > To: "Jonathon Jongsma" <jjongsma@xxxxxxxxxx> > Cc: spice-devel@xxxxxxxxxxxxxxx > Sent: Wednesday, April 2, 2014 11:42:41 AM > Subject: Re: [PATCH 1/2] Ensure keyboard modifiers are synchronized properly > > On Wed, Apr 2, 2014 at 5:58 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx>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@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > > > -- > Marc-André Lureau > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel