Re: [PATCH v2 libinput] tablet: if a serial comes in late, discard it

[Jason Gerecke]

On 08/31/2016 11:43 PM, Peter Hutterer wrote:
> If a tool starts reporting with serial 0 and later updates to a real serial,
> discard that serial and keep reporting as serial 0. We cannot really change
> the tool after proximity in as we don't know when callers query for the serial
> (well, we could know but any well-written caller will ask for the serial on
> the proximity in event, so what's the point).
> 
> Thus if we do get a serial in and the matching tool, check if we have a tool
> with the serial 0 already. If so, re-use that. This means we lose correct tool
> tracking on such tablets but so far these seem to only be on devices where the
> use of multiple tools is unlikely.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=97526
> 
> Signed-off-by: Peter Hutterer 
> ---
> Changes to v1:
> - change tool_list assignment to be more obvious
> - fix proximity out in the new test
> 
>  src/evdev-tablet.c | 27 -
>  test/tablet.c  | 88 
> ++
>  2 files changed, 107 insertions(+), 8 deletions(-)
> 
> diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c
> index 36ff6a5..70ac0f7 100644
> --- a/src/evdev-tablet.c
> +++ b/src/evdev-tablet.c
> @@ -900,13 +900,12 @@ tablet_get_tool(struct tablet_dispatch *tablet,
>   uint32_t tool_id,
>   uint32_t serial)
>  {
> + struct libinput *libinput = tablet_libinput_context(tablet);
>   struct libinput_tablet_tool *tool = NULL, *t;
>   struct list *tool_list;
>  
>   if (serial) {
> - struct libinput *libinput = tablet_libinput_context(tablet);
>   tool_list = >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) {
> @@ -914,20 +913,32 @@ tablet_get_tool(struct tablet_dispatch *tablet,
>   break;
>   }
>   }
> - } else {
> + }
> +
> + /* If we get a tool with a delayed serial number, we already created
> +  * a 0-serial number tool for it earlier. Re-use that, even though
> +  * it means we can't distinguish this tool from others.
> +  * https://bugs.freedesktop.org/show_bug.cgi?id=97526
> +  */
> + if (!tool) {
> + tool_list = >tool_list;
>   /* 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 = >tool_list;
> -
> - /* Same as above, but don't bother checking the serial number */
> - list_for_each(t, tool_list, link) {
> +  * list
> +  * Same as above, but don't bother checking the serial number
> +  */
> + list_for_each(t, >tool_list, link) {

Nit: Just use 'tool_list' instead of '>tool_list' here?

Aside from that, for this set:

Reviewed-by: Jason Gerecke 

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours

>   if (type == t->type) {
>   tool = t;
>   break;
>   }
>   }
> +
> + /* Didn't find the tool but we have a serial. Switch
> +  * tool_list back so we create in the correct list */
> + if (!tool && serial)
> + tool_list = >tool_list;
>   }
>  
>   /* If we didn't already have the new_tool in our list of tools,
> diff --git a/test/tablet.c b/test/tablet.c
> index 7bf6ac6..abb826a 100644
> --- a/test/tablet.c
> +++ b/test/tablet.c
> @@ -2153,6 +2153,93 @@ START_TEST(tools_without_serials)
>  }
>  END_TEST
>  
> +START_TEST(tool_delayed_serial)
> +{
> + struct litest_device *dev = litest_current_device();
> + struct libinput *li = dev->libinput;
> + struct libinput_event *event;
> + struct libinput_event_tablet_tool *tev;
> + struct libinput_tablet_tool *tool;
> + unsigned int serial;
> +
> + litest_drain_events(li);
> +
> + litest_event(dev, EV_ABS, ABS_X, 4500);
> + litest_event(dev, EV_ABS, ABS_Y, 2000);
> + litest_event(dev, EV_MSC, MSC_SERIAL, 0);
> + litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> + libinput_dispatch(li);
> +
> + event = libinput_get_event(li);
> + tev = litest_is_tablet_event(event,
> +  LIBINPUT_EVENT_TABLET_TOOL_PROXIMITY);
> + tool = libinput_event_tablet_tool_get_tool(tev);
> + serial = libinput_tablet_tool_get_serial(tool);
> + 

[PATCH v2 libinput] tablet: if a serial comes in late, discard it

[Peter Hutterer]

If a tool starts reporting with serial 0 and later updates to a real serial,
discard that serial and keep reporting as serial 0. We cannot really change
the tool after proximity in as we don't know when callers query for the serial
(well, we could know but any well-written caller will ask for the serial on
the proximity in event, so what's the point).

Thus if we do get a serial in and the matching tool, check if we have a tool
with the serial 0 already. If so, re-use that. This means we lose correct tool
tracking on such tablets but so far these seem to only be on devices where the
use of multiple tools is unlikely.

https://bugs.freedesktop.org/show_bug.cgi?id=97526

Signed-off-by: Peter Hutterer 
---
Changes to v1:
- change tool_list assignment to be more obvious
- fix proximity out in the new test

 src/evdev-tablet.c | 27 -
 test/tablet.c  | 88 ++
 2 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c
index 36ff6a5..70ac0f7 100644
--- a/src/evdev-tablet.c
+++ b/src/evdev-tablet.c
@@ -900,13 +900,12 @@ tablet_get_tool(struct tablet_dispatch *tablet,
uint32_t tool_id,
uint32_t serial)
 {
+   struct libinput *libinput = tablet_libinput_context(tablet);
struct libinput_tablet_tool *tool = NULL, *t;
struct list *tool_list;
 
if (serial) {
-   struct libinput *libinput = tablet_libinput_context(tablet);
tool_list = >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) {
@@ -914,20 +913,32 @@ tablet_get_tool(struct tablet_dispatch *tablet,
break;
}
}
-   } else {
+   }
+
+   /* If we get a tool with a delayed serial number, we already created
+* a 0-serial number tool for it earlier. Re-use that, even though
+* it means we can't distinguish this tool from others.
+* https://bugs.freedesktop.org/show_bug.cgi?id=97526
+*/
+   if (!tool) {
+   tool_list = >tool_list;
/* 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 = >tool_list;
-
-   /* Same as above, but don't bother checking the serial number */
-   list_for_each(t, tool_list, link) {
+* 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;
}
}
+
+   /* Didn't find the tool but we have a serial. Switch
+* tool_list back so we create in the correct list */
+   if (!tool && serial)
+   tool_list = >tool_list;
}
 
/* If we didn't already have the new_tool in our list of tools,
diff --git a/test/tablet.c b/test/tablet.c
index 7bf6ac6..abb826a 100644
--- a/test/tablet.c
+++ b/test/tablet.c
@@ -2153,6 +2153,93 @@ START_TEST(tools_without_serials)
 }
 END_TEST
 
+START_TEST(tool_delayed_serial)
+{
+   struct litest_device *dev = litest_current_device();
+   struct libinput *li = dev->libinput;
+   struct libinput_event *event;
+   struct libinput_event_tablet_tool *tev;
+   struct libinput_tablet_tool *tool;
+   unsigned int serial;
+
+   litest_drain_events(li);
+
+   litest_event(dev, EV_ABS, ABS_X, 4500);
+   litest_event(dev, EV_ABS, ABS_Y, 2000);
+   litest_event(dev, EV_MSC, MSC_SERIAL, 0);
+   litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1);
+   litest_event(dev, EV_SYN, SYN_REPORT, 0);
+   libinput_dispatch(li);
+
+   event = libinput_get_event(li);
+   tev = litest_is_tablet_event(event,
+LIBINPUT_EVENT_TABLET_TOOL_PROXIMITY);
+   tool = libinput_event_tablet_tool_get_tool(tev);
+   serial = libinput_tablet_tool_get_serial(tool);
+   ck_assert_int_eq(serial, 0);
+   libinput_event_destroy(event);
+
+   for (int x = 4500; x < 8000; x += 1000) {
+   litest_event(dev, EV_ABS, ABS_X, x);
+   litest_event(dev, EV_ABS, ABS_Y, 2000);
+   litest_event(dev, EV_MSC, MSC_SERIAL, 0);
+   litest_event(dev, EV_SYN, SYN_REPORT, 0);
+   libinput_dispatch(li);
+   }
+   litest_drain_events(li);
+
+   /* Now send the serial */
+   litest_event(dev, EV_ABS,