Created attachment 281387 [details] [review]
wayland: set the interface vfuncs when declaring the keyboard interface

Otherwise the NULL vtable would be accessed when trying to release the keyboard.

Created attachment 281388 [details] [review]
wayland: set the interface vfuncs when declaring the touch interface

Otherwise the NULL vtable would be accessed when trying to release the touch
device.

Created attachment 281389 [details] [review]
wayland: Unset keyboard/pointer focus when releasing the data for these devices

Otherwise the focus_surface_listener list element becomes stale, and then
mangled if the devices' data is initialized again, and the memory memset().

Created attachment 281390 [details] [review]
wayland: Ensure the cursor tracker is set after MetaWaylandPointer reinitializations

It is set directly by MetaCursorTracker code during startup, but the pointer
was unset and left to NULL if the MetaWaylandPointer is released/initialized.

Created attachment 281391 [details] [review]
wayland: Notify modifiers after keymap changes

Anytime the keymap is changed, either directly, or indirectly through the
keyboard capability being released/initialized, there should be a
notification of the modifiers being changed too.

Created attachment 281392 [details] [review]
wayland: Toggle capabilities on VT switch

In this situation, all input devices are suspended, and the compositor must
reset the seat capabilities while this happens.

Although this is correct for all devices, this specifically fixes the keyboard
possibly keeping a wrong modifier state after the session is unpaused.

Review of attachment 281387 [details] [review]:

Yeesh. Not sure how this slipped through.

Review of attachment 281388 [details] [review]:

OK.

Review of attachment 281389 [details] [review]:

OK.

Review of attachment 281390 [details] [review]:

I'm not too happy with this fix... I pushed an alternate one.

https://git.gnome.org/browse/mutter/commit/?id=e49bbe2ed8aed844f15aabe289323036e2459245

Review of attachment 281391 [details] [review]:

::: src/wayland/meta-wayland-keyboard.c
@@ +61,3 @@
 #include "meta-wayland-private.h"
 
+void meta_wayland_keyboard_update_xkb_state (MetaWaylandKeyboard *keyboard);

This needs to be static.

Review of attachment 281392 [details] [review]:

::: src/wayland/meta-wayland-seat.c
@@ +216,3 @@
+
+void
+meta_wayland_seat_set_capabilities_enabled (MetaWaylandSeat *seat,

I'm not too happy with the name. I think it should release_devices / reclaim_devices, like Clutter.

I'm half of the opinion that we should remove the Clutter devices on VT switch, and look at the ClutterDeviceManager for these sorts of changes, but at the same time, mutter internals can't really deal with no VCP/VCK.

@@ +224,3 @@
+  wl_resource_for_each (resource, &seat->base_resource_list)
+    {
+      flags = (!enabled) ? 0 :

flags = enabled ? flags : 0;

@@ +227,3 @@
+        WL_SEAT_CAPABILITY_POINTER |
+        WL_SEAT_CAPABILITY_KEYBOARD |
+        WL_SEAT_CAPABILITY_TOUCH;

Right now we send CAPABILITY_TOUCH even if there is no touch device. This is an existing bug, I'm aware, but I think we should probably start to conditionalize this.

@@ +228,3 @@
+        WL_SEAT_CAPABILITY_KEYBOARD |
+        WL_SEAT_CAPABILITY_TOUCH;
+      wl_seat_send_capabilities (resource, flags);

I'd like to pull this code into a common place, so we can use it for new resources and for enabling/disabling the seat. Like, right now we'll do the wrong thing if a client binds to the seat while we're VT switched away. I think we should have a seat_enabled flag in the MetaWaylandSeat, and use that everywhere we send caps / manage devices.

@@ +231,3 @@
+    }
+
+  if (!enabled)

Don't do:

    if (!foo)
        ...
    else
        ...

@@ +242,3 @@
+      meta_wayland_keyboard_init (&seat->keyboard, seat->display);
+      meta_wayland_touch_init (&seat->touch, seat->display);
+      meta_display_sync_wayland_input_focus (meta_get_display ());

seat->display didn't work for some reason???

Review of attachment 281391 [details] [review]:

(Intentionally not reviewing this one. I know Rui has been working on the xkb code, so I'd like him to OK it)

Attachment 281387 [details] pushed as dfe1c10 - wayland: set the interface vfuncs when declaring the keyboard interface
Attachment 281388 [details] pushed as a02b844 - wayland: set the interface vfuncs when declaring the touch interface
Attachment 281389 [details] pushed as 1677a06 - wayland: Unset keyboard/pointer focus when releasing the data for these devices

(In reply to comment #12)
> Review of attachment 281392 [details] [review]:
> 
> ::: src/wayland/meta-wayland-seat.c
> @@ +216,3 @@
> +
> +void
> +meta_wayland_seat_set_capabilities_enabled (MetaWaylandSeat *seat,
> 
> I'm not too happy with the name. I think it should release_devices /
> reclaim_devices, like Clutter.
> 
> I'm half of the opinion that we should remove the Clutter devices on VT switch,
> and look at the ClutterDeviceManager for these sorts of changes, but at the
> same time, mutter internals can't really deal with no VCP/VCK.
> 
> @@ +224,3 @@
> +  wl_resource_for_each (resource, &seat->base_resource_list)
> +    {
> +      flags = (!enabled) ? 0 :
> 
> flags = enabled ? flags : 0;
> 
> @@ +227,3 @@
> +        WL_SEAT_CAPABILITY_POINTER |
> +        WL_SEAT_CAPABILITY_KEYBOARD |
> +        WL_SEAT_CAPABILITY_TOUCH;
> 
> Right now we send CAPABILITY_TOUCH even if there is no touch device. This is an
> existing bug, I'm aware, but I think we should probably start to conditionalize
> this.

Yeah, I also reached to that conclusion. that needs some more work though, this is the least necessary to have vt switch work, quite handy if you run gnome-shell native besides the system Xorg session.

> 
> @@ +228,3 @@
> +        WL_SEAT_CAPABILITY_KEYBOARD |
> +        WL_SEAT_CAPABILITY_TOUCH;
> +      wl_seat_send_capabilities (resource, flags);
> 
> I'd like to pull this code into a common place, so we can use it for new
> resources and for enabling/disabling the seat. Like, right now we'll do the
> wrong thing if a client binds to the seat while we're VT switched away. I think
> we should have a seat_enabled flag in the MetaWaylandSeat, and use that
> everywhere we send caps / manage devices.

maybe we should maintain a capabilities flags field on the seat struct, and have a set_capabilities() that initializes/disposes per device. As for merging this with set_capabilities_enabled(), state should be preserved somewhere, and a push/pop approach doesn't fit with the "devices being (un)plugged" scenario, so maybe these should be kept separate.

> 
> @@ +231,3 @@
> +    }
> +
> +  if (!enabled)
> 
> Don't do:
> 
>     if (!foo)
>         ...
>     else
>         ...
> 
> @@ +242,3 @@
> +      meta_wayland_keyboard_init (&seat->keyboard, seat->display);
> +      meta_wayland_touch_init (&seat->touch, seat->display);
> +      meta_display_sync_wayland_input_focus (meta_get_display ());
> 
> seat->display didn't work for some reason???

wl_display vs MetaDisplay, should be probably named wl_display on MetaWaylandSeat.

Review of attachment 281391 [details] [review]:

Looks fine otherwise. I'll rebase my patches on top

::: src/wayland/meta-wayland-keyboard.c
@@ +265,3 @@
+{
+  MetaWaylandXkbInfo *xkb_info = &keyboard->xkb_info;
+  guint latched, locked, group;

These should be xkb_mod_mask_t

@@ +267,3 @@
+  guint latched, locked, group;
+
+  /* Preserv latched/locked modifiers state */

typo: Preserve

Comment on attachment 281391 [details] [review]
wayland: Notify modifiers after keymap changes

Attachment 281391 [details] pushed as 32565e0 - wayland: Notify modifiers after keymap changes

Created attachment 281476 [details] [review]
wayland: Add MetaWaylandSeat methods to manipulate capabilities

meta_wayland_seat_set_capabilities update the capabilities mask, while
meta_wayland_seat_release/reclaim_devices() toggle the capabilities mask
off/on, so clients receive the effective combination of these.

Created attachment 281477 [details] [review]
wayland: Toggle capabilities on VT switch

In this situation, all input devices are suspended, and the compositor must
reset the seat capabilities while this happens.

Although this is correct for all devices, this specifically fixes the keyboard
possibly keeping a wrong modifier state after the session is unpaused.

Review of attachment 281476 [details] [review]:

I don't like this approach at all. I'd rather calculate the capabilities based on what we have.

    has_pointer = (pointer_device != NULL) && !suspended;

    if (has_pointer)
      {
        meta_wayland_pointer_init (&seat->pointer);
        capabilities |= WL_SEAT_CAPABILITY_POINTER;
      }
    else
      meta_wayland_pointer_release (&seat->pointer);

etc.

::: src/wayland/meta-wayland-seat.h
@@ +42,3 @@
   MetaWaylandDataDevice data_device;
+
+  guint capabilities : 7;

This is meant to be : 3; I assume.

@@ +56,3 @@
+
+void meta_wayland_seat_set_capabilities_enabled (MetaWaylandSeat *seat,
+                                                 gboolean         enabled);

From a previous version of the patch?

Review of attachment 281477 [details] [review]:

Looks good to me.

(In reply to comment #20)
> Review of attachment 281476 [details] [review]:
> 
> I don't like this approach at all. I'd rather calculate the capabilities based
> on what we have.
> 
>     has_pointer = (pointer_device != NULL) && !suspended;
> 
>     if (has_pointer)
>       {
>         meta_wayland_pointer_init (&seat->pointer);
>         capabilities |= WL_SEAT_CAPABILITY_POINTER;
>       }
>     else
>       meta_wayland_pointer_release (&seat->pointer);
> 
> etc.

That's not how things work... Clutter creates fake VCP/VCK devices on evdev, those never go away, and there is no "Virtual Core Touch", touch events go instead through the VCP. 

OTOH, "slave" devices on evdev wrap very closely libinput_device, so you'll get one slave per /dev/input/event* special file. Those do go away and return back on libinput_suspend/resume(), but the capabilities should be the logical OR of all those devices, we've got no single pointer we can conveniently check.

> 
> ::: src/wayland/meta-wayland-seat.h
> @@ +42,3 @@
>    MetaWaylandDataDevice data_device;
> +
> +  guint capabilities : 7;
> 
> This is meant to be : 3; I assume.

I went for 3 initially, but tablet support will come in at some point, and it might generate some headaches if field size is not noticed/updated, so I went for using a full byte for the current flags to avoid that in the foreseeable future. There should be 3 more bytes of padding for any future flags, should still be enough.

> 
> @@ +56,3 @@
> +
> +void meta_wayland_seat_set_capabilities_enabled (MetaWaylandSeat *seat,
> +                                                 gboolean         enabled);
> 
> From a previous version of the patch?

Oops, yes...

Yeah, and mutter doesn't support pointerless or keyboardless devices yet, so I'm fine with having POINTER / KEYBOARD always exist.

I don't know how touchscreens work. If it creates a fake virtual master touch device for get_core_device (CLUTTER_TOUCH_DEVICE);, is there a has_any_slave(); API or something we can use to determine if the device is backed by anything real?

And actually, if we do have that, we don't need a separate "enabled" boolean, because the revoke/reclaim should drop all slaves, no?

I was more suggesting to remove set_capabilities / unset_capabilities as an API, and instead just rely on Clutter's state and our suspended state. I see capabilities as an output of what our internal state is, not something that controls our internal state.

(In reply to comment #22)
> I went for 3 initially, but tablet support will come in at some point, and it
> might generate some headaches if field size is not noticed/updated, so I went
> for using a full byte for the current flags to avoid that in the foreseeable
> future. There should be 3 more bytes of padding for any future flags, should
> still be enough.

Just use a full guint.

(In reply to comment #23)
> Yeah, and mutter doesn't support pointerless or keyboardless devices yet, so
> I'm fine with having POINTER / KEYBOARD always exist.
> 
> I don't know how touchscreens work. If it creates a fake virtual master touch
> device for get_core_device (CLUTTER_TOUCH_DEVICE);, is there a has_any_slave();
> API or something we can use to determine if the device is backed by anything
> real?

No... again there's no "master touch device", all pointer and touch events do:
clutter_event_set_device (event, seat->core_pointer);

As for API helping here, the best we've got is clutter_input_device_get_slave_devices(), we can iterate over all slave devices and figure the capability flags that apply at that given point, and then match against the stored ones, but there's no call or object that gives us that information as tidily.

> 
> And actually, if we do have that, we don't need a separate "enabled" boolean,
> because the revoke/reclaim should drop all slaves, no?

Yeah right, when keeping that I thought about Xorg behavior where slave devices are still added/removed out of VT, they're just kept disabled, but libinput certainly doesn't behave like that.

> 
> I was more suggesting to remove set_capabilities / unset_capabilities as an
> API, and instead just rely on Clutter's state and our suspended state. I see
> capabilities as an output of what our internal state is, not something that
> controls our internal state.

Nod, the code that figures this out from the ClutterDeviceManager can be kept within meta-wayland-seat.c indeed.

> 
> (In reply to comment #22)
> > I went for 3 initially, but tablet support will come in at some point, and it
> > might generate some headaches if field size is not noticed/updated, so I went
> > for using a full byte for the current flags to avoid that in the foreseeable
> > future. There should be 3 more bytes of padding for any future flags, should
> > still be enough.
> 
> Just use a full guint.

sure, I hope we never need every bit there, for my sanity ;)

Created attachment 281497 [details] [review]
seat: Listen for ClutterDeviceManager signals in order to update capabilities

The capability flags are determined from the device types of the slave devices
that are currently attached. This also happens whenever a device is added or
removed, so the capabilities are kept up to date, and clients know about these.

On VT switch, all slave devices are temporarily removed, so the cascade of
signals will make the seat end up with capabililities=0 while input is suspended.

Review of attachment 281497 [details] [review]:

::: src/wayland/meta-wayland-seat.c
@@ +99,3 @@
+lookup_device_capabilities (ClutterDeviceManager *device_manager)
+{
+  const GSList *devices, *dev;

const GSList *devices, *l;

@@ +106,3 @@
+  for (dev = devices; dev; dev = dev->next)
+    {
+      ClutterInputDeviceType device_type;

ClutterInputDevice *dev = l->data;

@@ +108,3 @@
+      ClutterInputDeviceType device_type;
+
+      if (clutter_input_device_get_device_mode (dev->data) == CLUTTER_INPUT_MODE_MASTER)

A comment here about why we're looking for slave devices (Clutter's fake VCP/VCK) would be *extremely* helpful to future people trying to figure out what's going on.

@@ +125,3 @@
+          break;
+        default:
+          g_warning ("Ignoring device '%s' with unhandled type %d",

Can this be a g_debug instead? It's certainly not important enough to be a warning, especially when it's a valid scenario, right?

@@ +151,3 @@
+    meta_wayland_pointer_init (&seat->pointer, seat->display);
+  else if (CAPABILITY_DISABLED (prev_flags, flags, WL_SEAT_CAPABILITY_POINTER))
+    meta_wayland_pointer_release (&seat->pointer);

I'm really not a fan of the way this function looks. I really don't like calculating the capabilities of this and then making decisions based on that. That said, I'm not going to block on it.

@@ +162,3 @@
+      /* Post-initialization, ensure the input focus is in sync */
+      if (display)
+        meta_display_sync_wayland_input_focus (display);

I mean, I know we do this all over the place, but I'm curious. Why? Why do we need to sync Wayland input focus here?

@@ +205,2 @@
   wl_list_init (&seat->base_resource_list);
+  seat->display = display;

Can you rename this to wl_display, as we said?

Created attachment 281499 [details] [review]
seat: Listen for ClutterDeviceManager signals in order to update capabilities

The capability flags are determined from the device types of the slave devices
that are currently attached. This also happens whenever a device is added or
removed, so the capabilities are kept up to date, and clients know about these.

On VT switch, all slave devices are temporarily removed, so the cascade of
signals will make the seat end up with capabililities=0 while input is suspended.

(In reply to comment #26)
> Review of attachment 281497 [details] [review]:
> +      if (clutter_input_device_get_device_mode (dev->data) ==
> CLUTTER_INPUT_MODE_MASTER)
> 
> A comment here about why we're looking for slave devices (Clutter's fake
> VCP/VCK) would be *extremely* helpful to future people trying to figure out
> what's going on.

Added a small comment

> 
> @@ +125,3 @@
> +          break;
> +        default:
> +          g_warning ("Ignoring device '%s' with unhandled type %d",
> 
> Can this be a g_debug instead? It's certainly not important enough to be a
> warning, especially when it's a valid scenario, right?

Changed this to a g_debug, but eventually/ideally mutter should handle all devices handled by libinput/clutter as support appears there.

> 
> @@ +151,3 @@
> +    meta_wayland_pointer_init (&seat->pointer, seat->display);
> +  else if (CAPABILITY_DISABLED (prev_flags, flags,
> WL_SEAT_CAPABILITY_POINTER))
> +    meta_wayland_pointer_release (&seat->pointer);
> 
> I'm really not a fan of the way this function looks. I really don't like
> calculating the capabilities of this and then making decisions based on that.
> That said, I'm not going to block on it.

As I see it, the state has to be retained somewhere... maybe if libinput were used directly this could be tidier, just because the semantics match.

> 
> @@ +162,3 @@
> +      /* Post-initialization, ensure the input focus is in sync */
> +      if (display)
> +        meta_display_sync_wayland_input_focus (display);
> 
> I mean, I know we do this all over the place, but I'm curious. Why? Why do we
> need to sync Wayland input focus here?

meta_display_sync_wayland_input_focus() ultimately calls meta_wayland_keyboard_set_focus(). The focus surface information isn't kept across MetaWaylandKeyboard release/init, so this is needed
to ensure the just initialized keyboard points to the current focus window. Otherwise keyboard doesn't work after vt switch/plug until you focus into some other window.

> 
> @@ +205,2 @@
>    wl_list_init (&seat->base_resource_list);
> +  seat->display = display;
> 
> Can you rename this to wl_display, as we said?

Sure, done

(In reply to comment #28)
> Changed this to a g_debug, but eventually/ideally mutter should handle all
> devices handled by libinput/clutter as support appears there.

Can we guarantee that none of these will happen?

https://git.gnome.org/browse/clutter/tree/clutter/clutter-enums.h#n756

There's ongoing support for tablets in libinput / Wayland, and I don't think plugging in a tablet should cause a user-visible warning. If you feel strongly, feel free to make it a warning.

> meta_display_sync_wayland_input_focus() ultimately calls
> meta_wayland_keyboard_set_focus(). The focus surface information isn't kept
> across MetaWaylandKeyboard release/init, so this is needed
> to ensure the just initialized keyboard points to the current focus window.
> Otherwise keyboard doesn't work after vt switch/plug until you focus into some
> other window.

Ah, right. Although we also do force a repick, which is a bit bizarre, but OK. We also need to get grab_op / focus_window out of MetaDisplay, but that's a cleanup for me.

Review of attachment 281499 [details] [review]:

OK.

(In reply to comment #29)
> (In reply to comment #28)
> > Changed this to a g_debug, but eventually/ideally mutter should handle all
> > devices handled by libinput/clutter as support appears there.
> 
> Can we guarantee that none of these will happen?
> 
> https://git.gnome.org/browse/clutter/tree/clutter/clutter-enums.h#n756

I've added to the switch the device types used so far in evdev:
https://git.gnome.org/browse/clutter/tree/clutter/evdev/clutter-input-device-evdev.c?h=clutter-1.20#n189

> 
> There's ongoing support for tablets in libinput / Wayland, and I don't think
> plugging in a tablet should cause a user-visible warning. If you feel strongly,
> feel free to make it a warning.

Nah, just the g_debug should be fine, I'll be definitely keeping an eye when that happens in libinput, it'll be probably me doing the clutter/mutter/gtk+ support when that happens.

> 
> > meta_display_sync_wayland_input_focus() ultimately calls
> > meta_wayland_keyboard_set_focus(). The focus surface information isn't kept
> > across MetaWaylandKeyboard release/init, so this is needed
> > to ensure the just initialized keyboard points to the current focus window.
> > Otherwise keyboard doesn't work after vt switch/plug until you focus into some
> > other window.
> 
> Ah, right. Although we also do force a repick, which is a bit bizarre, but OK.
> We also need to get grab_op / focus_window out of MetaDisplay, but that's a
> cleanup for me.

Attachment 281499 [details] pushed as ac448bd - seat: Listen for ClutterDeviceManager signals in order to update capabilities