Created attachment 332130 [details] [review]
settings: Support mouse and trackball accel profile

Support changing the mouse and trackball acceleration profile. This
makes it possible to for example disable pointer acceleration by
choosing the 'flat' profile.

This adds an optional dependency on gudev. Gudev is used by the X11
backend to detect whether a device is a mouse or not. Without gudev
support, the accel profile settings has have effect for mouse devices.

Trackball still uses the "strstr" approach, since udev doesn't support
tagging devices as trackball devices yet.

Review of attachment 332130 [details] [review]:

"Without gudev support, the accel profile settings has have effect for mouse devices"

"has have" => "has no"?

::: src/backends/x11/meta-input-settings-x11.c
@@ +479,3 @@
+    meta_input_settings_x11_get_instance_private (settings_x11);
+
+  g_object_unref (&priv->udev_client);

Shouldn't references be released in dispose()? Could use g_clear_object().

(In reply to Florian Müllner from comment #2)
> Review of attachment 332130 [details] [review] [review]:
> 
> "Without gudev support, the accel profile settings has have effect for mouse
> devices"
> 
> "has have" => "has no"?

Oops. Seems I "fixed" my grammar by making it worse.

> 
> ::: src/backends/x11/meta-input-settings-x11.c
> @@ +479,3 @@
> +    meta_input_settings_x11_get_instance_private (settings_x11);
> +
> +  g_object_unref (&priv->udev_client);
> 
> Shouldn't references be released in dispose()? Could use g_clear_object().

I thought that only mattered if there was a risk of circular dependencies, which there are no here. I could change it to a dispose I guess.

(In reply to Jonas Ådahl from comment #3)
> > Shouldn't references be released in dispose()? Could use g_clear_object().
> 
> I thought that only mattered if there was a risk of circular dependencies,
> which there are no here.

Yeah, I just have the habit of using the "right" function in all cases to avoid mistakes when later additions would require dispose() but are just added to the existing function. It's unlikely that this will ever be an issue in this case, so up to you if you want to change it or not.

Changed to g_clear_object() and dispose instead of finalize.

Attachment 332130 [details] pushed as 23c4ac6 - settings: Support mouse and trackball accel profile

update_pointer_accel_profile (input_settings, mouse_settings, device);
update_pointer_accel_profile (input_settings, trackball_settings, device);

Should not above lines be added in apply_device_settings? Like other update_* functions...

(In reply to Alberts Muktupāvels from comment #6)
> update_pointer_accel_profile (input_settings, mouse_settings, device);
> update_pointer_accel_profile (input_settings, trackball_settings, device);
> 
> Should not above lines be added in apply_device_settings? Like other
> update_* functions...

Well spotted. Attaching patches.

Created attachment 332324 [details] [review]
MetaInputSettings: Don't initialize the same setting twice

Two settings were set twice on the same device. Now instead group the
generic update functions together, removing the redundant calls.

Created attachment 332325 [details] [review]
MetaInputSettings: Initialize the accel-profile setting

Shouldn't just update them when they change; they also need to be
set when initializing.

Created attachment 332328 [details] [review]
MetaInputSettings: Initialize the accel-profile setting

Shouldn't just update them when they change; they also need to be
set when initializing.

---

Now actually compile tested.

Comment on attachment 332324 [details] [review]
MetaInputSettings: Don't initialize the same setting twice

LGTM

Comment on attachment 332328 [details] [review]
MetaInputSettings: Initialize the accel-profile setting

...but here in turn you seem to add doubly calls on the same device, just with different settings :)

Reading through code I see a similar problem in meta_input_settings_changed_cb(), update_pointer_accel_profile(..., NULL) is called from mouse/trackball settings, but no check is performed when iterating through the devices, that looks like it will result in all devices being updated to whatever setting was changed last.

I'll suggest splitting the device ==/!= NULL cases into 2 functions:

- update_pointer_accel_profile(MetaInputSettings*, ClutterInputDevice*) that looks up the right settings based on the device type
- update_pointer_accel_profile_from_settings(MetaInputSettings,GSettings) (or _foreach maybe?) that iterates through devices and skips the ones where the given GSettings don't apply.

(In reply to Carlos Garnacho from comment #12)
> Comment on attachment 332328 [details] [review] [review]
> MetaInputSettings: Initialize the accel-profile setting
> 
> ...but here in turn you seem to add doubly calls on the same device, just
> with different settings :)

Right. The passed settings is used to determine the device that gets updated.

> 
> Reading through code I see a similar problem in
> meta_input_settings_changed_cb(), update_pointer_accel_profile(..., NULL) is
> called from mouse/trackball settings, but no check is performed when
> iterating through the devices, that looks like it will result in all devices
> being updated to whatever setting was changed last.

Since the check is backend specific, it is checked there, in the backend. In the -native backend we use the struct udev_device from libinput_device, while in the -x11 backend we use the device node from the X11 device and create a GUdevDevice from it. So, only the correct device will get updated still, even if update_pointer_accel_profile is called with it passed.

Maybe we should consider moving device class tagging to ClutterInputDevice and have the clutter backneds deal with it. The API we have is awkward though, we have "device type" which can be "pointer" or "touchpad" etc, and if we introduce "trackball" and "mouse", we'd break things here and there. The reasonable thing, I think, would be to introduce another API next to device type (and remove the touchpad device type). If so, that should be a follow up though IMHO.

> 
> I'll suggest splitting the device ==/!= NULL cases into 2 functions:
> 
> - update_pointer_accel_profile(MetaInputSettings*, ClutterInputDevice*) that
> looks up the right settings based on the device type
> - update_pointer_accel_profile_from_settings(MetaInputSettings,GSettings)
> (or _foreach maybe?) that iterates through devices and skips the ones where
> the given GSettings don't apply.

I suppose we could. update_pointer_accel_profil() would still need the GSettings though, if we don't want to have the backend find the proper GSetting and fetch the value itself (which we don't seem to do anywhere yet). It'd do the exact same thing passing NULL vs not NULL as input device.

(In reply to Jonas Ådahl from comment #13)
> (In reply to Carlos Garnacho from comment #12)
> > Comment on attachment 332328 [details] [review] [review] [review]
> > MetaInputSettings: Initialize the accel-profile setting
> > 
> > ...but here in turn you seem to add doubly calls on the same device, just
> > with different settings :)
> 
> Right. The passed settings is used to determine the device that gets updated.

Ah, the thing I missed is that it's x11/native implementations which perform the check on the device. This makes the problems I saw somewhat moot.

> 
> > 
> > Reading through code I see a similar problem in
> > meta_input_settings_changed_cb(), update_pointer_accel_profile(..., NULL) is
> > called from mouse/trackball settings, but no check is performed when
> > iterating through the devices, that looks like it will result in all devices
> > being updated to whatever setting was changed last.
> 
> Since the check is backend specific, it is checked there, in the backend. In
> the -native backend we use the struct udev_device from libinput_device,
> while in the -x11 backend we use the device node from the X11 device and
> create a GUdevDevice from it. So, only the correct device will get updated
> still, even if update_pointer_accel_profile is called with it passed.

Uhm... This is the first precedence of device checks being performed in the vfunc. It was an original design choice to keep these checks out from vfuncs, and make those only about applying the configuration. 

Back then, ClutterInputDeviceType was "sufficient" too... When adding later trackball-specific configuration, I added this meta_input_device_is_trackball() helper in meta-input-settings.c. I agree your backend-specific checks are far more correct, but I kinda expected this to be used when reading your patch last evening :).

> 
> Maybe we should consider moving device class tagging to ClutterInputDevice
> and have the clutter backneds deal with it. The API we have is awkward
> though, we have "device type" which can be "pointer" or "touchpad" etc, and
> if we introduce "trackball" and "mouse", we'd break things here and there.
> The reasonable thing, I think, would be to introduce another API next to
> device type (and remove the touchpad device type). If so, that should be a
> follow up though IMHO.

I agree, previously thinking about this I came up to the conclusion we should split capabilities (eg. rel/abs,key,...) from device type (trackball, touchpad, "test",...). Still, I don't think ClutterInputDeviceType can be reused in a compatible manner :(.

> 
> > 
> > I'll suggest splitting the device ==/!= NULL cases into 2 functions:
> > 
> > - update_pointer_accel_profile(MetaInputSettings*, ClutterInputDevice*) that
> > looks up the right settings based on the device type
> > - update_pointer_accel_profile_from_settings(MetaInputSettings,GSettings)
> > (or _foreach maybe?) that iterates through devices and skips the ones where
> > the given GSettings don't apply.
> 
> I suppose we could. update_pointer_accel_profil() would still need the
> GSettings though, if we don't want to have the backend find the proper
> GSetting and fetch the value itself (which we don't seem to do anywhere
> yet). It'd do the exact same thing passing NULL vs not NULL as input device.

In other places where this happens (mouse vs touchpad, basically), we have this get_settings_for_device_type() helper. Unfortunately I don't think we can make this helper cover the trackball case, as trackballs want to use priv->mouse_settings for most other settings, and we don't have a "trackball" device type.

(In reply to Carlos Garnacho from comment #14)
> (In reply to Jonas Ådahl from comment #13)
> > (In reply to Carlos Garnacho from comment #12)
> > > Reading through code I see a similar problem in
> > > meta_input_settings_changed_cb(), update_pointer_accel_profile(..., NULL) is
> > > called from mouse/trackball settings, but no check is performed when
> > > iterating through the devices, that looks like it will result in all devices
> > > being updated to whatever setting was changed last.
> > 
> > Since the check is backend specific, it is checked there, in the backend. In
> > the -native backend we use the struct udev_device from libinput_device,
> > while in the -x11 backend we use the device node from the X11 device and
> > create a GUdevDevice from it. So, only the correct device will get updated
> > still, even if update_pointer_accel_profile is called with it passed.
> 
> Uhm... This is the first precedence of device checks being performed in the
> vfunc. It was an original design choice to keep these checks out from
> vfuncs, and make those only about applying the configuration. 
> 
> Back then, ClutterInputDeviceType was "sufficient" too... When adding later
> trackball-specific configuration, I added this
> meta_input_device_is_trackball() helper in meta-input-settings.c. I agree
> your backend-specific checks are far more correct, but I kinda expected this
> to be used when reading your patch last evening :).

Well, could use is-trackball() but then half of the device type checking would in be -native/x11 and half in the generic, which seemed wrong. FWIW, I talked to Peter about trackball identification, and we concluded that we should just put it in udev with the rest of all the identification.

> 
> > 
> > Maybe we should consider moving device class tagging to ClutterInputDevice
> > and have the clutter backneds deal with it. The API we have is awkward
> > though, we have "device type" which can be "pointer" or "touchpad" etc, and
> > if we introduce "trackball" and "mouse", we'd break things here and there.
> > The reasonable thing, I think, would be to introduce another API next to
> > device type (and remove the touchpad device type). If so, that should be a
> > follow up though IMHO.
> 
> I agree, previously thinking about this I came up to the conclusion we
> should split capabilities (eg. rel/abs,key,...) from device type (trackball,
> touchpad, "test",...). Still, I don't think ClutterInputDeviceType can be
> reused in a compatible manner :(.

Right. I think the clutter device type should just be 'keyboard', 'pointer', 'touch', 'tablet' and then we introduce another thing... hmm. clutter_input_device_pointer_type()? I.e. something that'd be 'mouse', 'touchpad', 'trackball', 'trackpoint' maybe. That way we don't need to duplicate 'keyboard' and 'touch' in a new type since those don't have that many differnet type of hardware incarnations

> 
> > 
> > > 
> > > I'll suggest splitting the device ==/!= NULL cases into 2 functions:
> > > 
> > > - update_pointer_accel_profile(MetaInputSettings*, ClutterInputDevice*) that
> > > looks up the right settings based on the device type
> > > - update_pointer_accel_profile_from_settings(MetaInputSettings,GSettings)
> > > (or _foreach maybe?) that iterates through devices and skips the ones where
> > > the given GSettings don't apply.
> > 
> > I suppose we could. update_pointer_accel_profil() would still need the
> > GSettings though, if we don't want to have the backend find the proper
> > GSetting and fetch the value itself (which we don't seem to do anywhere
> > yet). It'd do the exact same thing passing NULL vs not NULL as input device.
> 
> In other places where this happens (mouse vs touchpad, basically), we have
> this get_settings_for_device_type() helper. Unfortunately I don't think we
> can make this helper cover the trackball case, as trackballs want to use
> priv->mouse_settings for most other settings, and we don't have a
> "trackball" device type.

So what do you suggest?

Comment on attachment 332328 [details] [review]
MetaInputSettings: Initialize the accel-profile setting

The patch looks alright to me after seeing the x11/native specific code checks the device type. There's improvements to do here, but let's take those to a different bug.

Comment on attachment 332328 [details] [review]
MetaInputSettings: Initialize the accel-profile setting

The patch looks alright to me after seeing the x11/native specific code checks the device type. There's improvements to do here, but let's take those to a different bug.

Attachment 332324 [details] pushed as 53e3d0d - MetaInputSettings: Don't initialize the same setting twice
Attachment 332328 [details] pushed as 53061c7 - MetaInputSettings: Initialize the accel-profile setting

Filed bug #769460 about the device type detection.

*** Bug 763293 has been marked as a duplicate of this bug. ***