On Thu, Aug 07, 2014 at 07:00:52PM -0400, Stephen Chandler Paul wrote:
> With tablets that don't support serial numbers, we can't guarantee that the
> tool
> objects are unique. Because of this, this can give clients the false
> impression
> that a tool without a serial number is being shared between tablets when it
> very
> well might not be. So we keep tools without serial numbers in a list that's
> local to the tablet they belong to, and keep tools with serials in a list
> that's
> local to the libinput context.
the libinput context isn't really local, so I just replaced this to "global
within the libinput context" which I think is a bit easier to grasp.
>
> Signed-off-by: Stephen Chandler Paul <thatsly...@gmail.com>
> ---
>
> Changes (forgot to make these with the last patch, sorry!):
> - Add comment to explain why we use a separate tool list in the code
> - Remove leftover include
>
> src/evdev-tablet.c | 41 ++++++++++++++++++++-----
> src/evdev-tablet.h | 3 ++
> test/tablet.c | 87
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 123 insertions(+), 8 deletions(-)
>
> diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c
> index 31dd8d7..15f0663 100644
> --- a/src/evdev-tablet.c
> +++ b/src/evdev-tablet.c
> @@ -259,17 +259,36 @@ tablet_process_misc(struct tablet_dispatch *tablet,
> }
>
> static struct libinput_tool *
> -tablet_get_tool(struct libinput *li,
> +tablet_get_tool(struct tablet_dispatch *tablet,
> enum libinput_tool_type type,
> uint32_t serial)
> {
> struct libinput_tool *tool = NULL, *t;
> + struct list *tool_list;
>
> - /* Check if we already have the tool in our list of tools */
> - list_for_each(t, &li->tool_list, link) {
> - if (type == t->type && serial == t->serial) {
> - tool = t;
> - break;
> + if (serial) {
> + tool_list = &tablet->device->base.seat->libinput->tool_list;
> +
> + /* Check if we already have the tool in our list of tools */
> + list_for_each(t, tool_list, link) {
> + if (type == t->type && serial == t->serial) {
> + tool = t;
> + break;
> + }
> + }
> + } else {
> + /* We can't guarantee that tools without serial numbers are
> + * unique, so we keep them local to the tablet that they come
> + * into proximity of instead of storing them in the global tool
> + * list */
> + tool_list = &tablet->tool_list;
> +
> + /* Same as above, but don't bother checking the serial number */
> + list_for_each(t, tool_list, link) {
> + if (type == t->type) {
> + tool = t;
> + break;
> + }
> }
> }
>
> @@ -283,7 +302,7 @@ tablet_get_tool(struct libinput *li,
> .refcount = 1,
> };
>
> - list_insert(&li->tool_list, &tool->link);
> + list_insert(tool_list, &tool->link);
> }
>
> return tool;
> @@ -382,7 +401,7 @@ tablet_flush(struct tablet_dispatch *tablet,
> uint32_t time)
> {
> struct libinput_tool *tool =
> - tablet_get_tool(device->base.seat->libinput,
> + tablet_get_tool(tablet,
> tablet->current_tool_type,
> tablet->current_tool_serial);
>
> @@ -473,6 +492,11 @@ tablet_destroy(struct evdev_dispatch *dispatch)
> {
> struct tablet_dispatch *tablet =
> (struct tablet_dispatch*)dispatch;
> + struct libinput_tool *tool, *tmp;
> +
> + list_for_each_safe(tool, tmp, &tablet->tool_list, link) {
> + libinput_tool_unref(tool);
> + }
>
> free(tablet);
> }
> @@ -490,6 +514,7 @@ tablet_init(struct tablet_dispatch *tablet,
> tablet->device = device;
> tablet->status = TABLET_NONE;
> tablet->current_tool_type = LIBINPUT_TOOL_NONE;
> + list_init(&tablet->tool_list);
>
> tablet_mark_all_axes_changed(tablet, device);
>
> diff --git a/src/evdev-tablet.h b/src/evdev-tablet.h
> index 1b53d20..adc3aa4 100644
> --- a/src/evdev-tablet.h
> +++ b/src/evdev-tablet.h
> @@ -49,6 +49,9 @@ struct tablet_dispatch {
> unsigned char changed_axes[NCHARS(LIBINPUT_TABLET_AXIS_CNT)];
> double axes[LIBINPUT_TABLET_AXIS_CNT];
>
> + /* Only used for tablets that don't report serial numbers */
> + struct list tool_list;
> +
> struct button_state button_state;
> struct button_state prev_button_state;
>
> diff --git a/test/tablet.c b/test/tablet.c
> index 0cd6357..2de0989 100644
> --- a/test/tablet.c
> +++ b/test/tablet.c
> @@ -681,6 +681,91 @@ START_TEST(pad_buttons_ignored)
> }
> END_TEST
>
> +START_TEST(tools_with_serials)
> +{
> + struct libinput *li = litest_create_context();
> + struct litest_device *dev[2];
> + struct libinput_tool *tool[2];
> + struct libinput_event *event;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + dev[i] = litest_add_device_with_overrides(li,
> + LITEST_WACOM_INTUOS,
> + NULL,
> + NULL,
> + NULL,
> + NULL);
> +
> + litest_event(dev[i], EV_KEY, BTN_TOOL_PEN, 1);
> + litest_event(dev[i], EV_MSC, MSC_SERIAL, 100);
> + litest_event(dev[i], EV_SYN, SYN_REPORT, 0);
> +
> + libinput_dispatch(li);
> + while ((event = libinput_get_event(li))) {
> + if (libinput_event_get_type(event) ==
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN) {
> + struct libinput_event_tablet *t =
> + libinput_event_get_tablet_event(event);
> +
> + tool[i] = libinput_event_tablet_get_tool(t);
> + }
> +
> + libinput_event_destroy(event);
> + }
> + }
> +
> + /* We should get the same object for both devices */
> + ck_assert_ptr_eq(tool[0], tool[1]);
There's a slight chance that if we never get the prox in event, we don't set
tool[i] and then end up comparing uninitialized values. unlikely chance for
a false positive, but I've added the bits to initialize tool to NULL and
check for not-null here. same for the second test below.
merged otherwise, thanks.
Cheers,
Peter
> +
> + litest_delete_device(dev[0]);
> + litest_delete_device(dev[1]);
> + libinput_unref(li);
> +}
> +END_TEST
> +
> +START_TEST(tools_without_serials)
> +{
> + struct libinput *li = litest_create_context();
> + struct litest_device *dev[2];
> + struct libinput_tool *tool[2];
> + struct libinput_event *event;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + dev[i] = litest_add_device_with_overrides(li,
> + LITEST_WACOM_ISDV4,
> + NULL,
> + NULL,
> + NULL,
> + NULL);
> +
> + litest_event(dev[i], EV_KEY, BTN_TOOL_PEN, 1);
> + litest_event(dev[i], EV_SYN, SYN_REPORT, 0);
> +
> + libinput_dispatch(li);
> + while ((event = libinput_get_event(li))) {
> + if (libinput_event_get_type(event) ==
> + LIBINPUT_EVENT_TABLET_PROXIMITY_IN) {
> + struct libinput_event_tablet *t =
> + libinput_event_get_tablet_event(event);
> +
> + tool[i] = libinput_event_tablet_get_tool(t);
> + }
> +
> + libinput_event_destroy(event);
> + }
> + }
> +
> + /* We should get different tool objects for each device */
> + ck_assert_ptr_ne(tool[0], tool[1]);
> +
> + litest_delete_device(dev[0]);
> + litest_delete_device(dev[1]);
> + libinput_unref(li);
> +}
> +END_TEST
> +
> int
> main(int argc, char **argv)
> {
> @@ -688,6 +773,8 @@ main(int argc, char **argv)
> litest_add("tablet:tool_serial", tool_serial, LITEST_TABLET |
> LITEST_TOOL_SERIAL, LITEST_ANY);
> litest_add("tablet:tool_serial", serial_changes_tool, LITEST_TABLET |
> LITEST_TOOL_SERIAL, LITEST_ANY);
> litest_add("tablet:tool_serial", invalid_serials, LITEST_TABLET |
> LITEST_TOOL_SERIAL, LITEST_ANY);
> + litest_add_no_device("tablet:tool_serial", tools_with_serials);
> + litest_add_no_device("tablet:tool_serial", tools_without_serials);
> litest_add("tablet:proximity", proximity_out_clear_buttons,
> LITEST_TABLET, LITEST_ANY);
> litest_add("tablet:proximity", proximity_in_out, LITEST_TABLET,
> LITEST_ANY);
> litest_add("tablet:proximity", bad_distance_events, LITEST_TABLET |
> LITEST_DISTANCE, LITEST_ANY);
> --
> 1.8.5.5
>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
