After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705710 - evdev: fix X11 to evdev keycode translation
evdev: fix X11 to evdev keycode translation
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks:
 
 
Reported: 2013-08-09 08:11 UTC by Giovanni Campagna
Modified: 2013-08-16 14:28 UTC
See Also:
GNOME target: 3.10
GNOME version: ---


Attachments
evdev: fix X11 to evdev keycode translation (1.39 KB, patch)
2013-08-09 08:11 UTC, Giovanni Campagna
committed Details | Review
evdev: fix xkb_state handling (1.12 KB, patch)
2013-08-09 10:14 UTC, Giovanni Campagna
committed Details | Review
evdev: remove dead code (1.77 KB, patch)
2013-08-09 10:14 UTC, Giovanni Campagna
committed Details | Review
evdev: add master / slave device handling (19.60 KB, patch)
2013-08-09 10:14 UTC, Giovanni Campagna
reviewed Details | Review
evdev: allow hooking directly into libxkbcommon (3.24 KB, patch)
2013-08-09 16:44 UTC, Giovanni Campagna
reviewed Details | Review
evdev: don't update xkb state for autorepeated keys (1.43 KB, patch)
2013-08-09 16:44 UTC, Giovanni Campagna
reviewed Details | Review
evdev: implement wheel events (2.53 KB, patch)
2013-08-09 16:44 UTC, Giovanni Campagna
accepted-commit_now Details | Review
evdev: add master / slave device handling (19.21 KB, patch)
2013-08-13 16:13 UTC, Giovanni Campagna
committed Details | Review
evdev: allow hooking directly into libxkbcommon (4.77 KB, patch)
2013-08-13 16:13 UTC, Giovanni Campagna
committed Details | Review
evdev: don't update xkb state for autorepeated keys (2.22 KB, patch)
2013-08-13 16:13 UTC, Giovanni Campagna
committed Details | Review
evdev: implement wheel events (2.68 KB, patch)
2013-08-13 16:13 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-08-09 08:11:35 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.
Comment 1 Giovanni Campagna 2013-08-09 08:11:38 UTC
Created attachment 251219 [details] [review]
evdev: fix X11 to evdev keycode translation
Comment 2 Giovanni Campagna 2013-08-09 10:14:09 UTC
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.
Comment 3 Giovanni Campagna 2013-08-09 10:14:13 UTC
Created attachment 251228 [details] [review]
evdev: remove dead code

ClutterDeviceManager uses g_object_new directly, to pass the
necessary properties down.
Comment 4 Giovanni Campagna 2013-08-09 10:14:17 UTC
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.
Comment 5 Giovanni Campagna 2013-08-09 16:44:45 UTC
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.
Comment 6 Giovanni Campagna 2013-08-09 16:44:49 UTC
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.
Comment 7 Giovanni Campagna 2013-08-09 16:44:52 UTC
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.
Comment 8 Matthias Clasen 2013-08-10 15:14:33 UTC
I assume this is needed to make keyboard input work with Wayland in 3.10 ?
Comment 9 Giovanni Campagna 2013-08-10 15:18:31 UTC
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
Comment 10 Emmanuele Bassi (:ebassi) 2013-08-12 17:29:22 UTC
Review of attachment 251219 [details] [review]:

looks good.
Comment 11 Emmanuele Bassi (:ebassi) 2013-08-12 17:29:55 UTC
Review of attachment 251227 [details] [review]:

okay.
Comment 12 Emmanuele Bassi (:ebassi) 2013-08-12 17:30:13 UTC
Review of attachment 251228 [details] [review]:

okay.
Comment 13 Emmanuele Bassi (:ebassi) 2013-08-12 17:35:03 UTC
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.
Comment 14 Emmanuele Bassi (:ebassi) 2013-08-12 17:38:46 UTC
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.
Comment 15 Emmanuele Bassi (:ebassi) 2013-08-12 17:39:28 UTC
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.
Comment 16 Emmanuele Bassi (:ebassi) 2013-08-12 17:42:05 UTC
Review of attachment 251249 [details] [review]:

okay, though you should add a big fat warning for non-vertical scroll wheels.
Comment 17 Giovanni Campagna 2013-08-13 07:41:16 UTC
(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?
Comment 18 Emmanuele Bassi (:ebassi) 2013-08-13 12:58:12 UTC
(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.
Comment 19 Giovanni Campagna 2013-08-13 16:13:02 UTC
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.
Comment 20 Giovanni Campagna 2013-08-13 16:13:06 UTC
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.
Comment 21 Giovanni Campagna 2013-08-13 16:13:10 UTC
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.
Comment 22 Giovanni Campagna 2013-08-13 16:13:14 UTC
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.
Comment 23 Emmanuele Bassi (:ebassi) 2013-08-16 13:12:39 UTC
Review of attachment 251515 [details] [review]:

looks okay.
Comment 24 Emmanuele Bassi (:ebassi) 2013-08-16 13:14:09 UTC
Review of attachment 251516 [details] [review]:

typo in the commit message: "it's own" instead of "its own". other than that, looks okay.
Comment 25 Emmanuele Bassi (:ebassi) 2013-08-16 13:14:31 UTC
Review of attachment 251517 [details] [review]:

looks okay.
Comment 26 Emmanuele Bassi (:ebassi) 2013-08-16 13:14:46 UTC
Review of attachment 251518 [details] [review]:

looks okay.
Comment 27 Giovanni Campagna 2013-08-16 14:28:07 UTC
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