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

[Olivier Fourdan]

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.”

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

Cheers,
Olivier
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180308/35da7506/attachment-0001.html>