On Tue, Jul 20, 2010 at 8:48 PM, Peter Hutterer <[email protected]> wrote: > This addresses a hang in the driver when buttons are configured to send > keystrokes. The current code calls XkbGetCoreMap() which allocates during > the signal handler, causing server hangs. > > This patch changes the driver to use keycodes instead of keysyms. There are > a number of reasons for this: > - XkbCoreMap() only hands us a core keymap, not the XKB one that everyone > uses these days. There are some differences between the core and the XKB > map (mainly the use of modifiers), hence we should be focusing on the XKB > map anyway. > - Parsing the XKB map manually in the driver is complicated and not > something we want to do, especially when trying to maintain ABI compat > with multiple X server versions. On the other hand, the client-side API > for XKB is frozen like all of Xlib. > - The driver is not notified of XKB layout changes, thus a cached map would > potentially be out of date. Moving keysym to keycode conversion to the > client avoids this. > - The server does not communicate through keysyms to any client, keycodes > are the only information. It is up to the client to pick the keysym (and > glyph) based on the keycode. > > Note that this changes the property API. Clients that would write straight > to a button mapping property will need to change. AFAICT, only xsetwacom > does this at this point and xsetwacom is updated with this patch. > > Note that the current xsetwacom implementation will only focus on level 0 > keys. It will not automatically fill in the modifier states. Thus, to get > e.g. a string of "aBc", the following configuration is required: > xsetwacom set "device" "Button1" "key a +shift B -shift c" > > xsetwacom releases modifiers at the end of the configuration, thus to get a > single uppercase letter (or multiple letters), the following is enough: > xsetwacom set "device" "Button1" "key shift A" > > Note: this breaks Ctrl+/- zoom support for keyboard layouts that have +/- on > a different key than the 'us' keyboard layout. Did I mention that hacking up > gestures in the driver is a bad idea? > > Signed-off-by: Peter Hutterer <[email protected]>
Acked-by: Ping Cheng <[email protected]> > --- > src/wcmCommon.c | 103 ++++--------------------------------------------- > src/wcmTouchFilter.c | 18 ++++++--- > src/xf86Wacom.h | 2 +- > tools/xsetwacom.c | 67 +++++++++++++++++++++++++------- > 4 files changed, 74 insertions(+), 116 deletions(-) > > diff --git a/src/wcmCommon.c b/src/wcmCommon.c > index 2185725..4516cc6 100644 > --- a/src/wcmCommon.c > +++ b/src/wcmCommon.c > @@ -318,94 +318,9 @@ static void wcmSendButtons(LocalDevicePtr local, int > buttons, int rx, int ry, > } > } > > -/***************************************************************************** > - * wcmEmitKeysym -- > - * Emit a keydown/keyup event > - > ****************************************************************************/ > -static int ODDKEYSYM [][2] = > +void wcmEmitKeycode (DeviceIntPtr keydev, int keycode, int state) > { > - { XK_asciitilde, XK_grave }, > - { XK_exclam, XK_1 }, > - { XK_at, XK_2 }, > - { XK_numbersign, XK_3 }, > - { XK_dollar, XK_4 }, > - { XK_percent, XK_5 }, > - { XK_asciicircum, XK_6 }, > - { XK_ampersand, XK_7 }, > - { XK_asterisk, XK_8 }, > - { XK_parenleft, XK_9 }, > - { XK_parenright, XK_0 }, > - { XK_underscore, XK_minus }, > - { XK_plus, XK_equal }, > - { XK_braceleft, XK_bracketleft }, > - { XK_braceright, XK_bracketright }, > - { XK_colon, XK_semicolon }, > - { XK_quotedbl, XK_quoteright }, > - { XK_less, XK_comma }, > - { XK_greater, XK_period }, > - { XK_question, XK_slash }, > - { XK_bar, XK_backslash }, > - { 0, 0} > -}; > - > -void wcmEmitKeysym (DeviceIntPtr keydev, int keysym, int state) > -{ > - int i, j, alt_keysym = 0; > - > - /* Now that we have the keycode look for key index */ > -#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7 > - KeySymsRec *ksr = XkbGetCoreMap(keydev); > -#else > - KeySymsRec *ksr = &keydev->key->curKeySyms; > -#endif > - > - for (i = ksr->minKeyCode; i <= ksr->maxKeyCode; i++) > - if (ksr->map [(i - ksr->minKeyCode) * ksr->mapWidth] == > keysym) > - break; > - > - if (i > ksr->maxKeyCode) > - { > - if (isupper(keysym)) > - alt_keysym = tolower(keysym); > - else > - { > - j = 0; > - while (ODDKEYSYM [j][0]) > - { > - if (ODDKEYSYM [j][0] == keysym) > - { > - alt_keysym = ODDKEYSYM [j][1]; > - break; > - } > - j++; > - } > - } > - if ( alt_keysym ) > - { > - for (j = ksr->minKeyCode; j <= ksr->maxKeyCode; j++) > - if (ksr->map [(j - ksr->minKeyCode) * > ksr->mapWidth] == XK_Shift_L) > - break; > - if (state) > - xf86PostKeyboardEvent (keydev, j, 1); > - for (i = ksr->minKeyCode; i <= ksr->maxKeyCode; i++) > - if (ksr->map [(i - ksr->minKeyCode) * > ksr->mapWidth] == alt_keysym) > - break; > - xf86PostKeyboardEvent (keydev, i, state); > - if (!state) > - xf86PostKeyboardEvent (keydev, j, 0); > - } > - else > - xf86Msg (X_WARNING, "%s: Couldn't find key with code > %08x on keyboard device %s\n", > - keydev->name, keysym, keydev->name); > -#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7 > - free(ksr); > -#endif > - return; > - } > - xf86PostKeyboardEvent (keydev, i, state); > -#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7 > - free(ksr); > -#endif > + xf86PostKeyboardEvent (keydev, keycode, state); > } > > static void toggleDisplay(LocalDevicePtr local) > @@ -512,9 +427,9 @@ static void sendAButton(LocalDevicePtr local, int button, > int mask, > break; > case AC_KEY: > { > - int key_sym = (action & AC_CODE); > + int key_code = (action & AC_CODE); > int is_press = (action & > AC_KEYBTNPRESS); > - wcmEmitKeysym(local->dev, key_sym, > is_press); > + wcmEmitKeycode(local->dev, key_code, > is_press); > } > break; > case AC_MODETOGGLE: > @@ -565,15 +480,15 @@ static void sendAButton(LocalDevicePtr local, int > button, int mask, > break; > case AC_KEY: > { > - int key_sym = (action & AC_CODE); > + int key_code = (action & AC_CODE); > > /* don't care about releases here */ > if (!(action & AC_KEYBTNPRESS)) > break; > > - if (countPresses(key_sym, > &priv->keys[button][i], > + if (countPresses(key_code, > &priv->keys[button][i], > > ARRAY_SIZE(priv->keys[button]) - i)) > - wcmEmitKeysym(local->dev, > key_sym, 0); > + wcmEmitKeycode(local->dev, > key_code, 0); > } > } > > @@ -688,8 +603,8 @@ static void sendWheelStripEvents(LocalDevicePtr local, > const WacomDeviceState* d > break; > > case AC_KEY: > - wcmEmitKeysym(local->dev, (fakeButton & AC_CODE), 1); > - wcmEmitKeysym(local->dev, (fakeButton & AC_CODE), 0); > + wcmEmitKeycode(local->dev, (fakeButton & AC_CODE), 1); > + wcmEmitKeycode(local->dev, (fakeButton & AC_CODE), 0); > break; > > default: > diff --git a/src/wcmTouchFilter.c b/src/wcmTouchFilter.c > index 246b33c..7d3f860 100644 > --- a/src/wcmTouchFilter.c > +++ b/src/wcmTouchFilter.c > @@ -467,17 +467,23 @@ static void wcmFingerZoom(WacomDevicePtr priv) > return; > } > > - /* zooming? */ > - key = (dist > 0) ? XK_plus : XK_minus; > + /* zooming? > + FIXME: this hardcodes the positions of ctrl, + and - to the ones on > + the us keyboard layout. Tough luck. The alternative is to run > + through the XKB table and figure out where +/- are hiding. Good > + luck. > + Gesture support is not supposed to be in the driver... > + */ > + key = (dist > 0) ? 21 /*XK_plus*/ : 20 /*XK_minus*/; I have a solution for this one :). I'll make a patch soon. Ping > count -= common->wcmGestureParameters.wcmGestureUsed; > common->wcmGestureParameters.wcmGestureUsed += count; > while (count--) > { > - wcmEmitKeysym (priv->local->dev, XK_Control_L, 1); > - wcmEmitKeysym (priv->local->dev, key, 1); > - wcmEmitKeysym (priv->local->dev, key, 0); > - wcmEmitKeysym (priv->local->dev, XK_Control_L, 0); > + wcmEmitKeycode (priv->local->dev, 37 /*XK_Control_L*/, 1); > + wcmEmitKeycode (priv->local->dev, key, 1); > + wcmEmitKeycode (priv->local->dev, key, 0); > + wcmEmitKeycode (priv->local->dev, 37 /*XK_Control_L*/, 0); > } > } > > diff --git a/src/xf86Wacom.h b/src/xf86Wacom.h > index 2ec55b6..2067790 100644 > --- a/src/xf86Wacom.h > +++ b/src/xf86Wacom.h > @@ -161,7 +161,7 @@ extern int wcmDevSwitchMode(ClientPtr client, > DeviceIntPtr dev, int mode); > extern void wcmChangeScreen(LocalDevicePtr local, int value); > extern void wcmTilt2R(WacomDeviceStatePtr ds); > extern void wcmGestureFilter(WacomDevicePtr priv, int channel); > -extern void wcmEmitKeysym(DeviceIntPtr keydev, int keysym, int state); > +extern void wcmEmitKeycode(DeviceIntPtr keydev, int keycode, int state); > extern void wcmSoftOutEvent(LocalDevicePtr local); > > extern void wcmRotateTablet(LocalDevicePtr local, int value); > diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c > index 59f1270..23136af 100644 > --- a/tools/xsetwacom.c > +++ b/tools/xsetwacom.c > @@ -34,6 +34,7 @@ > #include <X11/Xlib.h> > #include <X11/Xatom.h> > #include <X11/extensions/XInput.h> > +#include <X11/XKBlib.h> > > #define TRACE(...) \ > if (verbose) fprintf(stderr, "... " __VA_ARGS__) > @@ -1197,16 +1198,16 @@ static int is_modifier(const char* modifier) > return 0; > } > > -static int special_map_keystrokes(int argc, char **argv, unsigned long > *ndata, unsigned long* data); > -static int special_map_button(int argc, char **argv, unsigned long *ndata, > unsigned long* data); > -static int special_map_core(int argc, char **argv, unsigned long *ndata, > unsigned long *data); > -static int special_map_modetoggle(int argc, char **argv, unsigned long > *ndata, unsigned long *data); > -static int special_map_displaytoggle(int argc, char **argv, unsigned long > *ndata, unsigned long *data); > +static int special_map_keystrokes(Display *dpy, int argc, char **argv, > unsigned long *ndata, unsigned long* data); > +static int special_map_button(Display *dpy, int argc, char **argv, unsigned > long *ndata, unsigned long* data); > +static int special_map_core(Display *dpy, int argc, char **argv, unsigned > long *ndata, unsigned long *data); > +static int special_map_modetoggle(Display *dpy, int argc, char **argv, > unsigned long *ndata, unsigned long *data); > +static int special_map_displaytoggle(Display *dpy, int argc, char **argv, > unsigned long *ndata, unsigned long *data); > > /* Valid keywords for the --set ButtonX options */ > struct keywords { > const char *keyword; > - int (*func)(int, char **, unsigned long*, unsigned long *); > + int (*func)(Display*, int, char **, unsigned long*, unsigned long *); > } keywords[] = { > {"key", special_map_keystrokes}, > {"button", special_map_button}, > @@ -1218,7 +1219,7 @@ struct keywords { > > /* the "core" keyword isn't supported anymore, we just have this here to > tell people that. */ > -static int special_map_core(int argc, char **argv, unsigned long *ndata, > unsigned long *data) > +static int special_map_core(Display *dpy, int argc, char **argv, unsigned > long *ndata, unsigned long *data) > { > static int once_only = 1; > if (once_only) > @@ -1230,7 +1231,7 @@ static int special_map_core(int argc, char **argv, > unsigned long *ndata, unsigne > return 0; > } > > -static int special_map_modetoggle(int argc, char **argv, unsigned long > *ndata, unsigned long *data) > +static int special_map_modetoggle(Display *dpy, int argc, char **argv, > unsigned long *ndata, unsigned long *data) > { > data[*ndata] = AC_MODETOGGLE; > > @@ -1239,7 +1240,7 @@ static int special_map_modetoggle(int argc, char > **argv, unsigned long *ndata, u > return 0; > } > > -static int special_map_displaytoggle(int argc, char **argv, unsigned long > *ndata, unsigned long *data) > +static int special_map_displaytoggle(Display *dpy, int argc, char **argv, > unsigned long *ndata, unsigned long *data) > { > data[*ndata] = AC_DISPLAYTOGGLE; > > @@ -1261,7 +1262,7 @@ static inline int is_valid_keyword(const char *keyword) > return 0; > } > > -static int special_map_button(int argc, char **argv, unsigned long *ndata, > unsigned long *data) > +static int special_map_button(Display *dpy, int argc, char **argv, unsigned > long *ndata, unsigned long *data) > { > int nitems = 0; > int i; > @@ -1306,11 +1307,44 @@ static int special_map_button(int argc, char **argv, > unsigned long *ndata, unsig > return nitems; > } > > +/* Return the first keycode to have the required keysym in the current group. > + TODOs: > + - parse other groups as well (do we need this?) > + - for keysyms not on level 0, return the keycodes for the modifiers as > + well > +*/ > +static int keysym_to_keycode(Display *dpy, KeySym sym) > +{ > + static XkbDescPtr xkb = NULL; > + XkbStateRec state; > + int group; > + int kc = 0; > + > + > + if (!xkb) > + xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, > XkbUseCoreKbd); > + XkbGetState(dpy, XkbUseCoreKbd, &state); > + group = state.group; > + > + for (kc = xkb->min_key_code; kc <= xkb->max_key_code; kc++) > + { > + KeySym* ks; > + int i; > + > + ks = XkbKeySymsPtr(xkb, kc); > + for (i = 0; i < XkbKeyGroupWidth(xkb, kc, state.group); i++) > + if (ks[i] == sym) > + goto out; > + } > + > +out: > + return kc; > +} > /* > Map gibberish like "ctrl alt f2" into the matching AC_KEY values. > Returns 1 on success or 0 otherwise. > */ > -static int special_map_keystrokes(int argc, char **argv, unsigned long > *ndata, unsigned long* data) > +static int special_map_keystrokes(Display *dpy, int argc, char **argv, > unsigned long *ndata, unsigned long* data) > { > int i; > int nitems = 0; > @@ -1318,6 +1352,7 @@ static int special_map_keystrokes(int argc, char > **argv, unsigned long *ndata, u > for (i = 0; i < argc; i++) > { > KeySym ks; > + KeyCode kc; > int need_press = 0, need_release = 0; > char *key = argv[i]; > > @@ -1358,12 +1393,14 @@ static int special_map_keystrokes(int argc, char > **argv, unsigned long *ndata, u > need_press = need_release = 1; > > ks = XStringToKeysym(key); > + kc = keysym_to_keycode(dpy, ks); > + > if (need_press) > - data[*ndata + nitems++] = AC_KEY | AC_KEYBTNPRESS | > ks; > + data[*ndata + nitems++] = AC_KEY | AC_KEYBTNPRESS | > kc; > if (need_release) > - data[*ndata + nitems++] = AC_KEY | ks; > + data[*ndata + nitems++] = AC_KEY | kc; > > - TRACE("Key map %ld ('%s') [%s,%s]\n", ks, > + TRACE("Key map %ld (%d, '%s') [%s,%s]\n", ks, kc, > XKeysymToString(ks), > need_press ? "press" : "", > need_release ? "release" : ""); > @@ -1481,7 +1518,7 @@ static void special_map_buttons(Display *dpy, XDevice > *dev, param_t* param, int > int parsed = 0; > if (strcasecmp(words[i], keywords[j].keyword) == 0) > { > - parsed = keywords[j].func(nwords - i - 1, > + parsed = keywords[j].func(dpy, nwords - i - 1, > &words[i + 1], > &nitems, data); > i += parsed; > -- > 1.7.1 > ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
