[Spice-devel] [PATCH spice-gtk v3 2/2] Send keyboard description

[Pavel Grunt]

Hi,

> 
> Hey,
> 
> I wonder if we always want to do this kind of synchronization
> automatically, or if it should be optional? This would mean a new
> property on SpiceGtkSession which would be checked before sending the
> layout.

I agree with you, it would be better to have it optional.

> 
> On Wed, Nov 05, 2014 at 03:20:33PM +0100, Pavel Grunt wrote:
> > The keyboard description of the client will be sent to the guest
> > when the connection is established.
> > 
> > RFE: https://bugs.freedesktop.org/show_bug.cgi?id=85332
> > ---
> > v3:
> >  - wrong g_return_val_if_fail replaced by g_warn_if_fail
> > v2:
> >  - reduced indentation level
> >  - removed checking all the channels
> > ---
> >  gtk/spice-gtk-session-priv.h |  1 +
> >  gtk/spice-gtk-session.c      | 63
> >  ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> > 
> > diff --git a/gtk/spice-gtk-session-priv.h b/gtk/spice-gtk-session-priv.h
> > index 91304b2..571f91f 100644
> > --- a/gtk/spice-gtk-session-priv.h
> > +++ b/gtk/spice-gtk-session-priv.h
> > @@ -28,6 +28,7 @@ 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_sync_keyboard_layout(SpiceGtkSession *self);
> >  
> >  G_END_DECLS
> >  
> > diff --git a/gtk/spice-gtk-session.c b/gtk/spice-gtk-session.c
> > index 52ad597..02de129 100644
> > --- a/gtk/spice-gtk-session.c
> > +++ b/gtk/spice-gtk-session.c
> > @@ -178,6 +178,13 @@ static void
> > spice_gtk_session_sync_keyboard_modifiers_for_channel(SpiceGtkSessio
> >      }
> >  }
> >  
> > +static void keymap_layout_changed(GdkKeymap *keymap, gpointer data)
> > +{
> > +    SpiceGtkSession *self = data;
> > +
> > +    spice_gtk_session_sync_keyboard_layout(self);
> > +}
> > +
> >  static void keymap_modifiers_changed(GdkKeymap *keymap, gpointer data)
> >  {
> >      SpiceGtkSession *self = data;
> > @@ -207,6 +214,8 @@ static void spice_gtk_session_init(SpiceGtkSession
> > *self)
> >                       G_CALLBACK(clipboard_owner_change), self);
> >      spice_g_signal_connect_object(keymap, "state-changed",
> >                                    G_CALLBACK(keymap_modifiers_changed),
> >                                    self, 0);
> > +    spice_g_signal_connect_object(keymap, "keys-changed",
> > +                                  G_CALLBACK(keymap_layout_changed), self,
> > 0);
> >  }
> >  
> >  static GObject *
> > @@ -1001,6 +1010,15 @@ static void channel_new(SpiceSession *session,
> > SpiceChannel *channel,
> >                           G_CALLBACK(clipboard_request), self);
> >          g_signal_connect(channel, "main-clipboard-selection-release",
> >                           G_CALLBACK(clipboard_release), self);
> > +        gboolean agent_connected;
> 
> I don't know if we have rules regarding declaring variables in the
> middle of a block, this is some C99 thing which some module prefer to
> avoid in case they are built with a C89 compiler.
> 
> > +        g_object_get(channel, "agent-connected", &agent_connected, NULL);
> > +        if (agent_connected) {
> > +            spice_gtk_session_sync_keyboard_layout(self);
> > +        } else {
> > +            g_signal_connect_swapped(channel, "notify::agent-connected",
> > +
> > G_CALLBACK(spice_gtk_session_sync_keyboard_layout),
> > +                                     self);
> > +        }
> >      }
> >      if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> >          spice_g_signal_connect_object(channel, "inputs-modifiers",
> > @@ -1195,3 +1213,48 @@ gboolean
> > spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self)
> >  
> >      return self->priv->pointer_grabbed;
> >  }
> > +
> > +static gchar *get_keyboard_description(void)
> > +{
> > +    gchar *keyboard_description = NULL;
> > +#ifdef HAVE_X11_XKBLIB_H
> > +    Display *dpy;
> > +    XkbDescPtr xkb;
> > +    int major = XkbMajorVersion, minor = XkbMinorVersion;
> > +
> > +    dpy = XkbOpenDisplay(NULL, NULL, NULL, &major, &minor, NULL);
> > +    g_return_val_if_fail(dpy != NULL, NULL);
> > +
> > +    xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, XkbUseCoreKbd);
> > +    g_warn_if_fail(xkb != NULL);
> > +    if (xkb != NULL) {
> > +        keyboard_description = g_strdelimit(
> > +            g_strdup_printf("%s", XGetAtomName(dpy, xkb->names->symbols)),
> > "_",'+');
> 
> It seems you need to call XFree on the value returned by XGetAtomName.
> This has the added advantage of splitting this very long line with
> multiple function calls ;)
> 
> > +        XkbFreeKeyboard(xkb, XkbAllComponentsMask, True);
> > +    }
> > +    XCloseDisplay(dpy);
> > +#elif defined(G_OS_WIN32)
> > +    gchar  *locale, *region;
> > +    locale = g_win32_getlocale();
> 
> I'd feel more comfortable with a g_return_val_if_fail (g_strrstr(locale,
> "_"), NULL);
> here.
> 
> > +    region = g_ascii_strdown(g_strrstr(locale, "_")+1, -1);
> > +    g_free(locale);
> > +    keyboard_description = g_strdup_printf("pc+%s", region);
> > +    g_free(region);
> > +#endif
> 
> Maybe add
> #else
>     g_debug("get_keyboard_description unimplemented on this platform");
> #endif
> so that we know about it? (or g_warning for something more invasive)
> 
I would prefer g_warning (user has to enable this behavior, so he should be warned that it is not working).

> Looks all good otherwise!
> 
> Christophe
> 

Thank you for the review, I will send a new version.

Pavel