[PATCH libinput 1/4] evdev: Ignore key/button release events if key was never pressed

Jonas Ådahl jadahl at gmail.com
Sun Aug 10 03:16:29 PDT 2014


On Mon, Aug 04, 2014 at 03:04:06PM +1000, Peter Hutterer wrote:
> 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.

Sure, I'll rename it.

> 
> >  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.

A, right. Thanks for noticing. I changed it to do it this way.

> 
> > +
> > +	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.

Fixed.

> 
> 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