GNOME Bugzilla – Bug 706494
an assortment of wayland and evdev related changes
Last modified: 2013-09-12 07:52:53 UTC
This way, the full state of the device is exposed.
Created attachment 252549 [details] [review] ClutterInputDevice: add new API for querying the modifier state
Created attachment 254055 [details] [review] evdev: update the state of the core pointer and core keyboard for all events These two devices are logically tied togheter, and their state should always be the same.
Created attachment 254065 [details] [review] ClutterEvent: add API to query the full keyboard state when the event was generated When talking to other applications or serializing the modifier state (and in particular when implementing a wayland compositor), the effective modifier state alone is not sufficient, one needs to know the base, latched and locked modifiers. Previously one could do with backend specific functionality such as clutter_device_manager_evdev_get_xkb_state(), but the problem is that the internal data structures are updated as soon as the events are fetched from the upstream source, but the events are reported to the application some time later, and thus the two can get out of sync. This way, on the other hand, the information is cached in the event, and provided to the application with the value that was current when the event was generated. Yes, get_xkb_state() was a terrible idea. Sorry...
Created attachment 254254 [details] [review] evdev: sync the keyboard state when releasing and reclaiming devices When we release a device, we lose all the events after that point, so our state can become stale. Similarly, we need to sync the state with the effectively pressed keys when we reclaim. This ensures that modifier keys don't get stuck when switching VTs using a keybinding.
Created attachment 254255 [details] [review] evdev: switch to libevdev for fetching the events libevdev is a library that wraps the evdev subsystem, with the ability to synchronize the state after a SYN_DROPPED event from the kernel.
Created attachment 254256 [details] [review] evdev: implement setting leds When the leds are changed in the keyboard state, propagate the change to the actual devices.
Renaming since this bug is now about being more resilient against unexpected keyboard state changes.
Created attachment 254450 [details] [review] evdev: implement horizontal scrolling If the kernel reports REL_HWHELL, convert it to horizontal scroll events. Also reorganize a bit the recognition for the other event enums.
Created attachment 254451 [details] [review] evdev: use EV_SYN/SYN_REPORT for dispatching motion events We can't dispatch a motion event for EV_REL (because we don't have yet the other half of the event), but we can't also queue them at the end of processing (because we may lose some history or have button/keys intermixed). Instead, we use EV_SYN, which means "one logical event was completed", and let the winsys-independent code do the actual motion compression.
Created attachment 254452 [details] [review] evdev: add minimal support for touchpads The added support is very very basic (single touch, motion only, no acceleration, no pressure recognition), but anything more complex requires a state machine that will be hopefully provided by libinputcommon in the future. And at least, with this patch the pointer moves, which will be useful for people testing wayland in 3.10 without a physical mouse.
Review of attachment 252549 [details] [review]: ::: clutter/clutter-input-device.c @@ +473,3 @@ + * + * Retrieves the last known modifiers state of the device + */ missing "Since: 1.16" annotation and missing "Return value" annotation.
Review of attachment 254055 [details] [review]: looks good.
Review of attachment 254065 [details] [review]: looks generally okay. ::: clutter/clutter-event.c @@ +290,3 @@ + +/** + I do have a preference for separate accessors, but I think we can let this slide. @@ +296,3 @@ + * @latched_state: (out) (allow-none): the latched modifier keys (currently released but still valid for one key press/release) + * @locked_state: (out) (allow-none): the locked modifier keys (valid until the lock key is pressed and released again) + private->latched_state = latched_state; "the logical OR of all the state bits above". it would also be useful to note that clutter_event_get_state() returns the effective state. @@ +301,3 @@ + * latched, locked and effective. This can be used to transmit to other + * applications, for example when implementing a wayland compositor. + missing Since: 1.16 annotation. ::: clutter/clutter-event.h @@ +421,3 @@ ClutterModifierType state); ClutterModifierType clutter_event_get_state (const ClutterEvent *event); +void clutter_event_get_state_full (const ClutterEvent *event, missing CLUTTER_AVAILABLE_IN_1_16 annotation.
Review of attachment 254254 [details] [review]: looks okay.
Review of attachment 254256 [details] [review]: looks okay.
Review of attachment 254255 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +514,3 @@ + } + + /* Nothing to do here */ shouldn't this be EAGAIN? @@ +573,3 @@ + + err = libevdev_next_event (source->dev, LIBEVDEV_READ_NORMAL, &ev); + while (err != -EAGAIN) shouldn't this be EAGAIN, not -EAGAIN? @@ +596,3 @@ + queue_event: + /* Drop events if we don't have any stage to forward them to */ + if (!stage) please, use explicit NULL comparison.
Review of attachment 254450 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +334,3 @@ guint32 time_, + gint32 value, + gboolean vertical) don't use a boolean argument: pass a ClutterOrientation value instead. @@ +468,3 @@ } + else + /* We don't know about this code, ignore */; please, don't do this. either use an explicit empty block or, better yet, print out the code number using a debug message.
Review of attachment 254451 [details] [review]: looks okay
Review of attachment 254452 [details] [review]: ::: clutter/evdev/clutter-device-manager-evdev.c @@ +481,3 @@ + +static int +static void I'd make this a static inline. also, coding style: missing space between function name and arguments, and arguments should be vertically aligned. @@ +483,3 @@ +hysteresis(int in, int center) +{ +process_relative_motion (ClutterEventSource *source, this should be a #define. also, I have no idea whence this number comes from.
(In reply to comment #16) > Review of attachment 254255 [details] [review]: > > ::: clutter/evdev/clutter-device-manager-evdev.c > @@ +514,3 @@ > + } > + > + /* Nothing to do here */ > > shouldn't this be EAGAIN? > > @@ +573,3 @@ > + > + err = libevdev_next_event (source->dev, LIBEVDEV_READ_NORMAL, &ev); > + while (err != -EAGAIN) > > shouldn't this be EAGAIN, not -EAGAIN? No, libevdev follows the kernel convention of returning 0 on success and negative errnos on failure (with the added twist of returning 1 if a SYN_DROPPED was read successfully)
(In reply to comment #19) > Review of attachment 254452 [details] [review]: > > @@ +483,3 @@ > +hysteresis(int in, int center) > +{ > +process_relative_motion (ClutterEventSource *source, > > this should be a #define. also, I have no idea whence this number comes from. Empiric testing on my touchpad. weston (where the hysteresis function comes from) has it configurable and depending on the limits reported by the touchpad.
Created attachment 254455 [details] [review] ClutterInputDevice: add new API for querying the modifier state This way, the full state of the device is exposed.
Created attachment 254456 [details] [review] evdev: update the state of the core pointer and core keyboard for all events These two devices are logically tied togheter, and their state should always be the same. Also, we need to update them after the event is queued, as the current modifier state (as opposed to the modifier mask in the event) should include also the effect of the last key press/release.
Created attachment 254457 [details] [review] ClutterEvent: add API to query the full keyboard state when the event was generated When talking to other applications or serializing the modifier state (and in particular when implementing a wayland compositor), the effective modifier state alone is not sufficient, one needs to know the base, latched and locked modifiers. Previously one could do with backend specific functionality such as clutter_device_manager_evdev_get_xkb_state(), but the problem is that the internal data structures are updated as soon as the events are fetched from the upstream source, but the events are reported to the application some time later, and thus the two can get out of sync. This way, on the other hand, the information is cached in the event, and provided to the application with the value that was current when the event was generated.
Created attachment 254458 [details] [review] evdev: sync the keyboard state when releasing and reclaiming devices When we release a device, we lose all the events after that point, so our state can become stale. Similarly, we need to sync the state with the effectively pressed keys when we reclaim. This ensures that modifier keys don't get stuck when switching VTs using a keybinding.
Created attachment 254459 [details] [review] evdev: switch to libevdev for fetching the events libevdev is a library that wraps the evdev subsystem, with the ability to synchronize the state after a SYN_DROPPED event from the kernel.
Created attachment 254460 [details] [review] evdev: implement setting leds When the leds are changed in the keyboard state, propagate the change to the actual devices.
Created attachment 254461 [details] [review] evdev: implement horizontal scrolling If the kernel reports REL_HWHELL, convert it to horizontal scroll events. Also reorganize a bit the recognition for the other event enums.
Created attachment 254462 [details] [review] evdev: use EV_SYN/SYN_REPORT for dispatching motion events We can't dispatch a motion event for EV_REL (because we don't have yet the other half of the event), but we can't also queue them at the end of processing (because we may lose some history or have button/keys intermixed). Instead, we use EV_SYN, which means "one logical event was completed", and let the winsys-independent code do the actual motion compression.
Created attachment 254463 [details] [review] evdev: add minimal support for touchpads The added support is very very basic (single touch, motion only, no acceleration, no pressure recognition), but anything more complex requires a state machine that will be hopefully provided by libinputcommon in the future. And at least, with this patch the pointer moves, which will be useful for people testing wayland in 3.10 without a physical mouse.
(In reply to comment #20) > > shouldn't this be EAGAIN, not -EAGAIN? > > No, libevdev follows the kernel convention of returning 0 on success and > negative errnos on failure (with the added twist of returning 1 if a > SYN_DROPPED was read successfully) yey for consistent userspace interfaces.
Review of attachment 254455 [details] [review]: looks good.
Review of attachment 254455 [details] [review]: actually, I lied: clutter.symbols need to be updated as well.
Review of attachment 254457 [details] [review]: needs to update clutter.symbols as well, but looks good.
Review of attachment 254459 [details] [review]: you should also add an entry in the release notes listed README.in file with the new dependency.
Review of attachment 254461 [details] [review]: looks good.
(In reply to comment #21) > (In reply to comment #19) > > Review of attachment 254452 [details] [review] [details]: > > > > @@ +483,3 @@ > > +hysteresis(int in, int center) > > +{ > > +process_relative_motion (ClutterEventSource *source, > > > > this should be a #define. also, I have no idea whence this number comes from. > > Empiric testing on my touchpad. weston (where the hysteresis function comes > from) has it configurable and depending on the limits reported by the touchpad. you should add a pretty big comment there, then.
(In reply to comment #3) > Created an attachment (id=254065) [details] [review] > Yes, get_xkb_state() was a terrible idea. Sorry... since the only user was gnome-shell, and you don't seem to want it any more, we should remove it before 1.16.0.
Created attachment 254470 [details] [review] evdev: remove keyboard state accessor It was a bad idea to add it, because clutter events are batched, so by the time the application processes one, the keyboard state internally tracked by clutter could be already different. Instead, apps should use clutter_event_get_state_full()
Attachment 254455 [details] pushed as 0db9075 - ClutterInputDevice: add new API for querying the modifier state Attachment 254456 [details] pushed as dd940a7 - evdev: update the state of the core pointer and core keyboard for all events Attachment 254457 [details] pushed as 59f1e53 - ClutterEvent: add API to query the full keyboard state when the event was generated Attachment 254458 [details] pushed as 19536c8 - evdev: sync the keyboard state when releasing and reclaiming devices Attachment 254459 [details] pushed as cd1749a - evdev: switch to libevdev for fetching the events Attachment 254460 [details] pushed as d882366 - evdev: implement setting leds Attachment 254461 [details] pushed as 5e005b4 - evdev: implement horizontal scrolling Attachment 254462 [details] pushed as 15d036e - evdev: use EV_SYN/SYN_REPORT for dispatching motion events
Review of attachment 254470 [details] [review]: looks good.
(In reply to comment #41) > Review of attachment 254470 [details] [review]: > > looks good. Ok, I'll hold pushing this last one until the mutter-wayland side (bug 706963) is merged. touchpad support also could be a little better.
Comment on attachment 254470 [details] [review] evdev: remove keyboard state accessor Attachment 254470 [details] pushed as d4ddabe - evdev: remove keyboard state accessor
Created attachment 254693 [details] [review] evdev: add minimal support for touchpads The added support is very very basic (single touch, motion only, no acceleration, no pressure recognition), but anything more complex requires a state machine that will be hopefully provided by libinputcommon in the future. And at least, with this patch the pointer moves, which will be useful for people testing wayland in 3.10 without a physical mouse.
I think it is really important to get the 'pointer moves' part of this done for 3.10
Review of attachment 254693 [details] [review]: looks okay to me.
Attachment 254693 [details] pushed as 89cd311 - evdev: add minimal support for touchpads