[PATCH libinput 2/3] tablet: Replace tool-update with proximity-in
Peter Hutterer
peter.hutterer at who-t.net
Wed Jun 25 20:57:57 PDT 2014
On Wed, Jun 25, 2014 at 02:30:16AM -0400, Stephen Chandler Paul wrote:
> A proximity-in event is something we want, especially since the current drafted
> wayland spec has a proximity-in event. Adding this also makes our events more
> consistent. And since we can just report the current tool in use with
> proximity-in events, we can get rid of the tool-update event.
>
> Signed-off-by: Stephen Chandler Paul <thatslyude at gmail.com>
Yes to the proximity in event, it makes the API more symmetric and it
ensures that we can send the original axis state.
> ---
> src/evdev-tablet.c | 62 +++++++++++++++++++++++++++-----------------------
> src/evdev-tablet.h | 5 +++-
> src/libinput-private.h | 6 ++---
> src/libinput.c | 28 +++++++++++------------
> src/libinput.h | 27 +++++++++++-----------
> test/tablet.c | 43 ++++++++++++++--------------------
> tools/event-debug.c | 10 ++++----
> tools/event-gui.c | 2 +-
> 8 files changed, 92 insertions(+), 91 deletions(-)
>
> diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c
> index 584b49e..44b631a 100644
> --- a/src/evdev-tablet.c
> +++ b/src/evdev-tablet.c
> @@ -99,6 +99,7 @@ tablet_update_tool(struct tablet_dispatch *tablet,
> tablet_set_status(tablet, TABLET_TOOL_UPDATED);
> }
> tablet_mark_all_axes_changed(tablet, device);
> + tablet_set_status(tablet, TABLET_TOOL_ENTERING_PROXIMITY);
> tablet_unset_status(tablet, TABLET_TOOL_OUT_OF_PROXIMITY);
> }
> else
> @@ -250,8 +251,6 @@ tablet_process_misc(struct tablet_dispatch *tablet,
> e->value != -1) {
> tablet->current_tool_serial = e->value;
> tablet_set_status(tablet, TABLET_TOOL_UPDATED);
> - tablet_unset_status(tablet,
> - TABLET_TOOL_OUT_OF_PROXIMITY);
> }
> break;
> default:
> @@ -264,36 +263,43 @@ tablet_process_misc(struct tablet_dispatch *tablet,
> }
>
> static void
> -tablet_notify_tool(struct tablet_dispatch *tablet,
> - struct evdev_device *device,
> - uint32_t time)
> +tablet_check_notify_proximity_in(struct tablet_dispatch *tablet,
> + struct evdev_device *device,
> + uint32_t time)
> {
> struct libinput_device *base = &device->base;
> - struct libinput_tool *tool;
> - struct libinput_tool *new_tool = NULL;
> -
> - /* Check if we already have the tool in our list of tools */
> - list_for_each(tool, &base->seat->libinput->tool_list, link) {
> - if (tablet->current_tool_type == tool->type &&
> - tablet->current_tool_serial == tool->serial) {
> - new_tool = tool;
> - break;
> +
> + if (tablet_has_status(tablet, TABLET_TOOL_UPDATED)) {
> + struct libinput_tool *tool,
> + *new_tool = NULL;
> +
> + /* Check if we already have the tool in our list of tools */
> + list_for_each(tool, &base->seat->libinput->tool_list, link) {
> + if (tablet->current_tool_type == tool->type &&
> + tablet->current_tool_serial == tool->serial) {
> + new_tool = tool;
> + break;
> + }
> }
> - }
>
> - /* If we didn't already have the tool in our list of tools, add it */
> - if (new_tool == NULL) {
> - new_tool = zalloc(sizeof *new_tool);
> - *new_tool = (struct libinput_tool) {
> - .type = tablet->current_tool_type,
> - .serial = tablet->current_tool_serial,
> - .refcount = 1,
> - };
> + /* If we didn't already have the new_tool in our list of tools,
> + * add it */
> + if (new_tool == NULL) {
> + new_tool = zalloc(sizeof *new_tool);
> + *new_tool = (struct libinput_tool) {
> + .type = tablet->current_tool_type,
> + .serial = tablet->current_tool_serial,
> + .refcount = 1,
> + };
> +
> + list_insert(&base->seat->libinput->tool_list,
> + &new_tool->link);
> + }
>
> - list_insert(&base->seat->libinput->tool_list, &new_tool->link);
> + tablet->current_tool = new_tool;
> }
I admit I really dislike functions where most of the body is behind an if
condition. In this case, it's better to have a helper function
tablet_get_tool(libinput, type, serial) that returns or creates the tool and
call that. You don't use tablet->current_tool anywhere else either, so you
can make that a local variable.
>
> - tablet_notify_tool_update(base, time, new_tool);
> + tablet_notify_proximity_in(base, time, tablet->current_tool);
> }
>
> static void
> @@ -383,9 +389,9 @@ tablet_flush(struct tablet_dispatch *tablet,
> /* Release all stylus buttons */
> tablet->button_state.stylus_buttons = 0;
> tablet_set_status(tablet, TABLET_BUTTONS_RELEASED);
> - } else if (tablet_has_status(tablet, TABLET_TOOL_UPDATED)) {
> - tablet_notify_tool(tablet, device, time);
> - tablet_unset_status(tablet, TABLET_TOOL_UPDATED);
> + } else if (tablet_has_status(tablet, TABLET_TOOL_ENTERING_PROXIMITY)) {
> + tablet_check_notify_proximity_in(tablet, device, time);
> + tablet_unset_status(tablet, TABLET_TOOL_ENTERING_PROXIMITY);
> }
>
> if (tablet_has_status(tablet, TABLET_AXES_UPDATED)) {
> diff --git a/src/evdev-tablet.h b/src/evdev-tablet.h
> index 89cf224..aa6b455 100644
> --- a/src/evdev-tablet.h
> +++ b/src/evdev-tablet.h
> @@ -35,7 +35,8 @@ enum tablet_status {
> TABLET_BUTTONS_RELEASED = 1 << 3,
> TABLET_STYLUS_IN_CONTACT = 1 << 4,
> TABLET_TOOL_LEAVING_PROXIMITY = 1 << 5,
> - TABLET_TOOL_OUT_OF_PROXIMITY = 1 << 6
> + TABLET_TOOL_OUT_OF_PROXIMITY = 1 << 6,
> + TABLET_TOOL_ENTERING_PROXIMITY = 1 << 7
> };
>
> struct button_state {
> @@ -54,6 +55,8 @@ struct tablet_dispatch {
>
> enum libinput_tool_type current_tool_type;
> uint32_t current_tool_serial;
> +
> + struct libinput_tool *current_tool;
> };
>
> static inline enum libinput_tablet_axis
> diff --git a/src/libinput-private.h b/src/libinput-private.h
> index db78cca..e59fe12 100644
> --- a/src/libinput-private.h
> +++ b/src/libinput-private.h
> @@ -218,9 +218,9 @@ tablet_notify_axis(struct libinput_device *device,
> double *axes);
>
> void
> -tablet_notify_tool_update(struct libinput_device *device,
> - uint32_t time,
> - struct libinput_tool *tool);
> +tablet_notify_proximity_in(struct libinput_device *device,
> + uint32_t time,
> + struct libinput_tool *tool);
>
> void
> tablet_notify_proximity_out(struct libinput_device *device,
> diff --git a/src/libinput.c b/src/libinput.c
> index e72ad8b..3726eb5 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -195,7 +195,7 @@ libinput_event_get_pointer_event(struct libinput_event *event)
> case LIBINPUT_EVENT_TOUCH_CANCEL:
> case LIBINPUT_EVENT_TOUCH_FRAME:
> case LIBINPUT_EVENT_TABLET_AXIS:
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> case LIBINPUT_EVENT_TABLET_BUTTON:
> break;
> @@ -225,7 +225,7 @@ libinput_event_get_keyboard_event(struct libinput_event *event)
> case LIBINPUT_EVENT_TOUCH_CANCEL:
> case LIBINPUT_EVENT_TOUCH_FRAME:
> case LIBINPUT_EVENT_TABLET_AXIS:
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> case LIBINPUT_EVENT_TABLET_BUTTON:
> break;
> @@ -255,7 +255,7 @@ libinput_event_get_touch_event(struct libinput_event *event)
> case LIBINPUT_EVENT_TOUCH_FRAME:
> return (struct libinput_event_touch *) event;
> case LIBINPUT_EVENT_TABLET_AXIS:
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> case LIBINPUT_EVENT_TABLET_BUTTON:
> break;
> @@ -284,7 +284,7 @@ libinput_event_get_tablet_event(struct libinput_event *event)
> case LIBINPUT_EVENT_TOUCH_FRAME:
> break;
> case LIBINPUT_EVENT_TABLET_AXIS:
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> case LIBINPUT_EVENT_TABLET_BUTTON:
> return (struct libinput_event_tablet *) event;
> @@ -313,7 +313,7 @@ libinput_event_get_device_notify_event(struct libinput_event *event)
> case LIBINPUT_EVENT_TOUCH_CANCEL:
> case LIBINPUT_EVENT_TOUCH_FRAME:
> case LIBINPUT_EVENT_TABLET_AXIS:
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> case LIBINPUT_EVENT_TABLET_BUTTON:
> break;
> @@ -1250,24 +1250,24 @@ tablet_notify_axis(struct libinput_device *device,
> }
>
> void
> -tablet_notify_tool_update(struct libinput_device *device,
> - uint32_t time,
> - struct libinput_tool *tool)
> +tablet_notify_proximity_in(struct libinput_device *device,
> + uint32_t time,
> + struct libinput_tool *tool)
> {
> - struct libinput_event_tablet *tool_update_event;
> + struct libinput_event_tablet *proximity_in_event;
>
> - tool_update_event = zalloc(sizeof *tool_update_event);
> - if (!tool_update_event)
> + proximity_in_event = zalloc(sizeof *proximity_in_event);
> + if (!proximity_in_event)
> return;
>
> - *tool_update_event = (struct libinput_event_tablet) {
> + *proximity_in_event = (struct libinput_event_tablet) {
> .time = time,
> .tool = tool,
> };
>
> post_device_event(device,
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE,
> - &tool_update_event->base);
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN,
> + &proximity_in_event->base);
> }
tbh, I think a better option here would be to add the axes to the event as
well. That way the caller gets the initial axis state together with the
first event and the next real axis event only includes the deltas in the
bit mask.
Which, for symmetry, would suggest sending the axes for the prox out event
as well as a "last known state", though that event must have the bitmasks
all on 0.
> void
> diff --git a/src/libinput.h b/src/libinput.h
> index 38b6610..49162dd 100644
> --- a/src/libinput.h
> +++ b/src/libinput.h
> @@ -261,10 +261,10 @@ enum libinput_event_type {
>
> LIBINPUT_EVENT_TABLET_AXIS = 600,
> /**
> - * Signals that a device with the @ref LIBINPUT_DEVICE_CAP_TABLET
> - * capability has changed its tool.
> + * Signals that a tool has come into proximity of a device with the @ref
> + * LIBINPUT_DEVICE_CAP_TABLET capability.
> */
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE,
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN,
> /**
> * Signals that a device with the @ref LIBINPUT_DEVICE_CAP_TABLET
> * capability has detected that there is no longer a tool in use. When
> @@ -305,8 +305,8 @@ struct libinput_event_touch;
> *
> * Tablet event representing an axis update, button press, or tool update. Valid
> * event types for this event are @ref LIBINPUT_EVENT_TABLET_AXIS, @ref
> - * LIBINPUT_EVENT_TABLET_TOOL_UPDATE, @ref LIBINPUT_EVENT_TABLET_TOOL_UPDATE and
> - * @ref LIBINPUT_EVENT_TABLET_BUTTON.
> + * LIBINPUT_EVENT_TABLET_PROXIMITY_IN, @ref LIBINPUT_EVENT_TABLET_PROXIMITY_IN
> + * and @ref LIBINPUT_EVENT_TABLET_BUTTON.
> */
> struct libinput_event_tablet;
>
> @@ -918,17 +918,18 @@ libinput_event_tablet_get_y_transformed(struct libinput_event_tablet *event,
> /**
> * @ingroup event_tablet
> *
> - * Return the new tool in use for this event.
> + * Returns the tool that came into proximity for this event.
> * For tablet events that are not of type @ref
> - * LIBINPUT_EVENT_TABLET_TOOL_UPDATE, this function returns NULL. By default,
> - * the lifetime of each tool is bound to the lifetime of the event, so the tool
> - * will be destroyed when the event is destroyed. However, the lifetime of the
> - * tool may be extended by using libinput_tool_ref() to increment the reference
> - * count of the tool. Whenever libinput detects that the tool is in proximity of
> - * any tablet that's connected, it will return the same libinput_tool object.
> + * LIBINPUT_EVENT_TABLET_PROXIMITY_IN, this function returns NULL. By default,
> + * the lifetime of each tool will stay valid for as long as it is being used,
> + * and is destroyed when another tool comes into proximity. However, the
> + * lifetime of the tool may be extended by using libinput_tool_ref() to
> + * increment the reference count of the tool. This guarantees that whenever the
> + * tool comes back into proximity of the tablet, that the same structure will be
> + * used to represent the tool.
> *
> * @note It is an application bug to call this function for events other than
> - * @ref LIBINPUT_EVENT_TABLET_TOOL_UPDATE.
> + * @ref LIBINPUT_EVENT_TABLET_PROXIMITY_IN.
> *
> * @note On tablets where the serial number of tools is not reported, each tool
> * cannot be guaranteed to be unique.
I mentioned this on IRC but for the archive: the get_tool() function should
work for any tablet event, especially now that we dropped the pad from the
tablet interface.
> diff --git a/test/tablet.c b/test/tablet.c
> index 6fb4465..0f0728e 100644
> --- a/test/tablet.c
> +++ b/test/tablet.c
> @@ -55,7 +55,7 @@ START_TEST(proximity_in_out)
>
> while ((event = libinput_get_event(li))) {
> if (libinput_event_get_type(event) ==
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE) {
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN) {
> struct libinput_tool * tool;
this test now also needs to check that all axes are updated.
> have_tool_update++;
> @@ -444,7 +444,7 @@ START_TEST(tool_serial)
> libinput_dispatch(li);
> while ((event = libinput_get_event(li))) {
> if (libinput_event_get_type(event) ==
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE) {
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN) {
> tablet_event = libinput_event_get_tablet_event(event);
> tool = libinput_event_tablet_get_tool(tablet_event);
>
> @@ -460,36 +460,35 @@ START_TEST(serial_changes_tool)
> {
> struct litest_device *dev = litest_current_device();
> struct libinput *li = dev->libinput;
> - struct libinput_event_tablet *tablet_event;
> struct libinput_event *event;
> + struct libinput_event_tablet *tablet_event;
please try to avoid spurious changes like this.
> struct libinput_tool *tool;
> - bool tool_updated = false;
>
> litest_drain_events(li);
>
> litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1);
> litest_event(dev, EV_MSC, MSC_SERIAL, 1000);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> -
> + litest_event(dev, EV_KEY, BTN_TOOL_PEN, 0);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> litest_drain_events(li);
>
> + litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1);
> litest_event(dev, EV_MSC, MSC_SERIAL, 2000);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
what happens if the MSC_SERIAL changes on it's own? do we handle this?
shouldn't happen on a device, but that doesn't mean we won't see it.
(hint: if you need to change the event sequence to adjust a test, it's
usually worth leaving the original sequence to make sure whatever we did
before is still handled)
>
> libinput_dispatch(li);
> while ((event = libinput_get_event(li))) {
> if (libinput_event_get_type(event) ==
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE) {
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN) {
> tablet_event = libinput_event_get_tablet_event(event);
> tool = libinput_event_tablet_get_tool(tablet_event);
>
> ck_assert_uint_eq(libinput_tool_get_serial(tool), 2000);
> - tool_updated = true;
> }
>
> libinput_event_destroy(event);
> }
> - ck_assert(tool_updated);
why did you take that check away? you're not guaranteeing that the event is
sent, so if we never see a proximity event we still succeed
Cheers,
Peter
> }
> END_TEST
>
> @@ -498,42 +497,34 @@ START_TEST(invalid_serials)
> struct litest_device *dev = litest_current_device();
> struct libinput *li = dev->libinput;
> struct libinput_event *event;
> - bool tool_updated = false;
> + struct libinput_event_tablet *tablet_event;
> + struct libinput_tool *tool;
>
> litest_drain_events(li);
>
> litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1);
> litest_event(dev, EV_MSC, MSC_SERIAL, 1000);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> + litest_event(dev, EV_KEY, BTN_TOOL_PEN, 0);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> litest_drain_events(li);
>
> + litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1);
> litest_event(dev, EV_MSC, MSC_SERIAL, -1);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
>
> libinput_dispatch(li);
> while ((event = libinput_get_event(li))) {
> if (libinput_event_get_type(event) ==
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE)
> - tool_updated = true;
> -
> - libinput_event_destroy(event);
> - }
> - ck_assert(!tool_updated);
> -
> - /* Make sure libinput doesn't report a tool update when the serial
> - * number goes back from -1 to what it was previously */
> - litest_event(dev, EV_MSC, MSC_SERIAL, 1000);
> - litest_event(dev, EV_SYN, SYN_REPORT, 0);
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN) {
> + tablet_event = libinput_event_get_tablet_event(event);
> + tool = libinput_event_tablet_get_tool(tablet_event);
>
> - libinput_dispatch(li);
> - while ((event = libinput_get_event(li))) {
> - if (libinput_event_get_type(event) ==
> - LIBINPUT_EVENT_TABLET_TOOL_UPDATE)
> - tool_updated = true;
> + ck_assert_uint_eq(libinput_tool_get_serial(tool), 1000);
> + }
>
> libinput_event_destroy(event);
> }
> - ck_assert(!tool_updated);
> }
> END_TEST
>
> diff --git a/tools/event-debug.c b/tools/event-debug.c
> index 299587b..5c6a062 100644
> --- a/tools/event-debug.c
> +++ b/tools/event-debug.c
> @@ -243,8 +243,8 @@ print_event_header(struct libinput_event *ev)
> case LIBINPUT_EVENT_TABLET_AXIS:
> type = "TABLET_AXIS";
> break;
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> - type = "TABLET_TOOL_UPDATE";
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> + type = "TABLET_PROXIMITY_IN";
> break;
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> type = "TABLET_PROXIMITY_OUT";
> @@ -424,7 +424,7 @@ print_touch_event_without_coords(struct libinput_event *ev)
> }
>
> static void
> -print_tool_update_event(struct libinput_event *ev)
> +print_proximity_in_event(struct libinput_event *ev)
> {
> struct libinput_event_tablet *t = libinput_event_get_tablet_event(ev);
> struct libinput_tool *tool = libinput_event_tablet_get_tool(t);
> @@ -543,8 +543,8 @@ handle_and_print_events(struct libinput *li)
> case LIBINPUT_EVENT_TABLET_AXIS:
> print_tablet_axis_event(ev);
> break;
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> - print_tool_update_event(ev);
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> + print_proximity_in_event(ev);
> break;
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> print_proximity_out_event(ev);
> diff --git a/tools/event-gui.c b/tools/event-gui.c
> index b184a42..7bc3828 100644
> --- a/tools/event-gui.c
> +++ b/tools/event-gui.c
> @@ -376,7 +376,7 @@ handle_event_libinput(GIOChannel *source, GIOCondition condition, gpointer data)
> }
> break;
> case LIBINPUT_EVENT_TABLET_AXIS:
> - case LIBINPUT_EVENT_TABLET_TOOL_UPDATE:
> + case LIBINPUT_EVENT_TABLET_PROXIMITY_IN:
> case LIBINPUT_EVENT_TABLET_PROXIMITY_OUT:
> case LIBINPUT_EVENT_TABLET_BUTTON:
> break;
> --
> 1.8.5.5
>
More information about the wayland-devel
mailing list