Created attachment 360269 [details] [review]
wacom: Use GTK+ skeleton

The wacom module uses GDK resources indirectly through GsdDeviceMapper,
which turn out NULL when this object tries to attach itself to a
GdkScreen.

Created attachment 360270 [details] [review]
wacom: Add both tablet and touchscreen devices on initial iteration

The GsdDeviceManager::device-added callback would observe both
display-attached tablets and touchscreens, but only the former were
added in the initial loop.

This made touchscreens not properly mapped to outputs, as touchscreens
are usually built into the device.

Review of attachment 360269 [details] [review]:

sure

Review of attachment 360270 [details] [review]:

makes sense

Thanks! Pushed to master. I see gnome-3-26 is not branched yet, and this
does not apply to gnome-3-24, so we're good here.

Attachment 360269 [details] pushed as 0fb1b96 - wacom: Use GTK+ skeleton
Attachment 360270 [details] pushed as 08424fb - wacom: Add both tablet and touchscreen devices on initial iteration

Carlos,

Thank you for looking into this, unfortunately the fix is not complete. Starting with 4.14 the kernel has support for the capacitive home-button found on some
goodix touchscreens (on some x86 tablets the windows logo on the front is a capacitive button), so now the touchscreen is both a touchscreen and a keyboard.

xf86-input-libinput deals with this by splitting the /dev/input/event# node into 2 Xi2 devices, but plugins/common/gsd-device-manager-x11.c checks for duplicate devices based on the device path returned by xdevice_get_device_node which is the same /dev/input/event# node for both the keyboard and touchscreen Xi2 devices which xf86-input-libinput creates for the touchscreen.

So now depending on which get enumerated first (and my testing has shown that the enumeration order is random) either only the Touchscreen Xi2 dev gets seen by plugins/common/gsd-device-manager-x11.c; or only the keyboard Xi2 dev. The former is fine as g-s-d does not need to be aware of the keyboard Xi2 dev, if however the keybaord Xi2 dev gets enumerated g-s-d never sees the touchscreen and the mapping of the touchscreen to a monitor does not happen.

I believe the easiest way to fix this is for g-s-d to ignore the keyboard Xi2 devices for /dev/input/event# nodes which are both a touch device and a keyboard, note xf86-input-libinput calls these devices sub-devices. I see 2 ways for g-s-d to ignore these sub-devices:

1) Have xf86-input-libinput not set the XI_PROP_DEVICE_NODE property on sub-devices so that xdevice_get_device_node returns NULL as it already does for the 
"Virtual core XTEST pointer/keyboard"

2) Have xf86-input-libinput set a new XI_PROP_LIBINPUT_SUBDEV property on sub-devs which g-s-d can then test to ignore these.

1. Has the advantage that it requires no changes in g-s-d, but I'm not sure if that may have any bad side-effects.

I will send Peter Hutterer a mail about this, asking him to reply here in bugzilla.

Regards,

Hans

(In reply to Hans de Goede from comment #6)
> Carlos,
> 
> Thank you for looking into this, unfortunately the fix is not complete.
> Starting with 4.14 the kernel has support for the capacitive home-button
> found on some
> goodix touchscreens (on some x86 tablets the windows logo on the front is a
> capacitive button), so now the touchscreen is both a touchscreen and a
> keyboard.

Ouch, I see... I have no tablets with such button, so will leave testing to you :).

> 
> xf86-input-libinput deals with this by splitting the /dev/input/event# node
> into 2 Xi2 devices, but plugins/common/gsd-device-manager-x11.c checks for
> duplicate devices based on the device path returned by
> xdevice_get_device_node which is the same /dev/input/event# node for both
> the keyboard and touchscreen Xi2 devices which xf86-input-libinput creates
> for the touchscreen.
> 
> So now depending on which get enumerated first (and my testing has shown
> that the enumeration order is random) either only the Touchscreen Xi2 dev
> gets seen by plugins/common/gsd-device-manager-x11.c; or only the keyboard
> Xi2 dev. The former is fine as g-s-d does not need to be aware of the
> keyboard Xi2 dev, if however the keybaord Xi2 dev gets enumerated g-s-d
> never sees the touchscreen and the mapping of the touchscreen to a monitor
> does not happen.
> 
> I believe the easiest way to fix this is for g-s-d to ignore the keyboard
> Xi2 devices for /dev/input/event# nodes which are both a touch device and a
> keyboard, note xf86-input-libinput calls these devices sub-devices. I see 2
> ways for g-s-d to ignore these sub-devices:

IMHO there's no need to complicate this much from the g-s-d perspective. GsdDeviceManager is only used together with GsdDeviceMapper, so just devices with ABS axes are essential here. I think it's entirely safe to ignore all keyboards here.

Also, a heads up that this output mapping policy will be folded into mutter. There we have a less ad-hoc input device abstraction, so whatever we come up with here might not be needed after all.

Regardless of Peter considering this a xf86-input-libinput inconsistency or not, I'm attaching an untested patch to ignore keyboards on g-s-d.

Created attachment 360314 [details] [review]
common: Ignore (pure) keyboard devices on GsdDeviceManager

They are not needed for GsdDeviceMapper purposes, and can cause misbehavior
if the keyboard device "shares" the input node with a device we do need
mapping.

Fixes spotty touchscreen detection with those touchscreens that have a
capacitive menu/windows button.

Review of attachment 360314 [details] [review]:

makes sense, push if testing passes

bit late with the reply, sorry.

(In reply to Hans de Goede from comment #6)
> 1) Have xf86-input-libinput not set the XI_PROP_DEVICE_NODE property on
> sub-devices so that xdevice_get_device_node returns NULL as it already does
> for the 
> "Virtual core XTEST pointer/keyboard"
> 
> 2) Have xf86-input-libinput set a new XI_PROP_LIBINPUT_SUBDEV property on
> sub-devs which g-s-d can then test to ignore these.
 
sorry, both of these aren't really acceptable for the libinput driver which is doing the right thing here. The concept of multiple X devices per event node has been around for a while (wacom driver) so this is simply a bug in g-s-d.

Peter, thank you for the feedback,

Carlos, sorry for the slow response, testing your patch now. First thing which stands out:

gsd-device-manager-udev.c: In function ‘create_device’:
gsd-device-manager-udev.c:95:3: warning: ‘return’ with no value, in function returning non-void
   return;
   ^~~~~~
gsd-device-manager-udev.c:82:1: note: declared here
 create_device (GUdevDevice *udev_device)
 ^~~~~~~~~~~~~

Fixed this with this diff on top:

--- a/plugins/common/gsd-device-manager-udev.c
+++ b/plugins/common/gsd-device-manager-udev.c
@@ -92,7 +92,7 @@ create_device (GUdevDevice *udev_device)
         */
        device_type = udev_device_get_device_type (udev_device);
        if (device_type == GSD_DEVICE_TYPE_KEYBOARD)
-               return;
+               return NULL;
 
        parent = g_udev_device_get_parent (udev_device);
        g_assert (parent != NULL);
@@ -137,6 +137,9 @@ add_device (GsdUdevDeviceManager *manager,
                return;
 
        device = create_device (udev_device);
+       if (!device)
+               return;
+
        g_hash_table_insert (manager->devices, g_object_ref (udev_device), device);
        g_signal_emit_by_name (manager, "device-added", device);
 }

Carlos' patch with my fixup works for a GNOME3 on Xorg session, but gsd-wacom seems to be using gsd-device-manager-x11.c under wayland and seeing the xwayland devices ?

Debugging this further.

Ok, so my remark about using gsd-device-manager-x11.c probably is a red herring. The problem seems to be that plugins/common/daemon-skeleton-gtk.h should_run returns false for gsd-wacom under gdm an I was using gdm to test things under Wayland (and a normal session for testing the Xorg paths).

Created attachment 360668 [details] [review]
common: Ignore (pure) keyboard devices on GsdDeviceManager

New, fixed version of Carlos' patch.

Created attachment 360669 [details] [review]
common: Fix touchscreen vs display orientation mismatch under gdm

This fixes gsd-wacom not being active under gdm.

Ok, with these 2 patches everything works as it should.

Review of attachment 360314 [details] [review]:

oops

Review of attachment 360668 [details] [review]:

thanks, this seems fine

Review of attachment 360669 [details] [review]:

ok, makes sense

Attachment 360668 [details] pushed as 36ed918 - common: Ignore (pure) keyboard devices on GsdDeviceManager
Attachment 360669 [details] pushed as cf5b5fb - common: Fix touchscreen vs display orientation mismatch under gdm