GNOME Bugzilla – Bug 739397
Synchronize/apply input devices' session-wide configuration
Last modified: 2015-01-18 21:33:52 UTC
I'm attaching a few patches to make the mutter backends handle input device configuration, currently restricted to mouse/touchpad settings. The settings handled so far are: - mouse/touchpad handedness - mouse/touchpad acceleration - touchpad natural scroll - touchpad tap-enabled - touchpad enabling/disabling One thing worth noting, this at the moment imposes an indirect dependency on gnome-settings-daemon, as the gsettings schemas are installed from there. But using GSettings struck me as most practical since mutter wants the configuration applied right from startup, a dbus api for this is prone to synchronization issues on availability changes. Eventually, gsettings-desktop-schemas would be a good place for the schemas IMO. Some more things that I would think make sense to be eventually moved under mutter domain are: - cursor hiding/showing - touchpad disable-while-typing - touchpad edge scroll vs two-finger scroll (although there's no way currently to change the detected default on libinput) - horizontal scroll enabled (also lacking for libinput, something could be done on the mutter side though) - touchscreen mapping: There is currently no GSettings configuration for these at all, g-s-d figures out the mapping at runtime. I would say it's better to add settings for these too, so everything needed on the mutter side is a set_matrix() call, and the tricky heuristics can stay in g-s-d. - wacom device mapping: There's again nothing backing this yet on the libinput side, although nothing prevents us from doing this just for x11 at the moment. - keyboard repeat settings As for other input device features in gnome-settings-daemon, I'm more undecided, or for some, not so sure we should drag these along to a wayland world...
Created attachment 289649 [details] [review] backends: Add MetaInputSettings This object internally keeps track of the relevant input configuration, and goes through its vmethods in order to apply the configuration on the backend-specific devices. So far, only mouse/touchpad settings are actually attached to GSettings changes. ::set_matrix(), meant for tablets/touchscreens, is not hooked yet. One caveat is that meta_input_settings_create() may return NULL if the backend does not own the windowing system (wayland nested on X11 being the one case), and thus device settings can't be changed freely.
Created attachment 289650 [details] [review] backends/x11: Implement X11-specific MetaInputSettings This goes through modifying XI2 device properties, and painfully through XI1 calls in the case of set_speed()/set_left_handed()... Mixing these calls with XI2 should be ok, just unpleasant.
Created attachment 289651 [details] [review] backends/native: Add libinput-based MetaInputSettings implementation The libinput_device is fetched from the ClutterInputDevice, and configured through the libinput_device_*config* API.
Created attachment 289652 [details] [review] backend: Maintain a ClutterInputSettings object This object keeps itself track of device configuration, it's only so far only needed alive for as long as the backend object does.
Review of attachment 289650 [details] [review]: This code should change the xf86-input-libinput properties instead, and leave the gory XI2 code inside gnome-settings-daemon to fall back on.
(In reply to comment #5) > Review of attachment 289650 [details] [review]: > > This code should change the xf86-input-libinput properties instead, and leave > the gory XI2 code inside gnome-settings-daemon to fall back on. I agree, ack.
Created attachment 292464 [details] [review] backends: Add MetaInputSettings This object internally keeps track of the relevant input configuration, and goes through its vmethods in order to apply the configuration on the backend-specific devices. So far, only mouse/touchpad settings are actually attached to GSettings changes. ::set_matrix(), meant for tablets/touchscreens, is not hooked yet. One caveat is that meta_input_settings_create() may return NULL if the backend does not own the windowing system (wayland nested on X11 being the one case), and thus device settings can't be changed freely.
Created attachment 292465 [details] [review] backends/x11: Implement X11-specific MetaInputSettings This goes through modifying XI2 device properties, either common ones (eg. set on every device) or those specific to the libinput X11 driver. Keyboard repeat/rate are set through core and XKB APIs.
Created attachment 292466 [details] [review] backends/native: Add libinput-based MetaInputSettings implementation The libinput_device is fetched from the ClutterInputDevice, and configured through the libinput_device_*config* API.
Created attachment 292467 [details] [review] backend: Maintain a MetaInputSettings object This object keeps itself track of device configuration, it's only so far only needed alive for as long as the backend object does.
Created attachment 292468 [details] [review] native: Remove previous listener for keyboard settings The settings-daemon peripherals schemas are going away, and this is now handled through MetaInputSettings.
Created attachment 292469 [details] [review] wayland: Use the new keyboard settings location for repeat settings This makes keyboard repeat in clients in-sync with the input config changes.
A couple of notes from these last patches: - On wayland, disabling, and then enabling the touchpad makes libinput enter into busy wait, the patch at http://lists.freedesktop.org/archives/wayland-devel/2014-December/018916.html fixes the issue - On both x11/wayland, touchpad device type detection is somewhat amiss, the patches at bug #741350 make mutter apply correctly the config for those.
Just noticed that the commit log in the first patch is inaccurate now... Additionally to mice/touchpads, trackball and keyboard settings are indeed handled too.
Created attachment 292566 [details] [review] monitor-manager: Add method to find an output matrix This will be useful to determine input device matrices for touchscreens and tablets.
Created attachment 292567 [details] [review] input-settings: Handle device-to-output mapping For each device that can be mapped (touchscreens, tablets), the output will be fetched from settings and matched with the currently connected ones. If a match is found, the device matrix will be found out from the output configuration and set on the device. This is also updated both individually for newly connected devices, and collectively on output configuration changes.
These last 2 patches implement device mapping for touchscreens and tablets (the latter only actually applies to x11, libinput/clutter are not ready for tablets yet). The first patch is somewhat independent of the other changes, but the latter depends on the clutter patches in bug #740759 in order to create GSettings paths for each of these devices. NB: I think the matrices for inverted output modes are correct, but couldn't fully verify them, I only seemed to get META_MONITOR_TRANSFORM_FLIPPED/FLIPPED_180 regardless of my --rotate and --reflect combinations on xrandr.
Where did we end up on this? Is this ready for review?
*** Bug 728052 has been marked as a duplicate of this bug. ***
(In reply to comment #18) > Where did we end up on this? Is this ready for review? Yeah, I think this is ready for review. The clutter patches are still unapplied, but any potential change there just requires minor adjustments here.
Review of attachment 292464 [details] [review]: Not a giant fan of the indirection in this. I'd rather see something more obvious and straightforward, rather than function pointers all over the place. Might make the code slightly bigger, but hopefully much easier to read. ::: src/backends/meta-input-settings.c @@ +41,3 @@ +#include <meta/util.h> + +#define KEY_LEFT_HANDED "left-handed" Don't do this. Just use the string constants directly in the code. @@ +201,3 @@ + handedness = g_settings_get_enum (priv->touchpad_settings, KEY_LEFT_HANDED); + + if (handedness == G_DESKTOP_TOUCHPAD_HANDEDNESS_RIGHT) Use a switch statement so you don't forget an enum. @@ +414,3 @@ + + if (device && device_is_trackball (device)) + input_settings_class->set_scroll_button (input_settings, device, button); Braces on both sides. @@ +455,3 @@ +static void +meta_input_settings_changed_cb (GSettings *settings, + const gchar *key, Don't use "gchar" in new code. @@ +456,3 @@ +meta_input_settings_changed_cb (GSettings *settings, + const gchar *key, + MetaInputSettings *input_settings) I prefer "gpointer user_data" and "MetaInputSettings *input_settings = META_INPUT_SETTINGS (user_data);" in the code. @@ +460,3 @@ + MetaInputSettingsPrivate *priv; + + priv = meta_input_settings_get_instance_private (input_settings); You can put this on the same line as the decl. @@ +464,3 @@ + if (settings == priv->mouse_settings) + { + if (strcmp (key, KEY_LEFT_HANDED) == 0) This seems a bit ridiculous. Why not use the detailed signals? @@ +520,3 @@ + if (type == CLUTTER_POINTER_DEVICE) + { + update_mouse_left_handed (input_settings, NULL); Any reason you don't pass device? @@ +530,3 @@ + update_touchpad_left_handed (input_settings, device); + update_touchpad_tap_enabled (input_settings, device); + update_touchpad_scroll_method (input_settings, NULL); Any reason you don't pass device? @@ +621,3 @@ +#endif + if (!meta_is_wayland_compositor ()) + return g_object_new (META_TYPE_INPUT_SETTINGS_X11, NULL); Cool build breaks.
Review of attachment 292465 [details] [review]: ::: src/backends/x11/meta-input-settings-x11.c @@ +63,3 @@ + + if (rc == Success) + XFree (data_ret); Use meta_XFree unconditionally above the ChangeProperty call. @@ +156,3 @@ + GDesktopTouchpadScrollMethod mode) +{ + guchar values[3] = { 0 }; /* 2fg, edge, button */ Might want to add a comment that we don't support button? Was looking to see where [2] was assigned to. @@ +168,3 @@ + break; + default: + g_critical ("Unhandled scroll method %x", mode); You can use g_assert_not_reached ();. @@ +200,3 @@ + } + else + XAutoRepeatOff (xdisplay); Braces on both sides.
Review of attachment 292466 [details] [review]: ::: src/backends/native/meta-input-settings-native.c @@ +39,3 @@ + struct libinput_device *libinput_device; + + switch (mode) { bad brace style @@ +47,3 @@ + break; + case G_DESKTOP_DEVICE_SEND_EVENTS_ENABLED: + default: remove the default from here, put a g_assert_not_reached @@ +147,3 @@ + libinput_device = clutter_evdev_input_device_get_libinput_device (device); + + switch (mode) { bad brace style @@ +158,3 @@ + break; + default: + g_critical ("Unhandled scroll method %x", scroll_method); g_assert_not_reached
Review of attachment 292467 [details] [review]: cool. feel free to squash this in with the original commit tbh
Review of attachment 292468 [details] [review]: nice.
Review of attachment 292469 [details] [review]: good.
Review of attachment 292566 [details] [review]: ::: src/backends/meta-monitor-manager.c @@ +46,3 @@ +/* Array index matches MetaMonitorTransform */ +static gfloat trasform_matrices[][6] = { "trasform" @@ +47,3 @@ +/* Array index matches MetaMonitorTransform */ +static gfloat trasform_matrices[][6] = { + { 1, 0, 0, 0, 1, 0}, /* normal */ missing space here some amount of alignment would be nice @@ +1238,3 @@ + +static inline void +multiply_matrix (float a[6], sheesh. i know we have ten different matrix libs already, but this is just sort of weird. i don't know if we should use cogl / cairo or just keep this. i don't really care. feel free to keep this.
Review of attachment 292567 [details] [review]: yes. ::: src/backends/meta-input-settings.c @@ +717,3 @@ } + else + check_add_mappable_device (input_settings, device); brace style
Created attachment 294462 [details] [review] backends: Add MetaInputSettings This object internally keeps track of the relevant input configuration, and goes through its vmethods in order to apply the configuration on the backend-specific devices. So far, only mouse/touchpad settings are actually attached to GSettings changes. ::set_matrix(), meant for tablets/touchscreens, is not hooked yet. One caveat is that meta_input_settings_create() may return NULL if the backend does not own the windowing system (wayland nested on X11 being the one case), and thus device settings can't be changed freely.
Created attachment 294463 [details] [review] backends/x11: Implement X11-specific MetaInputSettings This goes through modifying XI2 device properties, either common ones (eg. set on every device) or those specific to the libinput X11 driver. Keyboard repeat/rate are set through core and XKB APIs.
Created attachment 294464 [details] [review] backends/native: Add libinput-based MetaInputSettings implementation The libinput_device is fetched from the ClutterInputDevice, and configured through the libinput_device_*config* API.
Created attachment 294465 [details] [review] input-settings: Handle device-to-output mapping For each device that can be mapped (touchscreens, tablets), the output will be fetched from settings and matched with the currently connected ones. If a match is found, the device matrix will be found out from the output configuration and set on the device. This is also updated both individually for newly connected devices, and collectively on output configuration changes.
Created attachment 294466 [details] [review] monitor-manager: Add method to find an output matrix This will be useful to determine input device matrices for touchscreens and tablets.
I applied most suggestions, some comments: (In reply to comment #21) > Review of attachment 292464 [details] [review]: > > Not a giant fan of the indirection in this. I'd rather see something more > obvious and straightforward, rather than function pointers all over the place. > Might make the code slightly bigger, but hopefully much easier to read. This is still untouched, I can think of two ways to simplify this: 1) Folding the vfuncs into: (* set_boolean_value) (MetaInputSettings, MetaInputBooleanValue, gboolean) (* set_enum_value) (MetaInputSettings, MetaInputEnumValue, guint) (* set_matrix) (MetaInputSettings, gfloat[]) 2) adding some meta_input_settings_bind_setting() helpers, or abusing g_settings_bind_with_mapping(), and calling that from init/constructed in both implementations. I'm not a big fan of this as this makes less evident when a backend lacks something. Or did you have something else specifically in mind? > @@ +520,3 @@ > + if (type == CLUTTER_POINTER_DEVICE) > + { > + update_mouse_left_handed (input_settings, NULL); > > Any reason you don't pass device? > > @@ +530,3 @@ > + update_touchpad_left_handed (input_settings, device); > + update_touchpad_tap_enabled (input_settings, device); > + update_touchpad_scroll_method (input_settings, NULL); > > Any reason you don't pass device? Oops, it was forgotten in these... (In reply to comment #27) > @@ +1238,3 @@ > + > +static inline void > +multiply_matrix (float a[6], > > sheesh. i know we have ten different matrix libs already, but this is just sort > of weird. i don't know if we should use cogl / cairo or just keep this. i don't > really care. feel free to keep this. I'm keeping this in the end... the struct format/packing in these matrix helpers is not the same than what x11/libinput expect, I initially thought of reusing cairo there, but just the fiddling with the cairo_matrix_t fields to put these in the right spot made things more confusing.
Review of attachment 294462 [details] [review]: ::: src/backends/meta-input-settings.c @@ +619,3 @@ +#endif + if (!meta_is_wayland_compositor ()) + return g_object_new (META_TYPE_INPUT_SETTINGS_X11, NULL); Still breaks the build.
Created attachment 294474 [details] [review] backends: Add MetaInputSettings This object internally keeps track of the relevant input configuration, and goes through its vmethods in order to apply the configuration on the backend-specific devices. So far, only mouse/touchpad settings are actually attached to GSettings changes. ::set_matrix(), meant for tablets/touchscreens, is not hooked yet. One caveat is that meta_input_settings_create() may return NULL if the backend does not own the windowing system (wayland nested on X11 being the one case), and thus device settings can't be changed freely.
Created attachment 294475 [details] [review] backends/x11: Implement X11-specific MetaInputSettings This goes through modifying XI2 device properties, either common ones (eg. set on every device) or those specific to the libinput X11 driver. Keyboard repeat/rate are set through core and XKB APIs.
Created attachment 294476 [details] [review] backends/native: Add libinput-based MetaInputSettings implementation The libinput_device is fetched from the ClutterInputDevice, and configured through the libinput_device_*config* API.
Created attachment 294603 [details] [review] backends/native: Add libinput-based MetaInputSettings implementation The libinput_device is fetched from the ClutterInputDevice, and configured through the libinput_device_*config* API.
The series doesn't apply due to ordering issues. Is this in a branch?
I've just pushed to wip/input-devices-config
Attachment 292468 [details] pushed as 049f67d - native: Remove previous listener for keyboard settings Attachment 292469 [details] pushed as 9d73b4e - wayland: Use the new keyboard settings location for repeat settings Attachment 294465 [details] pushed as 1dea181 - input-settings: Handle device-to-output mapping Attachment 294466 [details] pushed as 71c4138 - monitor-manager: Add method to find an output matrix Attachment 294474 [details] pushed as 460e1fd - backends: Add MetaInputSettings Attachment 294475 [details] pushed as 3c06f2d - backends/x11: Implement X11-specific MetaInputSettings Attachment 294603 [details] pushed as 2d878d3 - backends/native: Add libinput-based MetaInputSettings implementation Pushed, thanks.