[weston,4/4] input: Detect keyboard capabilities

Submitted by Derek Foreman on May 5, 2015, 8:01 p.m.

Details

Message ID 1430856114-20765-5-git-send-email-derekf@osg.samsung.com
State New
Delegated to: Jonas Ådahl
Headers show

Not browsing as part of any series.

Commit Message

Derek Foreman May 5, 2015, 8:01 p.m.
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@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(-)

Patch hide | download patch | download mbox

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 (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);

Comments

It sure seems like it would be better to just provide the list of 
available keys to the client programs, and let them make up their own 
categories of device types.

I'm not sure if requiring every Latin letter is a good test for a 
keyboard either. Aren't there some foreign layouts that omit some?

On 05/05/2015 01:01 PM, Derek Foreman wrote:

> +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);
> +}
On 05/05/15 03:59 PM, Bill Spitzak wrote:
> It sure seems like it would be better to just provide the list of
> available keys to the client programs, and let them make up their own
> categories of device types.

I think desktop-shell is still going to want this information,
regardless of whether we implement new protocol to feed it on to
clients.  So it may be the case that clients want it too, but I don't
think it would be better to only provide it to clients.

a wl_keyboard is actually the union of all keyboard devices in a seat
right now, so I guess any protocol that give this information to clients
has to be capable of providing a list of new keys (hotplug) and revoking
keys already available (hot unplug).

If there's interest in such protocol I can put something together, but
I'd like to see some wider interest first, as it'd be a lot of wasted
time if the code won't land.

My use case for this is another patch series I'm working on that lets
the shell auto-hide the virtual keyboard.  I haven't seen any client
side use case for knowing about these capabilities yet.

> I'm not sure if requiring every Latin letter is a good test for a
> keyboard either. Aren't there some foreign layouts that omit some?

Entirely possible, if anyone can suggest a better test, I'd do that
instead.  As long as we don't recognize some gaming left hand keyboard
that just has WASD and a few other keys on it as something that's fully
capable of typing text, I'd be happy...

> On 05/05/2015 01:01 PM, Derek Foreman wrote:
> 
>> +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);
>> +}
>
On 05/06/2015 09:24 AM, Derek Foreman wrote:
> On 05/05/15 03:59 PM, Bill Spitzak wrote:
>> It sure seems like it would be better to just provide the list of
>> available keys to the client programs, and let them make up their own
>> categories of device types.
>
> I think desktop-shell is still going to want this information,
> regardless of whether we implement new protocol to feed it on to
> clients.  So it may be the case that clients want it too, but I don't
> think it would be better to only provide it to clients.

I meant clients of libinput. In this case the shell is a client of 
libinput. What I am suggesting is that the desktop shell has the list of 
keys to test, etc, rather than libinput.

It is true that a plan needs to be made so that exact duplicates of 
everything in the libinput api can be delivered to Wayland clients so 
they are not deprived of any ability to use an input device. The obvious 
method is to make a Wayland version for every libinput event and 
request. An alternative is for Wayland to provide the fd and the clients 
talk to libinput directly. Both of these have problems.
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@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.

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

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.

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.


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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
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@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.

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

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

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.

> 
> 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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
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@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@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>