[PATCH libinput 3/4] evdev: Release still pressed keys/buttons when removing device
Jonas Ådahl
jadahl at gmail.com
Sun Aug 10 03:27:07 PDT 2014
On Mon, Aug 04, 2014 at 03:23:32PM +1000, Peter Hutterer wrote:
> 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 <jadahl at 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
Renamed.
>
> > +{
> > + return device->key_count[code];
> > +}
>
> do we still need is_key_pressed then? can we drop the mask?
The mask is to sort out the initial 'released' we'd get if a key is
pressed when we create or resume a libinput context. The counter here is
ment to help with multiplexing multiple sources of identical key presses
on the same device, and as such needs symmetric input, which it wouldn't
get without it.
On the other hand, we also have libinput_device_get_keys() to deal with
this, but IMO, we should deprecate that function in favour of always
providing symmetric key presses on a device. (Also I just noticed I
break _get_keys() with the mask patch as it should return only zeros
with it.)
We could also drop the mask and generate 'pressed' events when adding a
device; can we do that reliably without risking a race condition?
>
> > +
> > +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?
No reason, I'll fix it.
>
> > + }
> > + 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.
Hmm. I wonder why I didn't use that one here.
>
> with the for/while loop change (in both tests):
> Reviewed-by: Peter Hutterer <peter.hutterer at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
More information about the wayland-devel
mailing list