[Spice-devel] [PATCH] Use scancode instead of keycode names

[Daniel P. Berrangé]

On Thu, Mar 08, 2018 at 05:22:01PM +0100, Olivier Fourdan wrote:
> Hi,
> 
> On Thu, Mar 8, 2018 at 4:27 PM, Daniel P. Berrangé <berrange at redhat.com>
> wrote:
> 
> > On Thu, Mar 08, 2018 at 04:19:03PM +0100, Olivier Fourdan wrote:
> > > When running on Xwayland, the keycode mapping property is not available,
> > > which causes unknown keycode mapping errors and the keyboard doesn't
> > > work.
> > >
> > > Use a known scancode (“XK_Page_Up”) to check against the AT scancode and
> > > use “xfree86” if it matches or assume “evdev” for anything else.
> > >
> > > Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> > > ---
> > >  src/vncdisplaykeymap.c | 33 ++++-----------------------------
> > >  1 file changed, 4 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/src/vncdisplaykeymap.c b/src/vncdisplaykeymap.c
> > > index 9ee501d..135412f 100644
> > > --- a/src/vncdisplaykeymap.c
> > > +++ b/src/vncdisplaykeymap.c
> > > @@ -162,8 +162,6 @@ const guint16 *vnc_display_keymap_gdk2xtkbd_table(GdkWindow
> > *window,
> > >  {
> > >  #ifdef GDK_WINDOWING_X11
> > >       if (GDK_IS_X11_WINDOW(window)) {
> > > -             XkbDescPtr desc;
> > > -             const gchar *keycodes = NULL;
> > >                  GdkDisplay *dpy = gdk_window_get_display(window);
> > >
> > >               /* There is no easy way to determine what X11 server
> > > @@ -175,17 +173,6 @@ const guint16 *vnc_display_keymap_gdk2xtkbd_table(GdkWindow
> > *window,
> > >                */
> > >
> > >               Display *xdisplay = gdk_x11_display_get_xdisplay(dpy);
> > > -             desc = XkbGetMap(xdisplay,
> > > -                                   XkbGBN_AllComponentsMask,
> > > -                                   XkbUseCoreKbd);
> > > -             if (desc) {
> > > -                     if (XkbGetNames(xdisplay, XkbKeycodesNameMask,
> > desc) == Success) {
> > > -                             keycodes = gdk_x11_get_xatom_name(desc->
> > names->keycodes);
> > > -                             if (!keycodes)
> > > -                                     g_warning("could not lookup
> > keycode name");
> > > -                     }
> > > -                     XkbFreeKeyboard(desc, XkbGBN_AllComponentsMask,
> > True);
> > > -             }
> > >
> > >               if (check_for_xwin(dpy)) {
> > >                       VNC_DEBUG("Using xwin keycode mapping");
> > > @@ -195,26 +182,14 @@ const guint16 *vnc_display_keymap_gdk2xtkbd_table(GdkWindow
> > *window,
> > >                       VNC_DEBUG("Using xquartz keycode mapping");
> > >                       *maplen = G_N_ELEMENTS(keymap_xorgxquartz2xtkbd);
> > >                       return keymap_xorgxquartz2xtkbd;
> > > -             } else if (keycodes && STRPREFIX(keycodes, "evdev")) {
> > > -                     VNC_DEBUG("Using evdev keycode mapping");
> > > -                     *maplen = G_N_ELEMENTS(keymap_xorgevdev2xtkbd);
> > > -                     return keymap_xorgevdev2xtkbd;
> > > -             } else if (keycodes && STRPREFIX(keycodes, "xfree86")) {
> > > +             } else if (XKeysymToKeycode(xdisplay, XK_Page_Up) == 0x63)
> > {
> > >                       VNC_DEBUG("Using xfree86 keycode mapping");
> > >                       *maplen = G_N_ELEMENTS(keymap_xorgkbd2xtkbd);
> > >                       return keymap_xorgkbd2xtkbd;
> > >               } else {
> > > -                     g_warning("Unknown keycode mapping '%s'.\n"
> > > -                               "Please report to gtk-vnc-list at gnome.org
> > \n"
> > > -                               "including the following information:\n"
> > > -                               "\n"
> > > -                               "  - Operating system\n"
> > > -                               "  - GDK build\n"
> > > -                               "  - X11 Server\n"
> > > -                               "  - xprop -root\n"
> > > -                               "  - xdpyinfo\n",
> > > -                               keycodes);
> > > -                     return NULL;
> > > +                     VNC_DEBUG("Using evdev keycode mapping");
> > > +                     *maplen = G_N_ELEMENTS(keymap_xorgevdev2xtkbd);
> > > +                     return keymap_xorgevdev2xtkbd;
> >
> > I don't think this is desirable logic - blindly assuming everything
> > else is evdev is not valid - that is only true of an X server running
> > on a Linux host. Thus this has the effect of hiding the important
> > error message alerting us to use of other X servers we need to handle
> > and silently giving those users incorrect keymaps.
> >
> 
> This is what was suggested in downstream bug
> https://bugzilla.redhat.com/show_bug.cgi?id=1479682
> 
> If I may, I would point out at
> https://bugzilla.redhat.com/show_bug.cgi?id=1479682#c32 where you wrote:
> 
> “Oh, that's a nice idea, we can certainly try that approach. Just
> distinguishing AT from evdev is sufficient in this case.”

I'm happy to admit I was wrong when writing that, but I also wasn't
thinking we would delete the existing code that checks 'keycodes'
name, but rather augment it.

> Besides, the current code is not only broken, it also leaves the user with
> no working keyboard at all...

Surely we can put in a positive check for XKeysymToKeycode() value matching
that expected for evdev, and then still keep the existing fallback warning
message so we continue to detect broken platforms

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|