GNOME Bugzilla – Bug 705710
evdev: fix X11 to evdev keycode translation
Last modified: 2013-08-16 14:28:46 UTC
Hardware keycodes in Clutter events are x11 keycodes, which are the same as evdev + 8, but we need to reverse the translation when explicitly asked for an evdev keycode.
Created attachment 251219 [details] [review] evdev: fix X11 to evdev keycode translation
Created attachment 251227 [details] [review] evdev: fix xkb_state handling We must pass X11 keycodes, not evdev ones, to libxkbcommon, otherwise the modifier state is wrong.
Created attachment 251228 [details] [review] evdev: remove dead code ClutterDeviceManager uses g_object_new directly, to pass the necessary properties down.
Created attachment 251229 [details] [review] evdev: add master / slave device handling All evdev devices are slave devices, which means that xkb state and pointer position must be shared by emulating a core keyboard and a core pointer. Also, we must make sure to add all modifier state (keyboard and button) to our events.
Created attachment 251247 [details] [review] evdev: allow hooking directly into libxkbcommon A wayland compositor needs to have more keyboard state than ClutterModifierState exposes, so it makes sense for it to use xkb_state directly. Also, it makes sense for it to provide it's own keymap, to ensure a consistent view between the compositor and the wayland clients.
Created attachment 251248 [details] [review] evdev: don't update xkb state for autorepeated keys xkb_state_update_key() needs to be called only on state transitions, otherwise the state tracking gets confused and locks certain modifiers forever.
Created attachment 251249 [details] [review] evdev: implement wheel events Mouse wheel events come as EV_REL/REL_WHEEL, and we can convert them to clutter events on the assumption that scrolling with the wheel is always vertical.
I assume this is needed to make keyboard input work with Wayland in 3.10 ?
Yes, although the future is libinputcommon, which might replace a good amount of the current evdev backend. Not sure if that will be ready for 3.10
Review of attachment 251219 [details] [review]: looks good.
Review of attachment 251227 [details] [review]: okay.
Review of attachment 251228 [details] [review]: okay.
Review of attachment 251229 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +111,3 @@ + ClutterDeviceManagerEvdev *ret; + + g_object_get (device, "device-manager", &ret, NULL); you can access the InputDevice structure directly, here. it's all private to Clutter. @@ +621,3 @@ + id = INVALID_SLAVE_ID; + sscanf(device_file, "/dev/input/event%d", &id); please, check that sscanf() worked. also, coding style: needs a space between the function and the parenthesis. @@ +737,3 @@ + if (clutter_input_device_get_device_mode (device) == + CLUTTER_INPUT_MODE_SLAVE) don't break the condition. @@ +741,3 @@ + /* Install the GSource for this device */ + source = clutter_event_source_new (device_evdev); + if (G_LIKELY (source)) can it really fail? I'd probably assert() in case of NULL. @@ +761,3 @@ + if (clutter_input_device_get_device_mode (device) == + CLUTTER_INPUT_MODE_SLAVE) don't break the condition.
Review of attachment 251247 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +1221,3 @@ + * + * Instructs @evdev to use the speficied keyboard map. This will cause + * the backend to drop the state and create a new one with the new map, the documentation ought to be a bit clearer on the role of this function. for instance, if you expect people to change the struct xkb_keymap returned by get_keyboard_state() then what's the point of being able to set a new one? if you don't expect that to happen, then the get_keyboard_state() function should return a const struct xkb_state* instead.
Review of attachment 251248 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +201,3 @@ + /* 2 means autorepeat (see Documentation/input/input.txt from the kernel tree). + We must be careful and not pass multiple releases to xkb, otherwise it gets + confused and locks the modifiers */ we should probably use a #defined constant, instead of a magic value.
Review of attachment 251249 [details] [review]: okay, though you should add a big fat warning for non-vertical scroll wheels.
(In reply to comment #14) > Review of attachment 251247 [details] [review]: > > ::: clutter/evdev/clutter-device-manager-evdev.c > @@ +1221,3 @@ > + * > + * Instructs @evdev to use the speficied keyboard map. This will cause > + * the backend to drop the state and create a new one with the new map, > > the documentation ought to be a bit clearer on the role of this function. > > for instance, if you expect people to change the struct xkb_keymap returned by > get_keyboard_state() then what's the point of being able to set a new one? if > you don't expect that to happen, then the get_keyboard_state() function should > return a const struct xkb_state* instead. You can set a xkb_keymap, and retrieve an xkb_state. You need a xkb_keymap to create the xkb_state, and you need to the xkb_state to get detailed modifiers. But clutter needs the xkb_state too, because it updates it as events come. In general, the xkb_state would be readonly, but when you create a new xkb_state in clutter, you don't want to lose the information of which keys are down. Because the application already tracks those, it is allowed to set the keymap, retrieve the new state (which will be a different object) and update_key() for each key down. Maybe it is better if clutter tracks which keys are down too. (In reply to comment #16) > Review of attachment 251249 [details] [review]: > > okay, though you should add a big fat warning for non-vertical scroll wheels. A warning in what form?
(In reply to comment #17) > (In reply to comment #14) > > Review of attachment 251247 [details] [review] [details]: > > > > ::: clutter/evdev/clutter-device-manager-evdev.c > > @@ +1221,3 @@ > > + * > > + * Instructs @evdev to use the speficied keyboard map. This will cause > > + * the backend to drop the state and create a new one with the new map, > > > > the documentation ought to be a bit clearer on the role of this function. > > > > for instance, if you expect people to change the struct xkb_keymap returned by > > get_keyboard_state() then what's the point of being able to set a new one? if > > you don't expect that to happen, then the get_keyboard_state() function should > > return a const struct xkb_state* instead. > > You can set a xkb_keymap, and retrieve an xkb_state. > You need a xkb_keymap to create the xkb_state, and you need to the xkb_state to > get detailed modifiers. But clutter needs the xkb_state too, because it updates > it as events come. > In general, the xkb_state would be readonly, but when you create a new > xkb_state in clutter, you don't want to lose the information of which keys are > down. Because the application already tracks those, it is allowed to set the > keymap, retrieve the new state (which will be a different object) and > update_key() for each key down. > Maybe it is better if clutter tracks which keys are down too. since we're adding API for display managers/compositors, then we should probably just make it Do The Right Thing™, and provide an eventual escape hatch if and only if somebody asks for it. > (In reply to comment #16) > > Review of attachment 251249 [details] [review] [details]: > > > > okay, though you should add a big fat warning for non-vertical scroll wheels. > > A warning in what form? a comment near the code saying that we assume REL_WHEEL is vertical only and if we ever need to support horizontal scrolling wheels then we'll either need more details from evdev or a new enumeration value.
Created attachment 251515 [details] [review] evdev: add master / slave device handling All evdev devices are slave devices, which means that xkb state and pointer position must be shared by emulating a core keyboard and a core pointer. Also, we must make sure to add all modifier state (keyboard and button) to our events.
Created attachment 251516 [details] [review] evdev: allow hooking directly into libxkbcommon A wayland compositor needs to have more keyboard state than ClutterModifierState exposes, so it makes sense for it to use xkb_state directly. Also, it makes sense for it to provide it's own keymap, to ensure a consistent view between the compositor and the wayland clients.
Created attachment 251517 [details] [review] evdev: don't update xkb state for autorepeated keys xkb_state_update_key() needs to be called only on state transitions, otherwise the state tracking gets confused and locks certain modifiers forever.
Created attachment 251518 [details] [review] evdev: implement wheel events Mouse wheel events come as EV_REL/REL_WHEEL, and we can convert them to clutter events on the assumption that scrolling with the wheel is always vertical.
Review of attachment 251515 [details] [review]: looks okay.
Review of attachment 251516 [details] [review]: typo in the commit message: "it's own" instead of "its own". other than that, looks okay.
Review of attachment 251517 [details] [review]: looks okay.
Review of attachment 251518 [details] [review]: looks okay.
Attachment 251219 [details] pushed as a3557f7 - evdev: fix X11 to evdev keycode translation Attachment 251227 [details] pushed as d844cf5 - evdev: fix xkb_state handling Attachment 251228 [details] pushed as f749858 - evdev: remove dead code Attachment 251515 [details] pushed as 7865322 - evdev: add master / slave device handling Attachment 251516 [details] pushed as 8c358f1 - evdev: allow hooking directly into libxkbcommon Attachment 251517 [details] pushed as 7b780b0 - evdev: don't update xkb state for autorepeated keys Attachment 251518 [details] pushed as 0e519e2 - evdev: implement wheel events