GNOME Bugzilla – Bug 742593
Drop input settings managed by mutter
Last modified: 2015-01-21 12:12:44 UTC
In sync with bug #739397, g-s-d should drop the settings that would be managed by mutter. I'm attached some patches with scattered changes to do so.
Created attachment 294092 [details] [review] common: Add GsdDeviceManager This is a GUdev-based object that keeps track of the plugged in input devices. Meant to be used across g-s-d wherever some knowledge about these must remain. On X11, some API is still offered to access the gdk/X devices (which may not map 1:1 with the udev devices). For both platforms, the relevant /org/gnome/desktop/peripherals/... settings can be accessed, in order to interoperate with the settings that mutter applies on libinput devices.
Created attachment 294093 [details] [review] power: Restrict disabling of touchscreens/tablets to X11 This should be eventually done by the compositor. In the mean time, restrict it to X11, and remove the GsdDeviceMapper usage, as it's not strictly necessary.
Created attachment 294094 [details] [review] common: Make GsdDeviceMapper use devices from GsdDeviceManager This has brought some changes in the xrandr and wacom modules, which now use GsdDevice for device mapping. The use of GdkDevices and XInput properties remain for these settings not yet in the gsettings-desktop-schemas peripherals settings in the wacom case, and those bits have been made to run only on X11 as expected.
Created attachment 294095 [details] [review] wacom: Remove some dead code from GsdWacomDevice The direct usage of GsdDeviceMapper allows us to remove some code here.
Created attachment 294096 [details] [review] schemas: Remove "display" setting from wacom devices' schema This is part now of org.gnome.desktop.peripherals.[tablet|touchscreen], in use now by g-s-d.
Created attachment 294097 [details] [review] common: Add xdevice_is_libinput() This function pokes a libinput-driver-specific property, and can be used to find out whether a device uses the libinput driver or not. This will be used to determine whether a device is to be managed by mutter, or by g-s-d itself.
Created attachment 294098 [details] [review] keyboard: Remove handling of keyboard repeat settings This is managed by mutter now on X11 too, so no need to do it here again.
Created attachment 294099 [details] [review] keyboard: Apply numlock/bell settings on X11 only. These are not a windowing/desktop thing on wayland, so just ignore these there at the moment.
Created attachment 294100 [details] [review] mouse: Use gsettings-desktop-schemas mouse/touchpad settings Most settings have been moved there, so make this plugin use the new schemas for the handled settings, and remain using the settings-daemon schema for settings that can just be honored on X11. In order to simplify the situation for the future, the "middle-button-enabled", "horiz-scroll-enabled" and "disable-while-typing" settings have been removed, and the sane default is applied on non-libinput devices on X11. For all other settings, the mouse plugin has been made to ignore devices created through the libinput driver, as mutter handles these. Additionally, the entire plugin is disabled on Wayland, the settings remaining in g-s-d schemas are not a thing there.
(In reply to comment #1) > Created an attachment (id=294092) [details] [review] > common: Add GsdDeviceManager > > This is a GUdev-based object that keeps track of the plugged in input > devices. Meant to be used across g-s-d wherever some knowledge about these > must remain. > > On X11, some API is still offered to access the gdk/X devices (which may not > map 1:1 with the udev devices). > > For both platforms, the relevant /org/gnome/desktop/peripherals/... settings > can be accessed, in order to interoperate with the settings that mutter > applies on libinput devices. I should note that, for correct device mapping, input device size calculation in this patch softly depends on a systemd patch that's still not applied: http://lists.freedesktop.org/archives/systemd-devel/2014-December/026491.html , it went already through several reviews, I'll ping about it again.
Review of attachment 294092 [details] [review]: That code will completely fail to work properly on non-Linux targets. ::: plugins/mouse/gsd-mouse-manager.c @@ +904,2 @@ if (error) { + g_settings_set_boolean (manager->priv->gsd_mouse_settings, KEY_LOCATE_POINTER, FALSE); That seems to depend on a separate patch. Where is that patch?
Review of attachment 294093 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +925,3 @@ GList *devices, *l, *to_change; + if (gnome_settings_is_wayland ()) Sure. But please file a bug against gnome-shell for it, and add a reference in the code.
Review of attachment 294094 [details] [review]: ::: plugins/common/gsd-device-mapper.c @@ -46,3 @@ GSD_INPUT_IS_TOUCH = 1 << 2, /* touch device, either touchscreen or tablet */ - GSD_INPUT_IS_PEN = 1 << 3, /* pen device, either touchscreen or tablet */ - GSD_INPUT_IS_ERASER = 1 << 4, /* eraser device, either touchscreen or tablet */ Why remove the eraser?
Review of attachment 294095 [details] [review]: Fine by me.
Review of attachment 294096 [details] [review]: Will be need code in g-c-c for this as well?
Review of attachment 294097 [details] [review]: Sure.
Review of attachment 294098 [details] [review]: Can you please add a reference to the mutter patch for this in the commit message? mutter's version works for both evdev/libinput X11, and wayland?
Review of attachment 294099 [details] [review]: > These are not a windowing/desktop thing on wayland How does wayland/mutter handle num-lock LEDs? Is there something else available to remember those settings?
Review of attachment 294100 [details] [review]: > In order to simplify the situation for the future, the > "middle-button-enabled", "horiz-scroll-enabled" and "disable-while-typing" > settings have been removed, and the sane default is applied on non-libinput > devices on X11. disable-while-typing is useless when libinput is used, but it's necessary on older X11 evdev platforms. As it's a regression in functionality, the removal should be done separately from this commit. middle-button-enabled and horiz-scroll-enabled should have equivalents in libinput I would think.
I'll add for people reading this bug that the X11 evdev fallback code is only for *BSDs. There's no interest from my side to keep the fallback code longer term on Linux.
(In reply to comment #11) > Review of attachment 294092 [details] [review]: > > That code will completely fail to work properly on non-Linux targets. On X11, the object actually tracks addition/removal of devices through GdkDeviceManager signals, rather than udev events. I'll split that properly and make the x11 backend to poke XI properties, so udev is just used on wayland. > > ::: plugins/mouse/gsd-mouse-manager.c > @@ +904,2 @@ > if (error) { > + g_settings_set_boolean > (manager->priv->gsd_mouse_settings, KEY_LOCATE_POINTER, FALSE); > > That seems to depend on a separate patch. Where is that patch? Oops, this looks like a fixup mishap, I'll update the relevant patches (In reply to comment #12) > Review of attachment 294093 [details] [review]: > > ::: plugins/power/gsd-power-manager.c > @@ +925,3 @@ > GList *devices, *l, *to_change; > > + if (gnome_settings_is_wayland ()) > > Sure. But please file a bug against gnome-shell for it, and add a reference in > the code. Sure, I filed bug #742598 at the moment (In reply to comment #13) > Review of attachment 294094 [details] [review]: > > ::: plugins/common/gsd-device-mapper.c > @@ -46,3 @@ > GSD_INPUT_IS_TOUCH = 1 << 2, /* touch device, either > touchscreen or tablet */ > - GSD_INPUT_IS_PEN = 1 << 3, /* pen device, either touchscreen > or tablet */ > - GSD_INPUT_IS_ERASER = 1 << 4, /* eraser device, either > touchscreen or tablet */ > > Why remove the eraser? It seemed to me the most consistent way forward, the pen/eraser division only happens on X11, both are a single event* node on udev, and all we care for here is actually the GSettings path/schema (with both pen and eraser settings), so I got these merged on X11 .(In reply to comment #15) > Review of attachment 294096 [details] [review]: > > Will be need code in g-c-c for this as well? Yeah right, that's all still missing, not that the wacom panel works yet at all on wayland... but same applies for the mouse settings relocation, etc (In reply to comment #17) > Review of attachment 294098 [details] [review]: > > Can you please add a reference to the mutter patch for this in the commit > message? Sure, will do. > > mutter's version works for both evdev/libinput X11, and wayland? Yup, keyboard settings on X11 are driver independent, and on the native backend this is backed by clutter_evdev_set_keyboard_repeat() (In reply to comment #18) > Review of attachment 294099 [details] [review]: > > > These are not a windowing/desktop thing on wayland > > How does wayland/mutter handle num-lock LEDs? Is there something else available > to remember those settings? There's not much about that yet... Clutter's evdev backend handles led state, but offers no info/setters about these. (In reply to comment #19) > Review of attachment 294100 [details] [review]: > > > In order to simplify the situation for the future, the > > "middle-button-enabled", "horiz-scroll-enabled" and "disable-while-typing" > > settings have been removed, and the sane default is applied on non-libinput > > devices on X11. > > disable-while-typing is useless when libinput is used, but it's necessary on > older X11 evdev platforms. As it's a regression in functionality, the removal > should be done separately from this commit. Hmm, only the settings are removed. disable-while-typing is enabled by default (all but middle-button are), and syndaemon is still launched. The functionality should be present, and always-enabled for synaptics devices, libinput devices will be ignoring syndaemon obviously :). > > middle-button-enabled and horiz-scroll-enabled should have equivalents in > libinput I would think. Yeah, I agree (more about middle-button emulation than horiz scroll though :)
Created attachment 294467 [details] [review] common: Add GsdDeviceManager This is a GUdev-based object that keeps track of the plugged in input devices. Meant to be used across g-s-d wherever some knowledge about these must remain. On X11, some API is still offered to access the gdk/X devices (which may not map 1:1 with the udev devices). For both platforms, the relevant /org/gnome/desktop/peripherals/... settings can be accessed, in order to interoperate with the settings that mutter applies on libinput devices.
Created attachment 294468 [details] [review] common: Add udev-based GsdDeviceManager implementation This will be used on wayland environments in order to figure out the plugged devices and their capabilities.
Created attachment 294469 [details] [review] common: Add X11-based GsdDeviceManager implementation This will be used on X11 environments, poking device capabilities through device properties. This will be the default backend on X11.
Created attachment 294470 [details] [review] power: Restrict disabling of touchscreens/tablets to X11 This should be eventually done by the compositor. In the mean time, restrict it to X11, and remove the GsdDeviceMapper usage, as it's not strictly necessary.
Created attachment 294471 [details] [review] keyboard: Remove handling of keyboard repeat settings With patches from bug #739397 applied. This is managed by mutter on X11 too, so no need to do it here again.
Created attachment 294472 [details] [review] mouse: Use gsettings-desktop-schemas mouse/touchpad settings Most settings have been moved there, so make this plugin use the new schemas for the handled settings, and remain using the settings-daemon schema for settings that can just be honored on X11. In order to simplify the situation for the future, the "middle-button-enabled", "horiz-scroll-enabled" and "disable-while-typing" settings have been removed, and the sane default is applied on non-libinput devices on X11. For all other settings, the mouse plugin has been made to ignore devices created through the libinput driver, as mutter handles these. Additionally, the entire plugin is disabled on Wayland, the settings remaining in g-s-d schemas are not a thing there.
Created attachment 294612 [details] [review] keyboard: Remove handling of keyboard repeat settings With patches from bug #739397 applied. This is managed by mutter on X11 too, so no need to do it here again.
Created attachment 294613 [details] [review] mouse: Use gsettings-desktop-schemas mouse/touchpad settings Most settings have been moved there, so make this plugin use the new schemas for the handled settings, and remain using the settings-daemon schema for settings that can just be honored on X11. In order to simplify the situation for the future, the management of "middle-button-enabled", "horiz-scroll-enabled" and "disable-while-typing" settings have been removed, and the sane default is applied on non-libinput devices on X11. For all other settings, the mouse plugin has been made to ignore devices created through the libinput driver, as mutter handles these. Additionally, the entire plugin is disabled on Wayland, the settings remaining in g-s-d schemas are not a thing there.
Created attachment 294614 [details] [review] common: Migrate input settings to gsettings-desktop-schemas keys The deprecated keys are kept in isolated ".deprecated" suffixed schemas. On plugin startup, the settings-daemon paths are opened with these legacy schemas, and all user-modified keys are dumped to the new location and reset. This ensures the transition just happens once per-key, and ensures the dconf database is left clean.
As the ordering of patches is a bit messed up here, I've resuscited wip/settings-relocation containing all patches to this point
Created attachment 294617 [details] [review] common: Migrate input settings to gsettings-desktop-schemas keys The deprecated keys are kept in isolated ".deprecated" suffixed schemas. On plugin startup, the settings-daemon paths are opened with these legacy schemas, and all user-modified keys are dumped to the new location and reset. This ensures the transition just happens once per-key, and ensures the dconf database is left clean.
Review of attachment 294467 [details] [review]: otherwise looks good ::: plugins/common/gsd-device-manager.c @@ +152,3 @@ + g_free (priv->name); + g_free (priv->vendor_id); + g_free (priv->product_id); doesn't free ->device_file @@ +263,3 @@ + + if (!manager) { + } return NULL ? ok, I see that this is fleshed in the backend patches @@ +296,3 @@ + GsdDevicePrivate *priv; + + g_return_val_if_fail (GSD_IS_DEVICE (device), FALSE); method returns void
Review of attachment 294468 [details] [review]: ok ::: plugins/common/gsd-device-manager-udev.c @@ +214,3 @@ + while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &device)) { + device_type = gsd_device_get_device_type (device); + extra line @@ +222,3 @@ + gsd_device_get_device_ids (device, &v, &p); + + g_print (" DEV: %s %d | %s %s | %d %d\n", should be g_debug() now
Review of attachment 294469 [details] [review]: looks fine otherwise ::: plugins/common/gsd-device-manager-x11.c @@ +80,3 @@ + id = gdk_x11_device_get_id (gdk_device); + + device_file = xdevice_get_device_node (id); these 2 steps are already done in add_device and device_file should be ready to use here
Review of attachment 294470 [details] [review]: fine
Review of attachment 294094 [details] [review]: ::: plugins/wacom/gsd-wacom-manager.c @@ +1147,3 @@ + gsd_device); + + devices = gsd_device_get_gdk_devices (gsd_device, &n_gdk_devices); The patch here doesn't correspond to what's in wip/settings-relocation but this part should be skipped on wayland right?
Review of attachment 294099 [details] [review]: yeah, this is fine for now
Review of attachment 294612 [details] [review]: ++
Review of attachment 294613 [details] [review]: otherwise looks fine ::: plugins/mouse/gsd-mouse-manager.c @@ +1403,3 @@ ensure_touchpad_active (manager); + if (get_touchpad_enabled (manager)) { The code inside this if() is now done in ensure_touchpad_active() so this can go away
Review of attachment 294617 [details] [review]: neat :-)
Attachment 294094 [details] pushed as 9e17dce - common: Make GsdDeviceMapper use devices from GsdDeviceManager Attachment 294095 [details] pushed as f4b8633 - wacom: Remove some dead code from GsdWacomDevice Attachment 294096 [details] pushed as f867268 - schemas: Remove "display" setting from wacom devices' schema Attachment 294097 [details] pushed as 8cc7a76 - common: Add xdevice_is_libinput() Attachment 294099 [details] pushed as 2afd86a - keyboard: Apply numlock/bell settings on X11 only. Attachment 294467 [details] pushed as 0d1a455 - common: Add GsdDeviceManager Attachment 294468 [details] pushed as 9ccd9b6 - common: Add udev-based GsdDeviceManager implementation Attachment 294469 [details] pushed as a8f097c - common: Add X11-based GsdDeviceManager implementation Attachment 294470 [details] pushed as cc18139 - power: Restrict disabling of touchscreens/tablets to X11 Attachment 294612 [details] pushed as 1d88e72 - keyboard: Remove handling of keyboard repeat settings Attachment 294613 [details] pushed as ca754de - mouse: Use gsettings-desktop-schemas mouse/touchpad settings Attachment 294617 [details] pushed as f8ccbdf - common: Migrate input settings to gsettings-desktop-schemas keys
Attachment 294468 [details] fails when gudev is disabled. gsd-device-manager-udev.c:25:10: fatal error: 'gudev/gudev.h' file not found #include <gudev/gudev.h> ^ 1 error generated.
Created attachment 295078 [details] [review] build: Fix build problem when gudev and wacom are not available
Review of attachment 295078 [details] [review]: Thanks for spotting! I'd swear I put these files in a conditional... ::: plugins/common/gsd-device-mapper.c @@ +688,3 @@ +#else + info->capabilities = 0; +#endif /* HAVE_WACOM */ This is not good though, GsdDevice is meant to be present and functional on lack of udev (on X11 at least). Moving the #else branch here will leave capabilities unset if libwacom is not present. This will break mapping of touchscreens and tablet devices that are found out through the GsdDevice type. Plus, I don't see this fixing any compile or runtime errors. Please update the patch to contain only the Makefile.am change, and feel free to push that one.
The Makefile.am change was pushed as c5daf94 - build: Fix build problem when gudev is not available
Created attachment 295084 [details] [review] build: Fix build problem when wacom is not available I updated the gsd-device-mapper.c change because it does cause compilation error. The `type' variable is not declared when wacom is disabled.
Comment on attachment 295084 [details] [review] build: Fix build problem when wacom is not available Oh indeed. Doing this is much better. Thanks!
Attachment 295084 [details] pushed as 95a0e0e - build: Fix build problem when wacom is not available