GNOME Bugzilla – Bug 621204
Migrate to GSettings
Last modified: 2011-11-12 14:26:55 UTC
I've started working on porting Metacity to GSettings. We're particularly interested in this for GNOME Shell and Mutter, which depend on Metacity schemas that they sometimes override. Most GConf code is isolated in src/core/prefs.c, which is good. The bad side is that the code is very tricky, with lots of customization that I always have to decide whether to keep or drop. One of the important points is that it would be the right time to rename schemas to org.gnome.wm, in order to move them to an independent package like gnome-wm-data, which Metacity and Mutter would depend on. All of Metacity's settings would go there. I'll attach a patch when enums support has landed in GSettings. Let me know if you have suggestions or requirements.
I'm reading a comment in include/prefs.c, about keybindings: > For historical reasons, the first entry is > * governed by the pref FOO and the remainder are > * governed by the pref FOO_list This means ATM there is /apps/metacity/global_keybindings/cycle_group, with a string describing the stroke. And potentially /apps/metacity/global_keybindings/cycle_group_list with a list of strings with other strokes. Is it really necessary to respect History here? The code is really messy there because of this kind of constraint. It's a good occasion to do some cleanup, even if we lose a few strokes. Simply using *one* string list would be clearer. GSettings doesn't allow us to read a key that is not present in the schemas (it aborts the program). So if we want to respect History, we are required to add all _list keys with empty lists to the schemas. This is very ugly IMHO.
Created attachment 164708 [details] [review] Migrate to GSettings (v1) I'm attaching a rough version of what a patch may look like. It builds and runs fine, I have tested changing a few simple parameters and it works. This requires GLib 2.25.10 and dconf (or GConf 2.31 with the GSettings Backend). Regarding the question from my last comment, I've gone the clean/simple way, where bindings are stored in a single key with a list, instead of two (see patch message). Else we'd need to create two keys for each binding and make the code more messy, which IMO is a bad tradeoff. A few things still need an important cleaning - I've left FIXMEs all over the place. Let alone testing, which will be the hard part. Could a maintainer step in and say what are the requirements for the patch to be eventually committed? Do we need a separate branch to avoid depending on a too recent GLib? Do you prefer me to keep GConf in a separate file with a configure switch? Anyway, if people want to have a look at the general design, you're welcome! Reading the patch might not make sense for src/core/prefs.c where almost everything has changed, could be better to look at the result directly.
Created attachment 164923 [details] [review] Migrate to GSettings (v2) This new patch is reasonably clean and tested to be worth a review. Main change is the use of GSettingsGetMapping functions via g_settings_get_mapped() for complex strings, that allow to check for validity before accepting a value. This is implemented very basically since we cannot validate everything (fonts can be parsed with Pango, but themes aren't checked for existence). New patch also adapts schema-bindings.c to generate the schemas for bindings and commands on the fly. I've also added support for migration from GConf (gsettings-data-convert XML file). We still need a few modules to be ported to GSettings: for now, I've kept dummy schema definitions for Metacity to run correctly. gnome-desktop should move soon, but I don't know when gnome-terminal will. Not really a problem for a development version though, since we can run with our dummy schema until gnome-terminal is ported. I'd like somebody to tell me what are the current style conventions of Metacity, because it's hard to guess given the inconsistencies I've found (i++ or ++i, prototypes...).
Good news: the new gsettings-desktop-schemas module now makes it possible to drop dummy schemas from the patch (I was wrong about requiring gnome-terminal to be ported). Bad news: I've finally understood that keybindings are also accessed by gnome-keybinding-properties via the 50-metacity-*.xml files. g-k-p expect these keys to contain a string, and not a string list - so that's why Metacity has been using this hack with an additional list key. Removing this stuff allowed me to make the code clearer, so I don't think that's really a good idea to reintroduce it. I'd rather add to g-k-p support for string lists: it would be easy to make it work with the first entry in the list, and keep other bindings in the list without touching them. One may also question the interest of having several keybindings for an action at all: the only reason I can see is when many users are using the same deployment, and some may want compatibility bindings while others may want new bindings. Not very likely, though! Anyway, if the patch is kept as-is for bindings, g-k-p has to be changed (anyway it hasn't moved to GSettings yet). Else, the patch could easily be changed to use a single string for bindings, making everything simpler. Migration is also an issue: if nothing changes and we reintroduce the two keys (string + strv), data can be moved from GConf easily. If we choose to keep a single keybinding, and remove the list key, it's also easy. But if we go with the strv-only solution, the key can't be migrated since it was a string before (maybe a hack in gsettings-data-convert could make it a one-item list?). So much for the bad news... But AFAIK everything else is more or less all right! Last side note: enumerations can be handled automatically by GSettings now, generating them from glib-mkenums and adding them to the schemas. But that can be done later, since for now everything works from static enums I added to the schemas, and Metacity doesn't make use of glib-mkenums ATM. Assigning to Owen, since I'm cowardly dropping my responsibility about this on him.
The FOO and FOO_list support really does make things ugly, yes. It was added because of bug 164831.
Thomas: thanks for the pointer! Havoc said in that bug: > I'm not sure of the right approach. You can't just change > the keys to a list because it will break old versions of > GNOME using the same home directory. See forward/back > compat guidelines on http://www.gnome.org/projects/gconf/ I'm inclined to consider we should take advantage of the move to GSettings to fix this weird design. It's easy to fix gnome-control-center to accept string lists in addition to strings, and we get a much cleaner system (and code !). It would be very nice to find a way to do the migration though. The conversion tool doesn't handle this ATM, but we may find a way to run arbitrary scripts. But is that worth it? Having several keybindings for one action is cool, but I guess this feature is mainly used by distributors, rather than customized by users.
Note that this will be needed for gnome-settings-daemon to remove its dependency on GConf (as it sets the metacity theme key when using a11y).
[Removing GNOME3.0 target as decided in release-team meeting on March 03, 2011. This report has an "important" categorisation for GNOME3.0 but is not considered a hard blocker. For querying use the corresponding whiteboard entry added.]
So what is the status for this? Any chance to fix this for GNOME 3.2? Note that this bug is a blocker for other several modules
[Removing whiteboard entry as GNOME Target field is used.]
I don't think this is on the radar for 3.2 - last I talked to florian, this was 3.4 material
Created attachment 200705 [details] [review] Port preferences to GSettings Move preferences to GSettings, using mainly shared schemas from gsettings-desktop-schemas. Unlike GConf, GSettings support is not optional, as Gio is already a hard dependency of GTK+. Based on an initial patch from Milan Bouchet-Valat. I did a bunch of further changes (apart from rebasing + fixing warnings): - move schemas to gsettings-desktop-schemas - update keybinding files to not use conditions (I originally filed bug 653614 for support, but we agreed with the designers that it makes more sense to display a fixed set of bindings) - change how workspace names/commands are handled, to not hardcode limits from the schema - change how the different GSettings objects are stored (not strictly necessary here, but needed by mutter, as we don't use a fixed number of schemas there)
Created attachment 200767 [details] [review] Port preferences to GSettings Updated to keep code in line with the mutter patch in bug 635378.
Created attachment 200797 [details] [review] Port preferences to GSettings Keeping patch in sync with mutter.
Created attachment 201205 [details] [review] Port preferences to GSettings Adjust to schema changes / mutter review.
Created attachment 201242 [details] [review] Port preferences to GSettings Adapt to some more schema changes
Attachment 201242 [details] pushed as a228546 - Port preferences to GSettings The virtually identical mutter patch got approved, so I'm gonna push this as well.
This broke the build: make[4]: Entering directory `/home/kmaraas/src/gnome/metacity/src' CC bell.o In file included from core/bell.c:54:0: ./include/prefs.h:80:1: error: unknown type name ‘GDesktopFocusMode’ ./include/prefs.h:81:1: error: unknown type name ‘GDesktopFocusNewWindows’ ./include/prefs.h:104:1: error: unknown type name ‘GDesktopTitlebarAction’ ./include/prefs.h:105:1: error: unknown type name ‘GDesktopTitlebarAction’ ./include/prefs.h:106:1: error: unknown type name ‘GDesktopTitlebarAction’ ./include/prefs.h:205:1: error: unknown type name ‘GDesktopVisualBellType’ core/bell.c: In function ‘bell_flash_screen’: core/bell.c:134:39: error: ‘G_DESKTOP_FOCUS_MODE_CLICK’ undeclared (first use in this function) core/bell.c:134:39: note: each undeclared identifier is reported only once for each function it appears in core/bell.c: In function ‘bell_visual_notify’: core/bell.c:268:10: error: ‘G_DESKTOP_VISUAL_BELL_FULLSCREEN_FLASH’ undeclared (first use in this function) core/bell.c:271:10: error: ‘G_DESKTOP_VISUAL_BELL_FRAME_FLASH’ undeclared (first use in this function) core/bell.c:274:10: error: ‘G_DESKTOP_VISUAL_BELL_NONE’ undeclared (first use in this function) make[4]: *** [bell.o] Error 1 make[4]: Leaving directory `/home/kmaraas/src/gnome/metacity/src' make[3]: *** [all-recursive] Error 1 make[3]: Leaving directory `/home/kmaraas/src/gnome/metacity/src' make[2]: *** [all] Error 2 make[2]: Leaving directory `/home/kmaraas/src/gnome/metacity/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/kmaraas/src/gnome/metacity' make: *** [all] Error 2 *** Error during phase build of metacity: ########## Error running make *** [77/178]
You need a recent gsettings-desktop-schemas.
See http://git.gnome.org/browse/jhbuild/commit/?id=5d6e77f808d3dd43c
Shouldnt configure check for that? Could you add such a check?
(In reply to comment #21) > Shouldnt configure check for that? Could you add such a check? Sure - there already is a check for gsettings-desktop-schemas, it just does not include a version. Doubt it'll be of much help though, gsettings-desktop-schemas from master still reports version 3.2.0 ...
Blergh, I guess pre-release version increment? Can we change that? :) Distributions generally like the "damn build failure" then 2 min later: "oh, guess it is time to update buildrequires" :P
Created attachment 201289 [details] [review] build: Specify a minimum version for gsettings-desktop-schemas (In reply to comment #23) > Blergh, I guess pre-release version increment? Can we change that? :) I've requested a version bump in bug 663921
Attachment 201289 [details] pushed as c074d0f - build: Specify a minimum version for gsettings-desktop-schemas Version has been bumped, pushing