<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Apr 2, 2014 at 5:58 PM, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com" target="_blank">jjongsma@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. </blockquote><div> </div><div>I think you should have done it in gtk-session, which is a singleton for the session and is gtk-aware. </div><div>Why having a gobject with a property if the object is all private? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- 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 <<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>>. + */ + +#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 <<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>>. +*/ +#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 <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </blockquote></div> -- Marc-André Lureau </div></div>