[PATCH libinput 3/4] evdev: Release still pressed keys/buttons when removing device
Peter Hutterer
peter.hutterer at who-t.net
Sun Aug 10 22:39:08 PDT 2014
On Sun, Aug 10, 2014 at 12:27:07PM +0200, Jonas Ådahl wrote:
> 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.
can you pop a comment in that one is to mask events from the kernel, the
other one to count events from within libinput? That'd help greatly, I'm
sure a few months down the track someone will wonder why we have
two separate structs that seem to do the same thing.
> 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?
technically yes, but I'm not sure it is a good idea. I think it's better to
ignore any keys down when the device is added and wait for the next press.
the side-effects of out-of-order key events are greater than the benefits
here (e.g. was it Backspace, then Ctrl + Alt on resume or Ctrl + Alt, then
Backspace?)
So you're right, the mask is the best solution.
Cheers,
Peter
> > > +
> > > +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