On Tue, Jul 20, 2010 at 9:01 PM, Ping Cheng <[email protected]> wrote:
> 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.
It might be easier for you to amend the patch directly. Basically, we
can use "ctrl + mouse button 4/5" for zooming. So, it would be
something like the following:
button = (dist > 0) ? SCROLL_UP : SCROLL_DOWN;
......
wcmEmitKeycode (priv->local->dev, 37 /*XK_Control_L*/, 1);
while (count--)
{
xf86WcmSendButtonClick(priv, button, 1);
xf86WcmSendButtonClick(priv, button, 0);
}
wcmEmitKeycode (priv->local->dev, 37 /*XK_Control_L*/, 0);
This ugly gesture code will stay until the proper solution comes along....
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
