GNOME Bugzilla – Bug 631502
Port gnome-settings-daemon to gsettings
Last modified: 2012-03-09 15:16:06 UTC
Created attachment 171809 [details] [review] Patch Attached patch migrates several settings (mainly /apps/gnome/gnome_settings_daemon and a few others) to use GSettings instead of GConf, as well as migrates the url-handlers logic to use MIME handlers
Review of attachment 171809 [details] [review]: Schemas file are missing as well. ::: plugins/housekeeping/gsd-disk-space.c @@ +602,3 @@ /* Make sure we dont leave stale entries in ldsm_notified_hash */ + g_hash_table_foreach_remove (ldsm_notified_hash, + ldsm_is_hash_item_in_ignore_paths, NULL); You're leaking the container strv. A g_free(settings_list) should fix it. @@ +627,3 @@ + settings = g_settings_new (SETTINGS_HOUSEKEEPING_DIR); + if (settings != NULL) { g_settings_new() should never fail. ::: plugins/housekeeping/gsd-ldsm-dialog.c @@ +156,1 @@ + settings = g_settings_new (SETTINGS_HOUSEKEEPING_DIR); This looks like the same code as in gsd-disk-space.c Any chance to clean this up, or at least make the code accept strv's instead of GSList? ::: plugins/media-keys/gsd-media-keys-manager.c @@ +157,1 @@ Don't use execute(). Use g_app_info_launch() instead. @@ +160,2 @@ if ((cmd_term != NULL) && (strcmp (cmd_term, "") != 0)) { char *cmd_args; Does this actually need to be a warning? @@ +164,1 @@ if ((cmd_args != NULL) && (strcmp (cmd_term, "") != 0)) { g_settings_get_string never returns NULL. @@ +492,1 @@ + settings = g_settings_new ("org.gnome.desktop.url-handlers.mailto"); Nope, use x-scheme-handler/mailto instead. @@ +535,1 @@ + settings = g_settings_new ("org.gnome.desktop.url-handlers.http"); Handler for x-scheme-handler/http. @@ +690,3 @@ + settings = g_settings_new (SETTINGS_MISC_DIR); + /* FIXME: why is this key under org.gnome.settings-daemon?? */ + vol_step = g_settings_get_int (settings, "volume_step"); Just remove it, and use its default value as a #define. ::: plugins/xrandr/gsd-xrandr-manager.c @@ +60,3 @@ #define GSD_XRANDR_MANAGER_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), GSD_TYPE_XRANDR_MANAGER, GsdXrandrManagerPrivate)) +#define CONF_DIR "org.gnome.settings-daemon.xrandr" You namespaced "org.gnome.settings-daemon.plugins.housekeeping" for the housekeeping plugin. Pick one.
Created attachment 171812 [details] [review] Updated patch (just missing the schemas)
Created attachment 171831 [details] [review] Updated patch, with all owned schemas migrated and more porting
Review of attachment 171831 [details] [review]: Mostly coding style problems, and left-over conditions from GConf. ::: plugins/housekeeping/gsd-disk-space.c @@ +661,3 @@ ldsm_monitor = NULL; + if (settings != NULL) { Can settings be != NULL? ::: plugins/housekeeping/gsd-ldsm-dialog.c @@ +190,2 @@ } else { + g_warning ("Cannot change ignore preference - failed to get settings client"); g_settings_new() cannot fail. ::: plugins/media-keys/gsd-media-keys-manager.c @@ +158,3 @@ + settings = g_settings_new ("org.gnome.desktop.applications.terminal"); + cmd_term = g_settings_get_string (settings, "exec"); + if (strcmp (cmd_term, "") == 0) If prefer: if (cmd_term[0] == '\0') ::: plugins/mouse/gsd-mouse-manager.c @@ +963,3 @@ + GConfClient *client = gconf_client_get_default (); + + if (! strcmp (key, KEY_TOUCHPAD_DISABLE_W_TYPING)) { if (g_str_equal (key, KEY_TOUCHPAD_DISABLE_W_TYPING)) instead. And same of the others. @@ +1107,3 @@ g_return_if_fail (mouse_manager->priv != NULL); + if (mouse_manager->priv->touchpad_settings != NULL) Can it actually be NULL? And you're already unreffing it a couple of lines above. ::: plugins/xrandr/gsd-xrandr-manager.c @@ +2155,3 @@ GsdXrandrManager *manager) { + if (strcmp (key, CONF_KEY_SHOW_NOTIFICATION_ICON) == 0) g_str_equal again. @@ +2386,3 @@ manager); + if (manager->priv->settings != NULL) { Can it be NULL?
Review of attachment 171831 [details] [review]: Fixed issues in last review and committed
Still missing some GConf usage of keys owned by other modules, so keeping this open
Review of attachment 171831 [details] [review]: ::: plugins/mouse/gsd-mouse-manager.c @@ +69,3 @@ +#define KEY_SCROLL_METHOD "scroll_method" +#define KEY_PAD_HORIZ_SCROLL "horiz_scroll_enabled" +#define KEY_TOUCHPAD_ENABLED "touchpad_enabled" Shouldn’t you also replace underscores with dashes in value names being migrated? @@ +328,3 @@ device = device_is_touchpad (device_info[i]); if (device != NULL) { GConfClient *client = gconf_client_get_default (); client is no longer used. @@ +573,2 @@ if (error) { GConfClient *client; And here. ::: plugins/smartcard/gsd-smartcard-plugin.c @@ +57,3 @@ #define SM_LOGOUT_MODE_FORCE 2 +#define KEY_REMOVE_ACTION "removal_action" Again, untranslated underscore.
Also, in commit 07902dd Bastien Nocera has introduced this in data/Makefile.am: ::: data/Makefile.am @@ +1,8 @@ NULL = +gsettings_ENUM_NAMESPACE = org.gnome.desktop +gsettings_ENUM_FILES = gsd-enums.h + On my machine, this causes an org.gnome.desktop.enums.xml file to be generated, which, when installed, overwrites the file with the same name installed by gsettings-desktop-schemas, causing problems further in all schemes that use enums that were defined there. Am I doing something wrong, or should g-s-d use a different namespace?
Created attachment 172106 [details] [review] Port keyboard plugin to GSettings The per-hostname num-lock save needs thought, GSettings doesn't like it when reading from a key that doesn't exist.
Disabled the per-hostname num-lock saving, will file a separate bug about it. Attachment 172106 [details] pushed as 31a9b8c - Port keyboard plugin to GSettings
Sorry, shouldn't have been closed.
(In reply to comment #7) > Review of attachment 171831 [details] [review]: > > ::: plugins/mouse/gsd-mouse-manager.c > @@ +69,3 @@ > +#define KEY_SCROLL_METHOD "scroll_method" > +#define KEY_PAD_HORIZ_SCROLL "horiz_scroll_enabled" > +#define KEY_TOUCHPAD_ENABLED "touchpad_enabled" > > Shouldn’t you also replace underscores with dashes in value names being > migrated? That's been fixed. > @@ +328,3 @@ > device = device_is_touchpad (device_info[i]); > if (device != NULL) { > GConfClient *client = gconf_client_get_default (); > > client is no longer used. Nicely spotted, fixed. > @@ +573,2 @@ > if (error) { > GConfClient *client; > > And here. Fixed. > ::: plugins/smartcard/gsd-smartcard-plugin.c > @@ +57,3 @@ > #define SM_LOGOUT_MODE_FORCE 2 > > +#define KEY_REMOVE_ACTION "removal_action" > > Again, untranslated underscore. Fixed too.
(In reply to comment #8) > Also, in commit 07902dd Bastien Nocera has introduced this in data/Makefile.am: > > ::: data/Makefile.am > @@ +1,8 @@ > NULL = > > +gsettings_ENUM_NAMESPACE = org.gnome.desktop > +gsettings_ENUM_FILES = gsd-enums.h > + > > On my machine, this causes an org.gnome.desktop.enums.xml file to be generated, > which, when installed, overwrites the file with the same name installed by > gsettings-desktop-schemas, causing problems further in all schemes that use > enums that were defined there. Am I doing something wrong, or should g-s-d use > a different namespace? Fixed as well. Most of those settings should actually be in the org.gnome.settings-daemon namespace.
Created attachment 172266 [details] [review] Migrate mouse-a11y The patch migrates the a11y parts of the mouse plugin.
Review of attachment 172266 [details] [review]: It looks ok to me, not sure though about the org.gnome.mousetweaks ID used for g_settings. Shouldn't this be either under org.gnome.settings-daemon.peripherals.mouse or org.gnome.accessibility?
Review of attachment 172266 [details] [review]: The necessary changes to the schemas also seem to be missing. You'd need to add a new schema for those mousetweaks changes, and mousetweaks will need to rely on gnome-settings-daemon being present. ::: plugins/mouse/gsd-mouse-manager.c @@ +56,2 @@ #define GCONF_MOUSE_DIR "/desktop/gnome/peripherals/mouse" +#define MOUSETWEAKS_SCHEMA_ID "org.gnome.mousetweaks" Use org.gnome.settings-daemon.plugins.mouse as the namespace for GSettings please. @@ +63,3 @@ #define KEY_LOCATE_POINTER GCONF_MOUSE_DIR "/locate_pointer" +#define KEY_DWELL_ENABLED "dwell-enabled" +#define KEY_SSC_ENABLED "ssc-enabled" In both the key name and the define, ssc is not very descriptive. @@ +73,3 @@ { GSettings *touchpad_settings; + GSettings *mt_settings; mouse_settings instead.
(In reply to comment #16) > Review of attachment 172266 [details] [review]: > > The necessary changes to the schemas also seem to be missing. > > You'd need to add a new schema for those mousetweaks changes, and mousetweaks > will need to rely on gnome-settings-daemon being present. Thanks for the reviews. Just to clarify, do you mean we should move all the mousetweaks keys into g-s-d or just the 2 listed in the plugin. I'd prefer moving all of them to keep them together.
(In reply to comment #17) > (In reply to comment #16) > > Review of attachment 172266 [details] [review] [details]: > > > > The necessary changes to the schemas also seem to be missing. > > > > You'd need to add a new schema for those mousetweaks changes, and mousetweaks > > will need to rely on gnome-settings-daemon being present. > > Thanks for the reviews. Just to clarify, do you mean we should move all the > mousetweaks keys into g-s-d or just the 2 listed in the plugin. I'd prefer > moving all of them to keep them together. Probably just the ones applied by gnome-settings-daemon, rather than all of them.
Created attachment 172344 [details] [review] Migrate mouse-a11y ver. 2 Updated patch: - schema entries for the 2 keys - some variable renaming & mouse_settings
Comment on attachment 172344 [details] [review] Migrate mouse-a11y ver. 2 Please provide git-formatted patches in the future, it makes it easier for us to maintain authorship that way. commit 091c1295ca148e5dceba6489c500abc43b056770 Author: Gerd Kohlberger <lowfi@chello.at> Date: Thu Oct 14 13:07:41 2010 +0100 mouse: Port a11y settings to GSettings https://bugzilla.gnome.org/show_bug.cgi?id=631502
(In reply to comment #19) > Created an attachment (id=172344) [details] [review] > Migrate mouse-a11y ver. 2 Changes to gnome-settings-daemon.convert were missing from the patch (my fault for not checking). Is "secondary-click-enabled" the same setting as "delay_enable"? If so we should add: secondary-click-enabled = /desktop/gnome/accessibility/mouse/delay_enable to the .convert file.
(In reply to comment #21) > (In reply to comment #19) > > Created an attachment (id=172344) [details] [review] [details] [review] > > Migrate mouse-a11y ver. 2 > > Changes to gnome-settings-daemon.convert were missing from the patch (my fault > for not checking). > > Is "secondary-click-enabled" the same setting as "delay_enable"? > Yes delay-click was the meaningless old name. > If so we should add: > secondary-click-enabled = /desktop/gnome/accessibility/mouse/delay_enable > to the .convert file. Fixed in master: 6e1994446f09471f0cd130882f173eef6b058114
While `make install`ing today’s current version: test -n "" || /usr/bin/glib-compile-schemas /opt/gnome/share/glib-2.0/schemas /opt/gnome/share/glib-2.0/schemas/org.gnome.settings-daemon.plugins.keyboard.gschema.xml: Error on line 4 char 1: <schema id='org.gnome.settings-daemon.plugins.keyboard'> already specified Is glib-compile-schemas complaining that org.gnome.settings-daemon.plugins.keyboard is defined in both org.gnome.settings-daemon.plugins.gschema.xml:75 ('active', 'priority') and org.gnome.settings-daemon.plugins.keyboard.gschema.xml:3 ('disable-indicator', 'duplicate-leds')? I.e. it looks like schemas are not merged from several files. I couldn’t find if this is a deliberate design decision in GSettings or an absence of a feature. (Introduced in 786531ecea46dbbe2fb9bdea25a985db5bf36878 by Sergey V. Udaltsov <svu@gnome.org>)
Riht, Yuri, it should be just in one place, so fixed now in master
Created attachment 172839 [details] [review] xsettings: Port XSettings sync to GSettings
Review of attachment 172839 [details] [review]: For the changes in data/gnome-settings-daemon.convert and org.gnome.settings-daemon.peripherals.gschema.xml.in.in, please note I already added those 2 keys, since they are used in g-c-c mouse, so don't readd them. The rest looks ok
Comment on attachment 172839 [details] [review] xsettings: Port XSettings sync to GSettings Attachment 172839 [details] pushed as 6d74981 - xsettings: Port XSettings sync to GSettings Fixed the duplicate GSettings though
I think this can be closed now
It's not. The keyboard shortcuts plugin still uses GConf.
Still nothing ? In Gnome 2, volume_step (very useful) is handled in /apps/gnome_settings_daemon/volume_step There is no equivalent in Gnome 3.
(In reply to comment #30) > Still nothing ? > In Gnome 2, volume_step (very useful) is handled in > /apps/gnome_settings_daemon/volume_step > There is no equivalent in Gnome 3. It's gone, see bug 650371
Created attachment 201202 [details] [review] media-keys: Port custom keybindings to GSettings
Attachment 201202 [details] pushed as 95b1e9b - media-keys: Port custom keybindings to GSettings