Created attachment 264333 [details] [review]
evdev: Port evdev input backend to libinput

Instead of having its own evdev input device processing implementation,
make clutter's evdev backend use libinput to do input device processing
for it.

Two GObject parameters of ClutterInputDeviceEvdev (sysfs-path and
device-path) are deprecated as they are not used any more for
construction.

Before ClutterDeviceManagerEvdev had one virtual core keyboard and one
virtual core pointer device. These are now instead separated into seats,
which all have one virtual core keybard and pointer device respectively.

The 'global' core keyboard and pointer device are the core keyboard and
pointer device of the first seat that is created.

A ClutterInputDeviceEvdev can, as before, both represent a real physical
device or a virtual device, but is now instead created either via
clutter_input_device_evdev_new() for real devices, and
clutter_input_device_new_virtual() for virtual devices.

XKB state and button state is moved to the seat structure and is thus
separated per seat. Seats are not a concept exposed outside of clutter's
evdev backend.

ClutterScroll events are assumed to have their delta in the same
dimension as Wayland pointer axis events, meaning that 1.0, equals to
1.0 pixels motion event as if it would be a scroll movement on a touch
pad. This is to simplify using these events to implement a Wayland
compositor.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Review of attachment 264333 [details] [review]:

Not a full review:

::: clutter/evdev/clutter-device-manager-evdev.c
@@ +1139,3 @@
+                                              manager_evdev,
+                                              udev,
+                                              "seat0");

should udev_unref() here or so

::: clutter/evdev/clutter-input-device-evdev.c
@@ +50,3 @@
+                            CLUTTER_TYPE_INPUT_DEVICE);
+
+static gint global_device_id_next = 0;

I'd keep this starting at 2. There are assumptions in mutter, at least, that the core master pointer and keyboard are 2 and 3 and all other devices have ids > 3.

Shouldn't be difficult to fix those assumptions but I don't see a big enough reason to change this either...

@@ +239,3 @@
+                         "device-manager", manager,
+                         "device-type", type,
+                         "device-mode", CLUTTER_INPUT_MODE_SLAVE,

should be _MASTER

@@ +265,1 @@
+  libinput_device_led_update (priv->libinput_device, leds);

priv->libinput_device is NULL on master devices so this should likely have an if on that

::: configure.ac
@@ +148,3 @@
 m4_define([gdk_req_version],            [3.3.18])
+m4_define([libinput_req_version],       [0.0.90])
+m4_define([libudev_req_version],        [136])

you should also update README.in with these dependencies

Review of attachment 264333 [details] [review]:

This is generally looking good to me otherwise.

Just a couple more bugs I hit below:

::: clutter/evdev/clutter-device-manager-evdev.c
@@ +581,1 @@
 evdev_add_device (ClutterDeviceManagerEvdev *manager_evdev,

The return value isn't used so it could remain static void

@@ +863,3 @@
+        (struct libinput_event_added_seat *) event);
+
+      clutter_seat_evdev_new (manager_evdev, libinput_seat);

The seat never gets added to the ->seats list

@@ +871,3 @@
+      seat = libinput_seat_get_user_data (libinput_seat);
+
+      clutter_seat_evdev_free (seat);

likewise, it should be removed from the ->seats list here.

Created attachment 268004 [details] [review]
evdev: Port evdev input backend to libinput

Instead of having its own evdev input device processing implementation,
make clutter's evdev backend use libinput to do input device processing
for it.

Two GObject parameters of ClutterInputDeviceEvdev (sysfs-path and
device-path) are deprecated as they are not used any more for
construction.

Before ClutterDeviceManagerEvdev had one virtual core keyboard and one
virtual core pointer device. These are now instead separated into seats,
which all have one virtual core keybard and pointer device respectively.

The 'global' core keyboard and pointer device are the core keyboard and
pointer device of the first seat that is created.

A ClutterInputDeviceEvdev can, as before, both represent a real physical
device or a virtual device, but is now instead created either via
clutter_input_device_evdev_new() for real devices, and
clutter_input_device_new_virtual() for virtual devices.

XKB state and button state is moved to the seat structure and is thus
separated per seat. Seats are not a concept exposed outside of clutter's
evdev backend.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Review of attachment 268004 [details] [review]:

Preliminary review. I tried it and it worked as advertised.

Letting Rui or Emannuele do a more detailed review later.

::: clutter/evdev/clutter-device-manager-evdev.c
@@ +684,3 @@
+
+static ClutterSeatEvdev *
+get_seat_by_name (ClutterDeviceManagerEvdev *manager_evdev,

Is there a rationale for this being named "get_seat_by_name" ? Why not "get_clutter_seat_for_libinput_seat" ?

@@ +851,3 @@
   priv = manager_evdev->priv;
 
+  for (l = priv->seats; l; l = l->next)

Why was this changed? The old code with priv->devices should still work, no?

::: clutter/evdev/clutter-input-device-evdev.c
@@ +52,3 @@
+/*
+ * Having this set to 2 means that the core pointer and keyboard of the first
+ * created seat will get the ids 2 and 3.

You should probably explain that it's this way because X11 assigns the VCK/VCP the IDs "2" and "3".

Review of attachment 268004 [details] [review]:

thanks for the patch.

generally, it looks good. there are a bunch of coding style issues.

I'll let Rui comment on the patch as well for the logic.

::: README.in
@@ +42,3 @@
   • xkbcommon
+  • libudev ≥ @LIBUDEV_REQ_VERSION@
+  • libinput ≥ @LIBINPUT_REQ_VERSION@

there should be an item in the Release Notes for 1.18 that says that we dropped direct libevdev access in favour of libinput.

::: clutter/evdev/clutter-device-manager-evdev.c
@@ +103,3 @@
 G_DEFINE_TYPE_WITH_PRIVATE (ClutterDeviceManagerEvdev,
                             clutter_device_manager_evdev,
+                            CLUTTER_TYPE_DEVICE_MANAGER);

don't add a semi-colon: it's pointless.

@@ +684,3 @@
+
+static ClutterSeatEvdev *
+

`get_seat_by_name()` feels wrong, since you're passing a libinput_seat. you should either pass a libinput_seat and call it `get_seat_from_libinput_seat()`, or pass the seat name, and add a different function to create a new ClutterSeatEvdev from a libinput_seat, and add it to the device manager. I'd rather opt for the second approach: right now, get_seat_by_name() has this side effect of creating a new seat that is not documented at all.

@@ +693,1 @@
+  for (l = priv->seats; l; l = g_slist_next (l))

don't use g_slist_next() - use l->next; it's not like we're ever going to change the contents of GSList.

@@ +697,1 @@
+      if (!strcmp (libinput_seat_get_logical_name (libinput_seat),

don't use !strcmp(): use strcmp() == 0.

also, coding style: don't break lines inside a condition.

@@ +717,3 @@
+  libinput_seat = libinput_device_get_seat (libinput_device);
+  seat = libinput_seat_get_user_data (libinput_seat);
+  if (!seat)

coding style: use explicit NULL comparison for pointers.

@@ +723,3 @@
    * ClutterInputDevice API */
+  if (libinput_device_has_capability (libinput_device,
+                                      LIBINPUT_DEVICE_CAP_KEYBOARD))

coding style: don't break lines inside a condition.

@@ +851,3 @@
   priv = manager_evdev->priv;
 
+  for (l = priv->seats; l; l = l->next)

if we're going to go through the devices for each seat, then we don't need to store the list of devices inside the DeviceManager.

@@ +944,3 @@
+                    LIBINPUT_KEYBOARD_KEY_STATE_PRESSED;
+
+process_device_event (ClutterDeviceManagerEvdev *manager_evdev,

coding style: please, don't add line breaks after the parenthesis before the first argument.

@@ +960,3 @@
+        device = libinput_device_get_user_data (libinput_device);
+
+    {

same as above.

@@ +986,3 @@
+        stage_height = clutter_actor_get_height (CLUTTER_ACTOR (stage));
+
+          libinput_event_keyboard_get_time (key_event),

same as above.

as a side note: I can't *believe* libinput has a fixed point representation in its public API. this is so, so, so wrong.

@@ +989,3 @@
+          libinput_event_pointer_get_absolute_x_transformed (
+            motion_event, stage_width));
+          libinput_event_keyboard_get_key (key_event),

same as above.

@@ +994,3 @@
+
+        notify_absolute_motion (
+

same as above.

@@ +1011,3 @@
+                       LIBINPUT_POINTER_BUTTON_STATE_PRESSED;
+
+        device = libinput_device_get_user_data (libinput_device);

same as above.

@@ +1028,3 @@
+        device = libinput_device_get_user_data (libinput_device);
+
+      }

same as above.

@@ +1084,3 @@
+
+static int
+          libinput_event_pointer_get_time (motion_event),

coding style: arguments should go on separate lines.

::: clutter/evdev/clutter-input-device-evdev-private.h
@@ +27,3 @@
+typedef struct _ClutterInputDeviceEvdevPrivate ClutterInputDeviceEvdevPrivate;
+
+struct _ClutterInputDeviceEvdevPrivate

why do we need a shared private structure, when ClutterInputDeviceEvdev is a private/final type? we should probably fold the private instance data into the instance data itself. I don't expect people to subclass ClutterInputDeviceEvdev outside of Clutter, and even if we do allow that in the future, we can also migrate the instance data to a private structure at that time.

::: clutter/evdev/clutter-input-device-evdev.c
@@ +48,3 @@
 G_DEFINE_TYPE_WITH_PRIVATE (ClutterInputDeviceEvdev,
                             clutter_input_device_evdev,
+                            CLUTTER_TYPE_INPUT_DEVICE);

don't add a semi-colon: it's pointless.

@@ +54,3 @@
+ * created seat will get the ids 2 and 3.
+ */
+static gint global_device_id_next = 2;

'2' should probably be a #define like INITIAL_DEVICE_ID, with a comment about why we chose 2.

@@ +68,3 @@
     case PROP_SYSFS_PATH:
+      /* Deprecated */
+      g_value_set_string (value, NULL);

meh. not that I like it very much.

in theory, you could simply ignore it: the default GValue value for a string is NULL anyway.

to be fair, I always considered the evdev backend to not be committed to a stable API, given that's is used only by Mutter/GNOME Shell; so we could just remove these properties.

@@ +177,3 @@
 }
 
+ClutterInputDevice *

needs a documentation block.

@@ +187,3 @@
+
+  if (libinput_device_has_capability (libinput_device,
+{

coding style: please, don't break lines while inside a condition.

@@ +217,3 @@
+}
+
+    type = CLUTTER_EXTENSION_DEVICE;

missing documentation block.

@@ +226,3 @@
+  const char *name;
+
+                         "device-manager", manager,

coding style: braces on a new line, and a new indentation level.

::: clutter/evdev/clutter-input-device-evdev.h
@@ +58,2 @@
 typedef struct _ClutterInputDeviceEvdev ClutterInputDeviceEvdev;
+typedef struct _ClutterSeatEvdev ClutterSeatEvdev;

I guess if we want to use this API from gnome-shell then we'll need a ClutterSeatEvdev GType.

@@ +71,3 @@
+ClutterSeatEvdev *        _clutter_input_device_evdev_get_seat        (ClutterInputDeviceEvdev *device);
+
+ClutterInputDevice *      clutter_input_device_evdev_new_virtual      (ClutterDeviceManager    *manager,

coding style: arguments need to be on separate lines.

Review of attachment 268004 [details] [review]:

::: clutter/evdev/clutter-device-manager-evdev.c
@@ +684,3 @@
+
+static ClutterSeatEvdev *
+get_seat_by_name (ClutterDeviceManagerEvdev *manager_evdev,

The reason for having this function is that libinput_seat's are not "recycled" after having suspended libinput, and new ones are created. In weston seats may not be controlled only by the input layer, so a common id is used for identifying a seat, creating it on-demand. I think it'd be a good idea to change the semantics to reuse seats if the user keeps a reference to it, and if so, this _get_by_name() is not needed any more.

@@ +851,3 @@
   priv = manager_evdev->priv;
 
+  for (l = priv->seats; l; l = l->next)

The problem is the function clutter_device_manager_evdev_get_devices () which requires a const GSList * that the user (AFAIU) need not to free.

My intention was to have the DeviceManager devices list only do that, and nothing else.

@@ +986,3 @@
+        stage_height = clutter_actor_get_height (CLUTTER_ACTOR (stage));
+
+        x = li_fixed_to_double (

This choice was simply because it is the Wayland API uses compatible fixed point numbers, and libinput is meant to be a library used by Wayland compositors.

Created attachment 268477 [details] [review]
evdev: Port evdev input backend to libinput

Instead of having its own evdev input device processing implementation,
make clutter's evdev backend use libinput to do input device processing
for it.

Two GObject parameters of ClutterInputDeviceEvdev (sysfs-path and
device-path) are deprecated as they are not used any more for
construction.

Before ClutterDeviceManagerEvdev had one virtual core keyboard and one
virtual core pointer device. These are now instead separated into seats,
which all have one virtual core keybard and pointer device respectively.

The 'global' core keyboard and pointer device are the core keyboard and
pointer device of the first seat that is created.

A ClutterInputDeviceEvdev can, as before, both represent a real physical
device or a virtual device, but is now instead created either via
clutter_input_device_evdev_new() for real devices, and
clutter_input_device_new_virtual() for virtual devices.

XKB state and button state is moved to the seat structure and is thus
separated per seat. Seats are not a concept exposed outside of clutter's
evdev backend.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Review of attachment 268477 [details] [review]:

Looking good to me. Do we want to land this before there's a libinput tarball out?

::: clutter/evdev/clutter-device-manager-evdev.c
@@ +678,2 @@
+      if (_clutter_input_device_get_stage (device) == NULL)
+        _clutter_input_device_set_stage (device, stage);

This means that we can't unset the stage. Should be unconditional

@@ +739,3 @@
+
+  CLUTTER_NOTE (EVENT, "Added physical device '%s', type %s",
+                "Libinput device",

clutter_input_device_get_device_name() ?

::: clutter/evdev/clutter-input-device-evdev.c
@@ +33,2 @@
 #include "clutter-input-device-evdev.h"
+#include "clutter-input-device-evdev-private.h"

We don't need this -private header since clutter-input-device-evdev.h is private already and included in both places we need it.

Created attachment 268847 [details] [review]
evdev: Port evdev input backend to libinput

Instead of having its own evdev input device processing implementation,
make clutter's evdev backend use libinput to do input device processing
for it.

Two GObject parameters of ClutterInputDeviceEvdev (sysfs-path and
device-path) are deprecated as they are not used any more for
construction.

Before ClutterDeviceManagerEvdev had one virtual core keyboard and one
virtual core pointer device. These are now instead separated into seats,
which all have one virtual core keybard and pointer device respectively.

The 'global' core keyboard and pointer device are the core keyboard and
pointer device of the first seat that is created.

A ClutterInputDeviceEvdev can, as before, both represent a real physical
device or a virtual device, but is now instead created either via
clutter_input_device_evdev_new() for real devices, and
clutter_input_device_new_virtual() for virtual devices.

XKB state and button state is moved to the seat structure and is thus
separated per seat. Seats are not a concept exposed outside of clutter's
evdev backend.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

(In reply to comment #9)
> Looking good to me. Do we want to land this before there's a libinput tarball
> out?

I think we're about to call some basic API stable enough, so might be best to wait for that before merging anything.

Review of attachment 268847 [details] [review]:

looks good to me, except for the fact that we expose two new functions and I don't like that.

also, as Peter said, we should wait until libinput has a release with some form of API stability before pushing this commit.

::: clutter/evdev/clutter-input-device-evdev.c
@@ +100,3 @@
+ * Since: 1.18
+ */
+ *

if this constructor is going to be public, it will need to be in a publicly installed header. it also does much more than g_object_new(), which means that it's not really going to be usable by language bindings.

I guess the real question is: does this constructor need to be public? is Mutter/GNOME Shell going to use it? we don't provide ClutterInputDevice constructors for any other backend. personally, I think it's wrong to have it in the public API.

if it's not going to be public, the function name should start with an underscore, and the documentation block should not have /** at the beginning, lest gtk-doc thinks it needs to document it.

@@ +138,3 @@
+ */
+ClutterInputDevice *
+                         "device-type", type,

same as above, really.

::: configure.ac
@@ +477,3 @@
               [
                 CLUTTER_INPUT_BACKENDS="$CLUTTER_INPUT_BACKENDS evdev"
+                BACKEND_PC_FILES="$BACKEND_PC_FILES libudev >= $LIBUDEV_REQ_VERSION libinput >= $LIBINPUT_REQ_VERSION xkbcommon"

since Clutter does not expose any public symbol with libudev and libinput types, these should go in the private requirements, not the public ones.

(In reply to comment #12)
> since Clutter does not expose any public symbol with libudev and libinput
> types, these should go in the private requirements, not the public ones.

Not disagreeing with this for now but, I think we will soon need to expose libinput objects to upper layers though.

Things like configuring pointer and touchpad parameters from user settings will need to be done at the mutter or gnome-shell level and I don't think we want to wrap all of that in clutter API, especially since that part of libinput API isn't even fleshed out yet IIUC.

(In reply to comment #13)
> (In reply to comment #12)
> > since Clutter does not expose any public symbol with libudev and libinput
> > types, these should go in the private requirements, not the public ones.
> 
> Not disagreeing with this for now but, I think we will soon need to expose
> libinput objects to upper layers though.

eh, yuck. I already hate exposing native types for x11.

> Things like configuring pointer and touchpad parameters from user settings will
> need to be done at the mutter or gnome-shell level and I don't think we want to
> wrap all of that in clutter API, especially since that part of libinput API
> isn't even fleshed out yet IIUC.

if Mutter/Shell needs specific API we'll have to start using a specific #define, like CLUTTER_ENABLE_COMPOSITOR_API, and that you're going to be able to access only through C, and it also does not get any ABI stability guarantee. maybe even a separate shared object, even a shim one.

in any case, I think we'll care about that when we get to it. for the time being, let's make this whole thing private.

Doesn't the evdev backend already have no stability guarantee?

(In reply to comment #15)
> Doesn't the evdev backend already have no stability guarantee?

it's a completely informal definition, as it is now.

Created attachment 269070 [details] [review]
evdev: Port evdev input backend to libinput

Instead of having its own evdev input device processing implementation,
make clutter's evdev backend use libinput to do input device processing
for it.

Two GObject parameters of ClutterInputDeviceEvdev (sysfs-path and
device-path) are removed as they are not used any more.

Before ClutterDeviceManagerEvdev had one virtual core keyboard and one
virtual core pointer device. These are now instead separated into seats,
which all have one virtual core keyboard and pointer device respectively.

The 'global' core keyboard and pointer device are the core keyboard and
pointer device of the first seat that is created.

A ClutterInputDeviceEvdev can, as before, both represent a real physical
device or a virtual device, but is now instead created either via
_clutter_input_device_evdev_new() for real devices, and
_clutter_input_device_new_virtual() for virtual devices.

XKB state and button state is moved to the seat structure and is thus
separated per seat. Seats are not a concept exposed outside of clutter's
evdev backend.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

(In reply to comment #11)
> (In reply to comment #9)
> > Looking good to me. Do we want to land this before there's a libinput tarball
> > out?
> 
> I think we're about to call some basic API stable enough, so might be best to
> wait for that before merging anything.

Before we set anything in stone, can we review and test the weston first?

(In reply to comment #18)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Looking good to me. Do we want to land this before there's a libinput tarball
> > > out?
> > 
> > I think we're about to call some basic API stable enough, so might be best to
> > wait for that before merging anything.
> 
> Before we set anything in stone, can we review and test the weston first?

I agree with waiting, and think reviewing the implementation in weston should happen before any API stability is promised.

Thanks for the libinput release Jonas. I'm now pushing this. I think
it's clear that libinput's API isn't considered stable but that's
OK since Clutter's evdev backend isn't API stable either.

Attachment 269070 [details] pushed as dacb515 - evdev: Port evdev input backend to libinput

Thanks Rui. Might it be a good idea to check for 0.1.x during configure step? (It now checks for >= 0.0.90)