On Tue, Mar 25, 2014 at 09:45:54PM +0100, Jonas Ådahl wrote: > Add one test that checks uniqueness of seat slots when having multiple > devices with active touch points. > > Add one test that checks that libinput drops touch points when it could > not represent them with a seat wide slot. > > This commit also adds support for from a test case add test devices to > an existing libinput context. Only litest-genric-highres-touch supports > this so far and only generic artifical test devices should, in order to > keep emulated devices closer to their originals.
I'm generally happy with the test itself, but the extension to the litest framework I'm not convinced. There are a bunch of tests already that need specific devices and they just create a uinput device. I've got an unfinished series here that adds a wrapper to make this easier and with less boilerplate. My main worries with this here is that the deviceid is part of the litest_device now but meaningless for all other devices. And for this specific test you're essentially hiding one of the main features in the generic_highres device - that it has > MAX_SLOT slots. Plus the extra bits that elevate litest from a "initializes behind the scenes for multiple devices" to something that almost looks like an API when it really doesn't give you that much benefit over just creating a uinput device. Finally, we already have generic devices, you could simply add a generic_toomanyslots device. I'll try to clean up the uinput wrapper I mentioned above tomorrow, maybe that will be enough for this patch. a few other comments inline > > Signed-off-by: Jonas Ådahl <jad...@gmail.com> > --- > test/litest-generic-highres-touch.c | 28 +++++- > test/litest.c | 33 ++++++- > test/litest.h | 13 ++- > test/touch.c | 184 > ++++++++++++++++++++++++++++++++++++ > 4 files changed, 251 insertions(+), 7 deletions(-) > > diff --git a/test/litest-generic-highres-touch.c > b/test/litest-generic-highres-touch.c > index 68615c3..5f2f023 100644 > --- a/test/litest-generic-highres-touch.c > +++ b/test/litest-generic-highres-touch.c > @@ -23,11 +23,22 @@ > > #include "config.h" > > +#include <stdio.h> > + > #include "litest.h" > #include "litest-int.h" > #include "libinput-util.h" > > -void litest_generic_highres_touch_setup(void) > +static int device_ids = 0; > + > +static void > +litest_generic_highres_touch_destroy(struct litest_device *dev) > +{ > + device_ids &= ~dev->device_id; > +} > + > +void > +litest_generic_highres_touch_setup(void) > { > struct litest_device *d = > litest_create_device(LITEST_GENERIC_HIGHRES_TOUCH); > @@ -96,22 +107,33 @@ litest_create_generic_highres_touch(struct litest_device > *d) > { > struct libevdev *dev; > int rc; > + int device_id; > + char name[255]; > struct input_absinfo *a; > struct input_absinfo abs[] = { > { ABS_X, 0, 32767, 75 }, > { ABS_Y, 0, 32767, 129 }, > - { ABS_MT_SLOT, 0, 1, 0 }, > + { ABS_MT_SLOT, 0, 40, 0 }, > { ABS_MT_POSITION_X, 0, 32767, 0, 0, 10 }, > { ABS_MT_POSITION_Y, 0, 32767, 0, 0, 9 }, > { ABS_MT_TRACKING_ID, 0, 65535, 0 }, > }; > > d->interface = &interface; > + d->destroy = litest_generic_highres_touch_destroy; > > dev = libevdev_new(); > ck_assert(dev != NULL); > > - libevdev_set_name(dev, "Generic emulated highres touch device"); > + device_id = ffs(~device_ids) - 1; > + ck_assert_int_ge(device_id, 0); > + device_ids |= 1 << device_id; > + d->device_id = device_id; > + snprintf(name, sizeof name, > + "Generic emulated highres touch device (%d)", > + device_id); > + libevdev_set_name(dev, name); > + > libevdev_set_id_bustype(dev, 0x3); > libevdev_set_id_vendor(dev, 0xabcd); /* Some random vendor. */ > libevdev_set_id_product(dev, 0x1234); /* Some random product id. */ > diff --git a/test/litest.c b/test/litest.c > index 9241623..0e7da70 100644 > --- a/test/litest.c > +++ b/test/litest.c > @@ -328,7 +328,8 @@ const struct libinput_interface interface = { > }; > > struct litest_device * > -litest_create_device(enum litest_device_type which) > +litest_create_device_for(struct libinput *libinput, > + enum litest_device_type which) > { > struct litest_device *d = zalloc(sizeof(*d)); > int fd; > @@ -360,7 +361,7 @@ litest_create_device(enum litest_device_type which) > rc = libevdev_new_from_fd(fd, &d->evdev); > ck_assert_int_eq(rc, 0); > > - d->libinput = libinput_path_create_context(&interface, NULL); > + d->libinput = libinput; > ck_assert(d->libinput != NULL); > > d->libinput_device = libinput_path_add_device(d->libinput, path); > @@ -374,6 +375,28 @@ litest_create_device(enum litest_device_type which) > return d; > } > > +struct libinput * > +litest_create_context(void) > +{ > + struct libinput *libinput = > + libinput_path_create_context(&interface, NULL); > + ck_assert_notnull(libinput); > + return libinput; > +} > + > +struct litest_device * > +litest_create_device(enum litest_device_type which) > +{ > + struct libinput *libinput; > + struct litest_device *d; > + > + libinput = litest_create_context(); > + d = litest_create_device_for(libinput, which); > + d->owns_context = true; > + > + return d; > +} > + > int > litest_handle_events(struct litest_device *d) > { > @@ -394,8 +417,12 @@ litest_delete_device(struct litest_device *d) > if (!d) > return; > > + if (d->destroy) > + d->destroy(d); > + > libinput_device_unref(d->libinput_device); > - libinput_destroy(d->libinput); > + if (d->owns_context) > + libinput_destroy(d->libinput); > libevdev_free(d->evdev); > libevdev_uinput_destroy(d->uinput); > memset(d,0, sizeof(*d)); > diff --git a/test/litest.h b/test/litest.h > index 9f0f614..0aa1c99 100644 > --- a/test/litest.h > +++ b/test/litest.h > @@ -62,8 +62,11 @@ struct litest_device { > struct libevdev *evdev; > struct libevdev_uinput *uinput; > struct libinput *libinput; > + void (*destroy)(struct litest_device *d); > + bool owns_context; > struct libinput_device *libinput_device; > struct litest_device_interface *interface; > + int device_id; I'm not quite sure why you split these additions up. seems like they could just be appended to existing fields. > }; > > void litest_add(const char *name, void *func, > @@ -72,7 +75,11 @@ void litest_add(const char *name, void *func, > void litest_add_no_device(const char *name, void *func); > > int litest_run(int argc, char **argv); > -struct litest_device * litest_create_device(enum litest_device_type which); > + > +struct libinput *litest_create_context(void); > +struct litest_device *litest_create_device(enum litest_device_type which); > +struct litest_device *litest_create_device_for(struct libinput *libinput, > + enum litest_device_type which); > struct litest_device *litest_current_device(void); > void litest_delete_device(struct litest_device *d); > int litest_handle_events(struct litest_device *d); > @@ -100,4 +107,8 @@ void litest_button_click(struct litest_device *d, > bool is_press); > void litest_drain_events(struct libinput *li); > > +#ifndef ck_assert_notnull > +#define ck_assert_notnull(ptr) ck_assert_ptr_ne(ptr, NULL) > +#endif wouldn't a simple ck_assert(ptr != NULL) be enough? > + > #endif /* LITEST_H */ > diff --git a/test/touch.c b/test/touch.c > index 6833e10..9354c25 100644 > --- a/test/touch.c > +++ b/test/touch.c > @@ -22,6 +22,7 @@ > > #include <config.h> > > +#include <stdio.h> > #include <check.h> > #include <errno.h> > #include <fcntl.h> > @@ -92,6 +93,187 @@ START_TEST(touch_abs_transform) > } > END_TEST > > +START_TEST(touch_seat_slots) > +{ > + > + struct libinput *libinput; > + struct litest_device *dev1; > + struct litest_device *dev2; > + struct libinput_event *ev; > + struct libinput_event_touch *tev; > + int32_t seat_slot; > + int32_t last_seat_slot = -1; > + bool has_last_seat_slot = false; > + > + libinput = litest_create_context(); > + > + dev1 = litest_create_device_for(libinput, LITEST_GENERIC_HIGHRES_TOUCH); > + dev2 = litest_create_device_for(libinput, LITEST_GENERIC_HIGHRES_TOUCH); > + > + litest_touch_down(dev1, 0, 0, 0); > + litest_touch_down(dev2, 0, 0, 0); > + > + libinput_dispatch(libinput); > + > + /* > + * Read the two touch down events and ensure their seat slot are > + * unique. > + */ > + while ((ev = libinput_get_event(libinput))) { > + if (libinput_event_get_type(ev) != LIBINPUT_EVENT_TOUCH_DOWN) > + continue; > + > + tev = libinput_event_get_touch_event(ev); > + > + ck_assert_int_eq(libinput_event_touch_get_slot(tev), 0); > + seat_slot = libinput_event_touch_get_seat_slot(tev); > + ck_assert_int_ge(seat_slot, 0); > + > + if (!has_last_seat_slot) { > + has_last_seat_slot = true; > + last_seat_slot = seat_slot; > + continue; > + } > + > + ck_assert_int_ne(seat_slot, last_seat_slot); > + > + libinput_dispatch(libinput); > + } we re-use seat slots, I think the tests should also trigger that case, not just the initial touch down. otherwise we may miss a bug where cleaning up and re-assinging the same touch doesn't do the right thing. > + > + ck_assert(has_last_seat_slot); > + ck_assert_int_ne(seat_slot, last_seat_slot); > + > + litest_delete_device(dev1); > + litest_delete_device(dev2); > + > + libinput_destroy(libinput); > +} > +END_TEST > + > +START_TEST(touch_seat_slot_drop) > +{ > + struct libinput *libinput; > + struct litest_device *dev; > + struct libinput_event *ev; > + struct libinput_event_touch *tev; > + const int num_devices = 5; > + struct litest_device *devices[num_devices]; > + int slot; > + int seat_slot; > + enum libinput_event_type type; > + int found = 0; > + int i; > + const int num_tps = num_devices * 16; this would need a comment that 16 == MAX_SLOTS > + int slot_count = 0; > + int slot_max_count; > + int32_t slots[num_tps]; > + > + libinput = litest_create_context(); > + > + /* Activate touch points from several devices in order to surpass per > + * device touch slot limit. */ > + for (i = 0; i < num_devices; ++i) { > + devices[i] = > + litest_create_device_for(libinput, > + LITEST_GENERIC_HIGHRES_TOUCH); > + } if you add a litest_drain_events() here then you should be able to abort on anything that's not a touch event in the while loops. > + > + for (i = 0; i < num_devices; ++i) { > + dev = devices[i]; > + for (slot = 0; slot < num_tps; ++slot) > + litest_touch_down(dev, slot, 0, 0); > + } > + > + libinput_dispatch(libinput); > + > + /* Wait for 200 ms so we'll get all down events first. */ > + usleep(200000); that seems odd. is this timeout really needed? doesn't the dispatch call above get all of the touch down events? > + > + for (i = 0; i < num_devices; ++i) { > + dev = devices[i]; > + for (slot = 0; slot < num_tps; ++slot) > + litest_touch_up(dev, slot); > + } > + > + libinput_dispatch(libinput); > + > + /* Expect up to `num_tps`, but allow fewer. */ > + memset(slots, 0, sizeof slots); > + while ((ev = libinput_get_event(libinput))) { > + type = libinput_event_get_type(ev); > + if (type == LIBINPUT_EVENT_TOUCH_DOWN) { > + tev = libinput_event_get_touch_event(ev); > + seat_slot = libinput_event_touch_get_seat_slot(tev); > + > + ck_assert_int_ge(seat_slot, 0); > + ck_assert_int_lt(slot_count, num_tps); > + ck_assert_int_eq(slots[slot_count], 0); > + > + /* Assert uniqueness */ > + for (slot = 0; slot < slot_count; ++slot) > + ck_assert_int_ne(seat_slot, slots[slot]); > + > + slots[slot_count] = seat_slot; > + ++slot_count; > + } else if (type == LIBINPUT_EVENT_TOUCH_UP) { > + break; since you have the break here anyway, you could just move the litest_touch_up() calls to below this loop, then you won't need the usleep. > + } > + > + libinput_dispatch(libinput); > + } > + > + ck_assert_notnull(ev); > + ck_assert_int_gt(slot_count, 0); > + > + slot_max_count = slot_count; > + > + /* Make sure order was not shuffled and we received up before all > + * down, and that we received as many up as down. */ > + do { > + switch (libinput_event_get_type(ev)) { hmm, switch here but if/else in the previous loop. > + case LIBINPUT_EVENT_TOUCH_DOWN: > + ck_abort(); > + break; > + case LIBINPUT_EVENT_TOUCH_UP: > + tev = libinput_event_get_touch_event(ev); > + seat_slot = libinput_event_touch_get_seat_slot(tev); > + > + --slot_count; > + found = 0; > + for (slot = 0; slot < slot_max_count; ++slot) > + if (slots[slot] == seat_slot) > + ++found; > + ck_assert_int_eq(found, 1); > + break; > + default: > + break; > + } > + > + libinput_dispatch(libinput); > + > + if (slot_count == 0) > + break; > + } while ((ev = libinput_get_event(libinput))); > + > + /* Make sure we didn't receive too many up. */ > + while ((ev = libinput_get_event(libinput))) { > + type = libinput_event_get_type(ev); > + ck_assert_int_ne(type, LIBINPUT_EVENT_TOUCH_DOWN); > + ck_assert_int_ne(type, LIBINPUT_EVENT_TOUCH_UP); > + } there shouldn't be any event in the queue now, right? you haven't killed the devices, so any other events are probably not good. > + > + ck_assert_int_eq(slot_count, 0); > + > + for (i = 0; i < num_devices; ++i) > + litest_delete_device(devices[i]); > + > + libinput_destroy(libinput); > + > + fprintf(stderr, > + "libinput supports %s%d simultaneous touch seat slots\n", > + slot_max_count == num_tps ? ">=" : "", slot_max_count); is this debugging leftover? If not, this seems like something we could just export elsewhere for general use instead of printing it on every test run. (./tools/libinput-capabilities or so, though I do wonder who'd really need it) Cheers, Peter > +} > +END_TEST > > int > main(int argc, char **argv) > @@ -99,6 +281,8 @@ main(int argc, char **argv) > litest_add("touch:frame", touch_frame_events, LITEST_TOUCH, LITEST_ANY); > litest_add("touch:abs-transform", touch_abs_transform, > LITEST_TOUCH, LITEST_ANY); > + litest_add_no_device("touch:seat-slot", touch_seat_slots); > + litest_add_no_device("touch:seat-slot-drop", touch_seat_slot_drop); > > return litest_run(argc, argv); > } > -- > 1.8.3.2 > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel