[PATCH libinput 1/4] evdev: Ignore key/button release events if key was never pressed
Peter Hutterer
peter.hutterer at who-t.net
Sun Aug 3 22:04:06 PDT 2014
On Sun, Jul 27, 2014 at 11:28:28PM +0200, Jonas Ådahl wrote:
> The kernel may send a 'release' event without ever having sent a key
> 'pressed' event in case the key was pressed before libinput was
> initiated. Ignore these events so that we always guarantee a release
> event always comes after a pressed event for any given key or button.
>
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
> src/evdev.c | 33 ++++++++++++++++++++++++++++++++-
> src/evdev.h | 2 ++
> src/libinput-util.h | 29 +++++++++++++++++++++++++++++
> test/keyboard.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index 5872856..0f4874c 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -47,6 +47,18 @@ enum evdev_key_type {
> EVDEV_KEY_TYPE_BUTTON,
> };
>
> +static void
> +set_key_pressed(struct evdev_device *device, int code, int pressed)
> +{
> + long_set_bit_state(device->key_mask, code, pressed);
> +}
> +
> +static int
> +is_key_pressed(struct evdev_device *device, int code)
> +{
> + return long_bit_is_set(device->key_mask, code);
> +}
> +
I'd prefer for these to be "is_key_down" to avoid any ever-so-slight ambiguity
about whether pressed means currently down, pressed in the past, etc.
but if you want to leave it as-is, that's fine too.
> void
> evdev_device_led_update(struct evdev_device *device, enum libinput_led leds)
> {
> @@ -294,6 +306,8 @@ static inline void
> evdev_process_key(struct evdev_device *device,
> struct input_event *e, uint64_t time)
> {
> + enum evdev_key_type type;
> +
> /* ignore kernel key repeat */
> if (e->value == 2)
> return;
> @@ -306,7 +320,24 @@ evdev_process_key(struct evdev_device *device,
>
> evdev_flush_pending_event(device, time);
>
> - switch (get_key_type(e->code)) {
> + type = get_key_type(e->code);
> +
> + /* Ignore key release events from the kernel for keys that libinput
> + * never got a pressed event for. */
> + if (e->value == 0) {
> + switch (type) {
> + case EVDEV_KEY_TYPE_NONE:
> + break;
> + case EVDEV_KEY_TYPE_KEY:
> + case EVDEV_KEY_TYPE_BUTTON:
> + if (!is_key_pressed(device, e->code))
> + return;
> + }
> + }
> +
> + set_key_pressed(device, e->code, e->value);
> +
> + switch (type) {
> case EVDEV_KEY_TYPE_NONE:
> break;
> case EVDEV_KEY_TYPE_KEY:
> diff --git a/src/evdev.h b/src/evdev.h
> index fad1f84..f71d387 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -94,6 +94,8 @@ struct evdev_device {
> struct {
> struct motion_filter *filter;
> } pointer;
> +
> + unsigned long key_mask[NLONGS(KEY_CNT)];
> };
>
> #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
> diff --git a/src/libinput-util.h b/src/libinput-util.h
> index c0235ef..2f1a1db 100644
> --- a/src/libinput-util.h
> +++ b/src/libinput-util.h
> @@ -72,6 +72,8 @@ int list_empty(const struct list *list);
> pos = tmp, \
> tmp = container_of(pos->member.next, tmp, member))
>
> +#define LONG_BITS (sizeof(long) * 8)
> +#define NLONGS(x) (((x) + LONG_BITS - 1) / LONG_BITS)
> #define ARRAY_LENGTH(a) (sizeof (a) / sizeof (a)[0])
> #define ARRAY_FOR_EACH(_arr, _elem) \
> for (size_t _i = 0; (_elem = &_arr[_i]) && _i < ARRAY_LENGTH(_arr); _i++)
> @@ -150,4 +152,31 @@ vector_get_direction(int dx, int dy)
> return dir;
> }
>
> +static inline int
> +long_bit_is_set(const unsigned long *array, int bit)
> +{
> + return !!(array[bit / LONG_BITS] & (1LL << (bit % LONG_BITS)));
> +}
> +
> +static inline void
> +long_set_bit(unsigned long *array, int bit)
> +{
> + array[bit / LONG_BITS] |= (1LL << (bit % LONG_BITS));
> +}
> +
> +static inline void
> +long_clear_bit(unsigned long *array, int bit)
> +{
> + array[bit / LONG_BITS] &= ~(1LL << (bit % LONG_BITS));
> +}
> +
> +static inline void
> +long_set_bit_state(unsigned long *array, int bit, int state)
> +{
> + if (state)
> + long_set_bit(array, bit);
> + else
> + long_clear_bit(array, bit);
> +}
> +
> #endif /* LIBINPUT_UTIL_H */
> diff --git a/test/keyboard.c b/test/keyboard.c
> index a55405c..a7f500c 100644
> --- a/test/keyboard.c
> +++ b/test/keyboard.c
> @@ -112,10 +112,62 @@ START_TEST(keyboard_seat_key_count)
> }
> END_TEST
>
> +START_TEST(keyboard_ignore_no_pressed_release)
> +{
> + struct litest_device *dev;
> + struct libinput *libinput;
> + struct libinput_event *event;
> + struct libinput_event_keyboard *kevent;
> + int events[] = {
> + EV_KEY, KEY_A,
> + -1, -1,
> + };
> + enum libinput_key_state *state;
> + enum libinput_key_state expected_states[] = {
> + LIBINPUT_KEY_STATE_PRESSED,
> + LIBINPUT_KEY_STATE_RELEASED,
> + };
> +
> +
> + libinput = litest_create_context();
> + dev = litest_add_device_with_overrides(libinput,
> + LITEST_KEYBOARD,
> + "Generic keyboard",
> + NULL, NULL, events);
> +
> + litest_drain_events(libinput);
> +
> + litest_keyboard_key(dev, KEY_A, false);
> + litest_keyboard_key(dev, KEY_A, true);
> + litest_keyboard_key(dev, KEY_A, false);
> +
> + libinput_dispatch(libinput);
I don't think this tests what you want to test. the kernel should drop the
first key release event, so this test should pass even without your patch
in. for this to work you need to manually create a uinput device and then
create the context after sending the first key down event.
or, a bit easier, create a litest for LITEST_KEYBOARD device and create a
second libinput context after sending the key down event and then make sure the
litest->libinput context gets two pairs and the second context only one
pair.
> +
> + ARRAY_FOR_EACH(expected_states, state) {
> + event = libinput_get_event(libinput);
> + ck_assert_notnull(event);
> + ck_assert_int_eq(libinput_event_get_type(event),
> + LIBINPUT_EVENT_KEYBOARD_KEY);
> + kevent = libinput_event_get_keyboard_event(event);
> + ck_assert_int_eq(libinput_event_keyboard_get_key(kevent),
> + KEY_A);
> + ck_assert_int_eq(libinput_event_keyboard_get_key_state(kevent),
> + *state);
> + libinput_event_destroy(event);
> + libinput_dispatch(libinput);
> + }
> +
> + litest_assert_empty_queue(libinput);
> + litest_delete_device(dev);
> + 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);
fwiw, the first arg here is a suite name, so we don't need a separate one
for each test. "keyboard:key counting" would be ok for example, together
with other tests that do various key counting things.
Cheers,
Peter
>
> return litest_run(argc, argv);
> }
> --
> 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