[PATCH weston 4/4] input: Detect keyboard capabilities
Jonas Ådahl
jadahl at gmail.com
Wed May 20 19:28:52 PDT 2015
On Wed, May 20, 2015 at 12:32:46PM -0500, Derek Foreman wrote:
> Thanks for taking a look!
>
> On 20/05/15 05:04 AM, Jonas Ådahl wrote:
> > On Tue, May 05, 2015 at 03:01:54PM -0500, Derek Foreman wrote:
> >> Some devices (power buttons, acpi video bus driver) are considered
> >> keyboards but you can't type with them.
> >>
> >> As of libinput 0.15 we can query a keyboard to see which key events it
> >> can generate. We use this to detect if a keyboard can type letters or
> >> digits.
> >>
> >> The wayland protocol doesn't propagate these capabilites, so
> >> weston_seat_send_dirty_caps() will differentiate between dirty keyboards
> >> (which generate the weston signal) and dirty seats (which generate a
> >> wayland event) and only send the appropriate updates.
> >>
> >> Functions are provided for backends that don't use libinput to force these
> >> capabilities to a sensible state.
> >>
> >> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> >> ---
> >> configure.ac | 2 +-
> >> src/compositor-headless.c | 1 +
> >> src/compositor-rdp.c | 1 +
> >> src/compositor-wayland.c | 1 +
> >> src/compositor-x11.c | 1 +
> >> src/compositor.h | 8 ++++++++
> >> src/input.c | 29 +++++++++++++++++++++++++++--
> >> src/libinput-device.c | 32 +++++++++++++++++++++++++++++++-
> >> src/libinput-device.h | 4 +++-
> >> src/libinput-seat.c | 27 +++++++++++++++++++++++++++
> >> src/screen-share.c | 1 +
> >> 11 files changed, 102 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/configure.ac b/configure.ac
> >> index d9d8d8f..4e9ae20 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -155,7 +155,7 @@ if test x$enable_drm_compositor = xyes; then
> >> fi
> >>
> >>
> >> -PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.8.0])
> >> +PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.15.0])
> >> PKG_CHECK_MODULES(COMPOSITOR, [$COMPOSITOR_MODULES])
> >>
> >> AC_ARG_ENABLE(wayland-compositor, [ --enable-wayland-compositor],,
> >> diff --git a/src/compositor-headless.c b/src/compositor-headless.c
> >> index 0ddb26e..0b461c6 100644
> >> --- a/src/compositor-headless.c
> >> +++ b/src/compositor-headless.c
> >> @@ -184,6 +184,7 @@ headless_input_create(struct headless_compositor *c)
> >> if (weston_seat_init_keyboard(&c->fake_seat, NULL) < 0)
> >> return -1;
> >>
> >> + weston_seat_override_keyboard_caps(&c->fake_seat, true, true);
> >> weston_seat_send_dirty_caps(&c->fake_seat);
> >>
> >> return 0;
> >> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> >> index 5b50382..b6e3876 100644
> >> --- a/src/compositor-rdp.c
> >> +++ b/src/compositor-rdp.c
> >> @@ -842,6 +842,7 @@ xf_peer_post_connect(freerdp_peer* client)
> >> }
> >> weston_seat_init_keyboard(&peerCtx->item.seat, keymap);
> >> weston_seat_init_pointer(&peerCtx->item.seat);
> >> + weston_seat_override_keyboard_caps(&peerCtx->item.seat, true, true);
> >> weston_seat_send_dirty_caps(&peerCtx->item.seat);
> >>
> >> peerCtx->item.flags |= RDP_PEER_ACTIVATED;
> >> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> >> index 53159c4..1e33697 100644
> >> --- a/src/compositor-wayland.c
> >> +++ b/src/compositor-wayland.c
> >> @@ -1463,6 +1463,7 @@ input_handle_keymap(void *data, struct wl_keyboard *keyboard, uint32_t format,
> >> else
> >> weston_seat_init_keyboard(&input->base, keymap);
> >>
> >> + weston_seat_override_keyboard_caps(&input->base, true, true);
> >> weston_seat_send_dirty_caps(&input->base);
> >>
> >> xkb_keymap_unref(keymap);
> >> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> >> index 4a3b10e..69e3a14 100644
> >> --- a/src/compositor-x11.c
> >> +++ b/src/compositor-x11.c
> >> @@ -336,6 +336,7 @@ x11_input_create(struct x11_compositor *c, int no_input)
> >> return -1;
> >> xkb_keymap_unref(keymap);
> >>
> >> + weston_seat_override_keyboard_caps(&c->core_seat, true, true);
> >> weston_seat_send_dirty_caps(&c->core_seat);
> >>
> >> x11_compositor_setup_xkb(c);
> >> diff --git a/src/compositor.h b/src/compositor.h
> >> index e05b262..da6d126 100644
> >> --- a/src/compositor.h
> >> +++ b/src/compositor.h
> >> @@ -451,6 +451,10 @@ weston_touch_start_drag(struct weston_touch *touch,
> >> struct wl_client *client);
> >> void
> >> weston_seat_send_dirty_caps(struct weston_seat *seat);
> >> +void
> >> +weston_seat_override_keyboard_caps(struct weston_seat *seat,
> >> + bool has_letters,
> >> + bool has_digits);
> >>
> >> struct weston_xkb_info {
> >> struct xkb_keymap *keymap;
> >> @@ -505,6 +509,10 @@ struct weston_keyboard {
> >> enum weston_led leds;
> >> } xkb_state;
> >> struct xkb_keymap *pending_keymap;
> >> +
> >> + bool has_digits;
> >> + bool has_letters;
> >> + bool caps_dirty;
> >> };
> >>
> >> struct weston_seat {
> >> diff --git a/src/input.c b/src/input.c
> >> index c37bd20..3a0adc3 100644
> >> --- a/src/input.c
> >> +++ b/src/input.c
> >> @@ -601,14 +601,16 @@ weston_touch_destroy(struct weston_touch *touch)
> >>
> >> /* Send seat capability updates if necessary
> >> *
> >> - * Checks if the seat capabilities (WL_SEAT_CAPABILITY_*) have changed
> >> - * and propagates updates appropriately.
> >> + * Checks if the seat capabilities (WL_SEAT_CAPABILITY_*) or the keyboard
> >> + * capabilities (has_digits, has_letters) have changed and propagates
> >> + * updates appropriately.
> >> *
> >> * \param seat These seat to send capabilities changes for
> >> */
> >> WL_EXPORT void
> >> weston_seat_send_dirty_caps(struct weston_seat *seat)
> >> {
> >> + struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> >> enum wl_seat_capability caps = 0;
> >> struct wl_resource *resource;
> >>
> >> @@ -622,8 +624,15 @@ weston_seat_send_dirty_caps(struct weston_seat *seat)
> >>
> >> wl_resource_for_each(resource, &seat->base_resource_list)
> >> wl_seat_send_capabilities(resource, caps);
> >> + }
> >> +
> >> + if (seat->caps_dirty || (keyboard && keyboard->caps_dirty)) {
> >> wl_signal_emit(&seat->updated_caps_signal, seat);
> >> }
> >> +
> >> + if (keyboard)
> >> + keyboard->caps_dirty = false;
> >> +
> >> seat->caps_dirty = false;
> >> }
> >>
> >> @@ -2230,6 +2239,22 @@ weston_seat_release_keyboard(struct weston_seat *seat)
> >> }
> >> }
> >>
> >> +void
> >> +weston_seat_override_keyboard_caps(struct weston_seat *seat,
> >> + bool has_letters,
> >> + bool has_digits)
> >> +{
> >> + struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> >> +
> >> + if (!keyboard)
> >> + return;
> >> +
> >> + keyboard->caps_dirty = has_digits ^ keyboard->has_digits ||
> >> + has_letters ^ keyboard->has_letters;
> >> + keyboard->has_digits = has_digits;
> >> + keyboard->has_letters = has_letters;
> >> +}
> >> +
> >> WL_EXPORT void
> >> weston_seat_init_pointer(struct weston_seat *seat)
> >> {
> >> diff --git a/src/libinput-device.c b/src/libinput-device.c
> >> index 3fb507f..983665c 100644
> >> --- a/src/libinput-device.c
> >> +++ b/src/libinput-device.c
> >> @@ -480,6 +480,36 @@ configure_device(struct evdev_device *device)
> >> evdev_device_set_calibration(device);
> >> }
> >>
> >> +static enum evdev_device_seat_capability
> >> +keyboard_caps(struct libinput_device *dev)
> >> +{
> >> + bool letters = true, digits = true;
> >> + uint32_t alpha_keys[] = {
> >> + KEY_A, KEY_B, KEY_C, KEY_D, KEY_E, KEY_F,
> >> + KEY_G, KEY_H, KEY_I, KEY_J, KEY_K, KEY_L,
> >> + KEY_M, KEY_N, KEY_O, KEY_P, KEY_Q, KEY_R,
> >> + KEY_S, KEY_T, KEY_U, KEY_V, KEY_W, KEY_X,
> >> + KEY_Y, KEY_Z, KEY_RESERVED
> >> + };
> >> + uint32_t digit_keys[] = {
> >> + KEY_1, KEY_2, KEY_3, KEY_4, KEY_5, KEY_6,
> >> + KEY_7, KEY_8, KEY_9, KEY_0, KEY_RESERVED
> >> + };
> >> + int i;
> >> +
> >> + for (i = 0; letters && alpha_keys[i] != KEY_RESERVED; i++)
> >> + letters &= libinput_device_keyboard_has_key(dev,
> >> + alpha_keys[i]);
> >> +
> >> + for (i = 0; digits && digit_keys[i] != KEY_RESERVED; i++)
> >> + digits &= libinput_device_keyboard_has_key(dev,
> >> + digit_keys[i]);
> >> +
> >> + return EVDEV_SEAT_KEYBOARD |
> >> + (letters ? EVDEV_SEAT_KEYBOARD_LETTERS : 0) |
> >> + (digits ? EVDEV_SEAT_KEYBOARD_DIGITS : 0);
> >> +}
> >> +
> >> struct evdev_device *
> >> evdev_device_create(struct libinput_device *libinput_device,
> >> struct weston_seat *seat)
> >> @@ -497,7 +527,7 @@ evdev_device_create(struct libinput_device *libinput_device,
> >> if (libinput_device_has_capability(libinput_device,
> >> LIBINPUT_DEVICE_CAP_KEYBOARD)) {
> >> weston_seat_init_keyboard(seat, NULL);
> >> - device->seat_caps |= EVDEV_SEAT_KEYBOARD;
> >> + device->seat_caps |= keyboard_caps(libinput_device);
> >
> > If we don't want keyboards with neither numbers nor letters be
> > advertised as keyboards, can't we just simply not set
> > EVDEV_SEAT_KEYBOARD here? That way we'd need nothing of the extra API or
> > the extra calls.
>
> I assumed we would want those keyboards to still be advertised.
>
> It might be worthwhile to hide keyboards if the compositor binds all the
> keys they have (for example, the power button "keyboard" could end up
> like this at some point, if weston ever starts caring about the power
> button...), but I haven't looked into doing that at all yet.
I might have misunderstood your patches. I read it as you detect keyboard
capabilities and only treat real keyboards as keyboards. But what the
patch does is make the seat aware of the different keyboard capabilities,
so that the signal of updated seat capabilities gets emitted even when
the seat capabilities didn't change, but the "virtual keyboard"
capabilities changed. Sorry about the misunderstanding.
Given this, I think if we want to emit signals when keyboard
capabilities change, it should rather be a signal in weston_keyboard,
that is emitted when keyboard capability combination change. I don't
think there is any need for the new dirty/override API calls this way
either.
>
> > On a related note, doing something like this would mean that key events
> > would still be sent by libinput-device.c. As far as I can tell, this
> > means that if the only keyboard provided by libinput doesn't have digit
> > or letter keys, we'd never set seat->keyboard, but still call
> > notify_key, meaning a NULL pointer dereference in notify_key().
>
> Yeah, I don't think hiding/removing keyboards is a good idea, with the
> exception of hiding keyboards whose entire inventory has been grabbed by
> compositor keybinds. I'd thought about jamming them into a special
> administrative seat, but that's pretty disgusting.
>
> Hiding the keyboards exposes another interesting issue. A tablet with
> only a touch interface can currently have "keyboard focus" because it
> likely has a power button that shows up as a keyboard.
>
> Maybe that's something we'll have to address eventually anyway. It's
> already visible in multi seat since the first seat is the only one that
> gets the funny keyboards.
Yes, I see now that this is not something you were trying to address, I
suppose we can deal with this at a later date.
>
> > If, in libinput-device.c we'd ignore key events when we never set the
> > EVDEV_SEAT_KEYBOARD bit, this would mean that even if a client has a
> > wl_keyboard, it wouldn't ever get key events from non-keyboard keys.
>
> Well, wouldn't get events from keyboards that only have non-keyboard
> keys (or have an incomplete set of keyboard keys)?
>
> There might be keyboards with incomplete sets of keys like this thing:
> http://www.dx.com/cs/p/delux-t9-usb-2-0-wired-gaming-46-key-keyboard-w-3-mode-led-backlight-black-red-239391
>
> We're still going to want to pass any event that generates, even though
> it fails both the has_digits and has_letters tests in my patch...
> Of course, I wouldn't bet money on it accurately reporting what keycodes
> it's capable of passing anyway. :)
I wonder, should the test maybe be more forgiving so that has_* is true
if it has any? Then again for such a device, you can't really write
anything anyway, so what we really want might just be a device/keyboard
added/removed signal and a special "can someone write with any of these
keyboards?" test in the virtual keyboard integration part of the shell.
But exposing such information from non-libinput devices can be tricky,
so maybe what you already did is the best really.
>
> > I feels like the scope of wl_keyboard needs to be clarified before doing
> > this type of change, so that it is clear what type of events can be
> > relied on.
>
> I didn't think there would be interest in changing wl_keyboard at all.
>
> What I'm looking for here is a way to auto-hide a virtual keyboard based
> on whether there are real keyboards present that make it unnecessary.
> That could be done by putting the show/hide logic into the compositor
> and letting the compositor figure out what the attached keyboards can do.
Yea, regarding this, I only have some comments on the implementation
that I wrote above.
>
> We could still pass that decision on to the client/toolkit/whatever if
> we added wl proto for informing the client what keys are available, or
> even just simple digits/letters capabilities like this.
To send an available keys state might be tricky, since this might not be
available on certain backends (?), so maybe a more simple keyboard caps
interface makes most sense.
Jonas
>
> >
> > Jonas
> >
> >
> >> }
> >> if (libinput_device_has_capability(libinput_device,
> >> LIBINPUT_DEVICE_CAP_POINTER)) {
> >> diff --git a/src/libinput-device.h b/src/libinput-device.h
> >> index 0775743..70591c2 100644
> >> --- a/src/libinput-device.h
> >> +++ b/src/libinput-device.h
> >> @@ -34,7 +34,9 @@
> >> enum evdev_device_seat_capability {
> >> EVDEV_SEAT_POINTER = (1 << 0),
> >> EVDEV_SEAT_KEYBOARD = (1 << 1),
> >> - EVDEV_SEAT_TOUCH = (1 << 2)
> >> + EVDEV_SEAT_TOUCH = (1 << 2),
> >> + EVDEV_SEAT_KEYBOARD_DIGITS = (1 << 3),
> >> + EVDEV_SEAT_KEYBOARD_LETTERS = (1 << 4)
> >> };
> >>
> >> struct evdev_device {
> >> diff --git a/src/libinput-seat.c b/src/libinput-seat.c
> >> index f0fcd51..af695a6 100644
> >> --- a/src/libinput-seat.c
> >> +++ b/src/libinput-seat.c
> >> @@ -45,6 +45,30 @@ udev_seat_create(struct udev_input *input, const char *seat_name);
> >> static void
> >> udev_seat_destroy(struct udev_seat *seat);
> >>
> >> +static void
> >> +seat_rescan_keyboard_caps(struct udev_seat *udev_seat)
> >> +{
> >> + struct evdev_device *dev;
> >> + struct weston_seat *weston_seat = &udev_seat->base;
> >> + struct weston_keyboard *keyboard = weston_seat->keyboard_resource;
> >> + bool has_letters = false, has_digits = false;
> >> +
> >> + keyboard = weston_seat_get_keyboard(weston_seat);
> >> + if (!keyboard)
> >> + return;
> >> +
> >> + wl_list_for_each(dev, &udev_seat->devices_list, link) {
> >> + has_letters = has_letters || (dev->seat_caps & EVDEV_SEAT_KEYBOARD_LETTERS);
> >> + has_digits = has_digits || (dev->seat_caps & EVDEV_SEAT_KEYBOARD_DIGITS);
> >> + }
> >> +
> >> + if (has_letters != keyboard->has_letters || has_digits != keyboard->has_digits) {
> >> + keyboard->has_letters = has_letters;
> >> + keyboard->has_digits = has_digits;
> >> + keyboard->caps_dirty = true;
> >> + }
> >> +}
> >> +
> >> static struct udev_seat *
> >> get_udev_seat(struct udev_input *input, struct libinput_device *device)
> >> {
> >> @@ -81,6 +105,8 @@ device_added(struct udev_input *input, struct libinput_device *libinput_device)
> >> udev_seat = (struct udev_seat *) seat;
> >> wl_list_insert(udev_seat->devices_list.prev, &device->link);
> >>
> >> + seat_rescan_keyboard_caps(udev_seat);
> >> +
> >> pointer = weston_seat_get_pointer(seat);
> >> if (seat->output && pointer)
> >> weston_pointer_clamp(pointer,
> >> @@ -117,6 +143,7 @@ device_removed(struct udev_input *input, struct libinput_device *libinput_device
> >>
> >> device = libinput_device_get_user_data(libinput_device);
> >> evdev_device_destroy(device);
> >> + seat_rescan_keyboard_caps(udev_seat);
> >> weston_seat_send_dirty_caps(&udev_seat->base);
> >> }
> >>
> >> diff --git a/src/screen-share.c b/src/screen-share.c
> >> index 597ad9b..3e92d61 100644
> >> --- a/src/screen-share.c
> >> +++ b/src/screen-share.c
> >> @@ -214,6 +214,7 @@ ss_seat_handle_keymap(void *data, struct wl_keyboard *keyboard,
> >> else
> >> weston_seat_init_keyboard(&seat->base, keymap);
> >>
> >> + weston_seat_override_keyboard_caps(&seat->base, true, true);
> >> weston_seat_send_dirty_caps(&seat->base);
> >>
> >> xkb_keymap_unref(keymap);
> >> --
> >> 2.1.4
> >>
> >> _______________________________________________
> >> 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