On Mon, Dec 11, 2017 at 02:39:01PM +0000, Eric Engestrom wrote:
> On Friday, 2017-12-08 10:17:17 +1000, Peter Hutterer wrote:
> > Commit db3b6fe5f7f8 "fallback: change to handle the state at EV_SYN time"
> > introduced regressions for two types of event sequences.
> >
> > One is a kernel bug - some devices/drivers like the asus-wireless send a key
> > press + release within the same event frame which now cancels out and
> > disappears into the ether. This should be fixed in the kernel drivers but
> > there appear to be enough of them that we can't just pretend it's an
> > outlier.
> >
> > The second issue is a libinput bug. If we get two key events in the same
> > frame
> > (e.g. shift + A) we update the state correctly but the events are sent in
> > the
> > order of the event codes. KEY_A sorts before KEY_LEFTSHIFT and our shift + A
> > becomes A + shift.
> >
> > Fix this by treating key events as before db3b6fe5f7f8 - by sending them out
> > as we get them.
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=104030
> >
> > Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> > ---
> > src/evdev-fallback.c | 35 ++++++++++++++++----------------
> > test/test-keyboard.c | 56
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 73 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/evdev-fallback.c b/src/evdev-fallback.c
> > index 52a3bde1..0cd2497e 100644
> > --- a/src/evdev-fallback.c
> > +++ b/src/evdev-fallback.c
> > @@ -501,6 +501,22 @@ fallback_process_key(struct fallback_dispatch
> > *dispatch,
> > }
> >
> > hw_set_key_down(dispatch, e->code, e->value);
> > +
> > + switch (type) {
> > + case KEY_TYPE_NONE:
> > + break;
> > + case KEY_TYPE_KEY:
> > + fallback_keyboard_notify_key(
> > + dispatch,
> > + device,
> > + time,
> > + e->code,
> > + e->value ? LIBINPUT_KEY_STATE_PRESSED :
> > + LIBINPUT_KEY_STATE_RELEASED);
> > + break;
> > + case KEY_TYPE_BUTTON:
> > + break;
> > + }
> > }
> >
> > static void
> > @@ -834,27 +850,10 @@ fallback_handle_state(struct fallback_dispatch
> > *dispatch,
> > if (dispatch->pending_event & EVDEV_KEY) {
> > bool want_debounce = false;
> > for (unsigned int code = 0; code <= KEY_MAX; code++) {
> > - bool new_state;
> > -
> > if (!hw_key_has_changed(dispatch, code))
> > continue;
> >
> > - new_state = hw_is_key_down(dispatch, code);
> > -
> > - switch (get_key_type(code)) {
> > - case KEY_TYPE_NONE:
> > - break;
> > - case KEY_TYPE_KEY:
> > - fallback_keyboard_notify_key(
> > - dispatch,
> > - device,
> > - time,
> > - code,
> > - new_state ?
> > -
> > LIBINPUT_KEY_STATE_PRESSED :
> > -
> > LIBINPUT_KEY_STATE_RELEASED);
> > - break;
> > - case KEY_TYPE_BUTTON:
> > + if (get_key_type(code) == KEY_TYPE_BUTTON) {
> > want_debounce = true;
> > break;
>
> This will break out of the `for()` now :)
yep, good catch. that's intentional though. Previously, the loop did two
things: send out key events and check for want_debounce. Now it doesn't
send key events anymore so as soon as we know that we want to debounce, we
can exit the loop.
Cheers,
Peter
>
> > }
> > diff --git a/test/test-keyboard.c b/test/test-keyboard.c
> > index dc2e630e..db836381 100644
> > --- a/test/test-keyboard.c
> > +++ b/test/test-keyboard.c
> > @@ -365,6 +365,61 @@ START_TEST(keyboard_no_buttons)
> > }
> > END_TEST
> >
> > +START_TEST(keyboard_frame_order)
> > +{
> > + struct litest_device *dev = litest_current_device();
> > + struct libinput *li = dev->libinput;
> > +
> > + if (!libevdev_has_event_code(dev->evdev, EV_KEY, KEY_A) ||
> > + !libevdev_has_event_code(dev->evdev, EV_KEY, KEY_LEFTSHIFT))
> > + return;
> > +
> > + litest_drain_events(li);
> > +
> > + litest_event(dev, EV_KEY, KEY_LEFTSHIFT, 1);
> > + litest_event(dev, EV_KEY, KEY_A, 1);
> > + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> > + libinput_dispatch(li);
> > +
> > + litest_assert_key_event(li,
> > + KEY_LEFTSHIFT,
> > + LIBINPUT_KEY_STATE_PRESSED);
> > + litest_assert_key_event(li, KEY_A, LIBINPUT_KEY_STATE_PRESSED);
> > +
> > + litest_event(dev, EV_KEY, KEY_LEFTSHIFT, 0);
> > + litest_event(dev, EV_KEY, KEY_A, 0);
> > + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> > + libinput_dispatch(li);
> > +
> > + litest_assert_key_event(li,
> > + KEY_LEFTSHIFT,
> > + LIBINPUT_KEY_STATE_RELEASED);
> > + litest_assert_key_event(li, KEY_A, LIBINPUT_KEY_STATE_RELEASED);
> > +
> > + litest_event(dev, EV_KEY, KEY_A, 1);
> > + litest_event(dev, EV_KEY, KEY_LEFTSHIFT, 1);
> > + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> > + libinput_dispatch(li);
> > +
> > + litest_assert_key_event(li, KEY_A, LIBINPUT_KEY_STATE_PRESSED);
> > + litest_assert_key_event(li,
> > + KEY_LEFTSHIFT,
> > + LIBINPUT_KEY_STATE_PRESSED);
> > +
> > + litest_event(dev, EV_KEY, KEY_A, 0);
> > + litest_event(dev, EV_KEY, KEY_LEFTSHIFT, 0);
> > + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> > + libinput_dispatch(li);
> > +
> > + litest_assert_key_event(li, KEY_A, LIBINPUT_KEY_STATE_RELEASED);
> > + litest_assert_key_event(li,
> > + KEY_LEFTSHIFT,
> > + LIBINPUT_KEY_STATE_RELEASED);
> > +
> > + libinput_dispatch(li);
> > +}
> > +END_TEST
> > +
> > START_TEST(keyboard_leds)
> > {
> > struct litest_device *dev = litest_current_device();
> > @@ -432,6 +487,7 @@ litest_setup_tests_keyboard(void)
> > litest_add("keyboard:time", keyboard_time_usec, LITEST_KEYS,
> > LITEST_ANY);
> >
> > litest_add("keyboard:events", keyboard_no_buttons, LITEST_KEYS,
> > LITEST_ANY);
> > + litest_add("keyboard:events", keyboard_frame_order, LITEST_KEYS,
> > LITEST_ANY);
> >
> > litest_add("keyboard:leds", keyboard_leds, LITEST_ANY, LITEST_ANY);
> >
> > --
> > 2.13.6
> >
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
