On Sun, Jul 27, 2014 at 11:28:30PM +0200, Jonas Ådahl wrote:
> When removing a device, its not guaranteed that all button or key
> presses have been released, resulting in an invalid seat wide button
> count.
>
> Note that kernel devices normally will send release events when being
> unplugged, but this won't happen when removing a device from the path
> backend.
>
> Signed-off-by: Jonas Ådahl <jad...@gmail.com>
> ---
> src/evdev.c | 47 +++++++++++++++++++++++
> test/keyboard.c | 116
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> test/pointer.c | 90 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 253 insertions(+)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index f656a5e..ade7eec 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -60,6 +60,12 @@ is_key_pressed(struct evdev_device *device, int code)
> }
>
> static int
> +get_key_pressed_count(struct evdev_device *device, int code)
here too: pressed vs down
> +{
> + return device->key_count[code];
> +}
do we still need is_key_pressed then? can we drop the mask?
> +
> +static int
> update_key_pressed_count(struct evdev_device *device, int code, int pressed)
> {
> assert(code >= 0 && code < KEY_CNT);
> @@ -331,6 +337,45 @@ get_key_type(uint16_t code)
> }
>
> static void
> +release_pressed_keys(struct evdev_device *device)
> +{
> + struct libinput *libinput = device->base.seat->libinput;
> + struct timespec ts;
> + uint64_t time;
> + int code;
> +
> + if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) {
> + log_bug_libinput(libinput, "clock_gettime: %s\n",
> strerror(errno));
> + return;
> + }
> +
> + time = ts.tv_sec * 1000ULL + ts.tv_nsec / 1000000;
> +
> + for (code = 0; code < KEY_CNT; code++) {
> + if (get_key_pressed_count(device, code) > 0) {
> + switch (get_key_type(code)) {
> + case EVDEV_KEY_TYPE_NONE:
> + break;
> + case EVDEV_KEY_TYPE_KEY:
> + evdev_keyboard_notify_key(
> + device,
> + time,
> + code,
> + LIBINPUT_KEY_STATE_RELEASED);
> + break;
> + case EVDEV_KEY_TYPE_BUTTON:
> + evdev_pointer_notify_button(
> + device,
> + time,
> + code,
> + LIBINPUT_BUTTON_STATE_RELEASED);
> + break;
> + }
> + }
> + }
> +}
> +
> +static void
> evdev_process_touch_button(struct evdev_device *device,
> uint64_t time, int value)
> {
> @@ -1008,6 +1053,8 @@ evdev_device_remove(struct evdev_device *device)
> libinput_remove_source(device->base.seat->libinput,
> device->source);
>
> + release_pressed_keys(device);
> +
> if (device->mtdev)
> mtdev_close_delete(device->mtdev);
> close_restricted(device->base.seat->libinput, device->fd);
> diff --git a/test/keyboard.c b/test/keyboard.c
> index a7f500c..dc9c4ea 100644
> --- a/test/keyboard.c
> +++ b/test/keyboard.c
> @@ -25,6 +25,7 @@
> #include <check.h>
> #include <stdio.h>
>
> +#include "libinput-util.h"
> #include "litest.h"
>
> START_TEST(keyboard_seat_key_count)
> @@ -163,11 +164,126 @@ START_TEST(keyboard_ignore_no_pressed_release)
> }
> END_TEST
>
> +static void
> +test_key_event(struct litest_device *dev, unsigned int key, int state)
> +{
> + struct libinput *li = dev->libinput;
> + struct libinput_event *event;
> + struct libinput_event_keyboard *kevent;
> +
> + litest_event(dev, EV_KEY, key, state);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> +
> + libinput_dispatch(li);
> +
> + event = libinput_get_event(li);
> + ck_assert(event != NULL);
> + ck_assert_int_eq(libinput_event_get_type(event),
> LIBINPUT_EVENT_KEYBOARD_KEY);
> +
> + kevent = libinput_event_get_keyboard_event(event);
> + ck_assert(kevent != NULL);
> + ck_assert_int_eq(libinput_event_keyboard_get_key(kevent), key);
> + ck_assert_int_eq(libinput_event_keyboard_get_key_state(kevent),
> + state ? LIBINPUT_KEY_STATE_PRESSED :
> + LIBINPUT_KEY_STATE_RELEASED);
> + libinput_event_destroy(event);
> +}
> +
> +START_TEST(keyboard_key_auto_release)
> +{
> + struct libinput *libinput;
> + struct litest_device *dev;
> + struct libinput_event *event;
> + enum libinput_event_type type;
> + struct libinput_event_keyboard *kevent;
> + struct {
> + int code;
> + int released;
> + } keys[] = {
> + { .code = KEY_A, },
> + { .code = KEY_S, },
> + { .code = KEY_D, },
> + { .code = KEY_G, },
> + { .code = KEY_Z, },
> + { .code = KEY_DELETE, },
> + { .code = KEY_F24, },
> + };
> + int events[2 * (ARRAY_LENGTH(keys) + 1)];
> + unsigned i;
> + int key;
> + int valid_code;
> +
> + /* Enable all tested keys on the device */
> + for (i = 0; i < 2 * ARRAY_LENGTH(keys);) {
> + key = keys[i / 2].code;
> + events[i++] = EV_KEY;
> + events[i++] = key;
hmm, advancing i manually in a for loop is normally a big no-no.
any reason you don't just use a while loop here?
> + }
> + events[i++] = -1;
> + events[i++] = -1;
> +
> + libinput = litest_create_context();
> + dev = litest_add_device_with_overrides(libinput,
> + LITEST_KEYBOARD,
> + "Generic keyboard",
> + NULL, NULL, events);
> +
> + litest_drain_events(libinput);
> +
> + /* Send pressed events, without releasing */
> + for (i = 0; i < ARRAY_LENGTH(keys); ++i) {
> + test_key_event(dev, keys[i].code, 1);
> + }
> +
> + litest_drain_events(libinput);
> +
> + /* "Disconnect" device */
> + litest_delete_device(dev);
> +
> + /* Mark all released keys until device is removed */
> + while (1) {
> + event = libinput_get_event(libinput);
> + ck_assert_notnull(event);
> + type = libinput_event_get_type(event);
> +
> + if (type == LIBINPUT_EVENT_DEVICE_REMOVED) {
> + libinput_event_destroy(event);
> + break;
> + }
> +
> + ck_assert_int_eq(type, LIBINPUT_EVENT_KEYBOARD_KEY);
> + kevent = libinput_event_get_keyboard_event(event);
> + ck_assert_int_eq(libinput_event_keyboard_get_key_state(kevent),
> + LIBINPUT_KEY_STATE_RELEASED);
> + key = libinput_event_keyboard_get_key(kevent);
> +
> + valid_code = 0;
> + for (i = 0; i < ARRAY_LENGTH(keys); ++i) {
> + if (keys[i].code == key) {
> + ck_assert_int_eq(keys[i].released, 0);
> + keys[i].released = 1;
> + valid_code = 1;
> + }
> + }
just fyi, if you name the struct you could use ARRAY_FOR_EACH here, not that
it matters that much.
with the for/while loop change (in both tests):
Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>
Cheers,
Peterk
> + ck_assert_int_eq(valid_code, 1);
> + libinput_event_destroy(event);
> + }
> +
> + /* Check that all pressed keys has been released. */
> + for (i = 0; i < ARRAY_LENGTH(keys); ++i) {
> + ck_assert_int_eq(keys[i].released, 1);
> + }
> +
> + libinput_unref(libinput);
> +}
> +END_TEST
> +
> int
> main(int argc, char **argv)
> {
> litest_add_no_device("keyboard:seat key count",
> keyboard_seat_key_count);
> litest_add_no_device("keyboard:ignore no pressed release",
> keyboard_ignore_no_pressed_release);
> + litest_add_no_device("keyboard:key_auto_release",
> keyboard_key_auto_release);
>
> return litest_run(argc, argv);
> }
> diff --git a/test/pointer.c b/test/pointer.c
> index aa75274..c0af460 100644
> --- a/test/pointer.c
> +++ b/test/pointer.c
> @@ -152,6 +152,95 @@ START_TEST(pointer_button)
> }
> END_TEST
>
> +START_TEST(pointer_button_auto_release)
> +{
> + struct libinput *libinput;
> + struct litest_device *dev;
> + struct libinput_event *event;
> + enum libinput_event_type type;
> + struct libinput_event_pointer *pevent;
> + struct {
> + int code;
> + int released;
> + } buttons[] = {
> + { .code = BTN_LEFT, },
> + { .code = BTN_MIDDLE, },
> + { .code = BTN_EXTRA, },
> + { .code = BTN_SIDE, },
> + { .code = BTN_BACK, },
> + { .code = BTN_FORWARD, },
> + { .code = BTN_4, },
> + };
> + int events[2 * (ARRAY_LENGTH(buttons) + 1)];
> + unsigned i;
> + int button;
> + int valid_code;
> +
> + /* Enable all tested buttons on the device */
> + for (i = 0; i < 2 * ARRAY_LENGTH(buttons);) {
> + button = buttons[i / 2].code;
> + events[i++] = EV_KEY;
> + events[i++] = button;
> + }
> + events[i++] = -1;
> + events[i++] = -1;
> +
> + libinput = litest_create_context();
> + dev = litest_add_device_with_overrides(libinput,
> + LITEST_MOUSE,
> + "Generic mouse",
> + NULL, NULL, events);
> +
> + litest_drain_events(libinput);
> +
> + /* Send pressed events, without releasing */
> + for (i = 0; i < ARRAY_LENGTH(buttons); ++i) {
> + test_button_event(dev, buttons[i].code, 1);
> + }
> +
> + litest_drain_events(libinput);
> +
> + /* "Disconnect" device */
> + litest_delete_device(dev);
> +
> + /* Mark all released buttons until device is removed */
> + while (1) {
> + event = libinput_get_event(libinput);
> + ck_assert_notnull(event);
> + type = libinput_event_get_type(event);
> +
> + if (type == LIBINPUT_EVENT_DEVICE_REMOVED) {
> + libinput_event_destroy(event);
> + break;
> + }
> +
> + ck_assert_int_eq(type, LIBINPUT_EVENT_POINTER_BUTTON);
> + pevent = libinput_event_get_pointer_event(event);
> +
> ck_assert_int_eq(libinput_event_pointer_get_button_state(pevent),
> + LIBINPUT_BUTTON_STATE_RELEASED);
> + button = libinput_event_pointer_get_button(pevent);
> +
> + valid_code = 0;
> + for (i = 0; i < ARRAY_LENGTH(buttons); ++i) {
> + if (buttons[i].code == button) {
> + ck_assert_int_eq(buttons[i].released, 0);
> + buttons[i].released = 1;
> + valid_code = 1;
> + }
> + }
> + ck_assert_int_eq(valid_code, 1);
> + libinput_event_destroy(event);
> + }
> +
> + /* Check that all pressed buttons has been released. */
> + for (i = 0; i < ARRAY_LENGTH(buttons); ++i) {
> + ck_assert_int_eq(buttons[i].released, 1);
> + }
> +
> + libinput_unref(libinput);
> +}
> +END_TEST
> +
> static void
> test_wheel_event(struct litest_device *dev, int which, int amount)
> {
> @@ -300,6 +389,7 @@ int main (int argc, char **argv) {
>
> litest_add("pointer:motion", pointer_motion_relative, LITEST_POINTER,
> LITEST_ANY);
> litest_add("pointer:button", pointer_button, LITEST_BUTTON,
> LITEST_CLICKPAD);
> + litest_add_no_device("pointer:button_auto_release",
> pointer_button_auto_release);
> litest_add("pointer:scroll", pointer_scroll_wheel, LITEST_WHEEL,
> LITEST_ANY);
> litest_add_no_device("pointer:seat button count",
> pointer_seat_button_count);
>
> --
> 1.8.5.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
