After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 617917 - Migrate to GSettings
Migrate to GSettings
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Owen Taylor
gnome-shell-maint
Depends on: 621204 635379
Blocks: 622558
 
 
Reported: 2010-05-06 15:00 UTC by Milan Bouchet-Valat
Modified: 2012-06-08 17:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Migrate to GSettings (34.76 KB, patch)
2010-05-06 15:01 UTC, Milan Bouchet-Valat
rejected Details | Review
Migrate to GSettings (35.55 KB, patch)
2010-06-01 19:17 UTC, Milan Bouchet-Valat
rejected Details | Review
Migrate to GSettings (35.62 KB, patch)
2010-06-01 19:17 UTC, Milan Bouchet-Valat
none Details | Review
Migrate to GSettings (55.72 KB, patch)
2010-06-03 10:58 UTC, Milan Bouchet-Valat
none Details | Review
Migrate to GSettings (79.64 KB, patch)
2010-06-03 11:19 UTC, Milan Bouchet-Valat
none Details | Review
Migrate to GSettings (83.46 KB, patch)
2010-06-03 13:38 UTC, Milan Bouchet-Valat
none Details | Review
Migrate to GSettings (83.62 KB, patch)
2010-06-03 13:58 UTC, Milan Bouchet-Valat
needs-work Details | Review
Migrate to GSettings (85.01 KB, patch)
2010-06-05 09:45 UTC, Milan Bouchet-Valat
none Details | Review
Migrate to GSettings (86.16 KB, patch)
2010-06-06 19:38 UTC, Milan Bouchet-Valat
none Details | Review
Migrate to GSettings (86.20 KB, patch)
2010-06-16 18:15 UTC, Milan Bouchet-Valat
reviewed Details | Review

Description Milan Bouchet-Valat 2010-05-06 15:00:00 UTC
Use GSettings for all Shell configuration. GConf is kept to read configuration from external programs (Metacity and Nautilus) until they have moved to GSettings too.

GLib 2.25 is required, so we also pull GTK+ in jhbuild to avoid build errors. Schemas are converted to XML GSettings format. To get them detected when running the Shell, we set the GSETTINGS_SCHEMA_DIR environment variable on start. GObject introspection also requires running the Shell at make time, which forces us to compile the schemas to data/, and use it at this point.
Comment 1 Milan Bouchet-Valat 2010-05-06 15:01:01 UTC
Created attachment 160445 [details] [review]
Migrate to GSettings

Use GSettings for all Shell configuration. GConf is kept to read configuration from external programs (Metacity and Nautilus) until they have moved to GSettings too.

GLib 2.25 is required, so we also pull GTK+ in jhbuild to avoid build errors. Schemas are converted to XML GSettings format. To get them detected when running the Shell, we set the GSETTINGS_SCHEMA_DIR environment variable on start. GObject introspection also requires running the Shell at make time, which forces us to compile the schemas to data/, and use it at this point.
Comment 2 Milan Bouchet-Valat 2010-05-06 15:06:22 UTC
This is a first attempt to move to GSettings. Everything seems to work fine, except that I'm not sure how to set an environment variable for the introspection command in src/Makefile.am. So for now 'make' should fail unless you set
export GSETTINGS_SCHEMA_DIR=data/
from the top src dir.

I'm also not sure, when using g_settings_get_strv(), whether it's nicer to do
> strings = g_settings_get_strv("key")[0];
or
> let [strings, len] = g_settings_get_strv("key");
or
> let len;
> [strings, len] = g_settings_get_strv("key"); (when string is a global variable)

Since len is rarely useful in JS.


More generally, moving to GSettings is painful because it pulls GLib from jhbuild, and thus GTK+ to avoid build issues. Window themes aren't in line with the system's, which is kina ugly...
Comment 3 Dan Winship 2010-05-06 15:25:40 UTC
(In reply to comment #2)
> > [strings, len] = g_settings_get_strv("key"); (when string is a global variable)
> 
> Since len is rarely useful in JS.

g_settings_get_strv needs g-i annotations. There are probably other parts of gsettings that need annotation as well.

> More generally, moving to GSettings is painful because it pulls GLib from
> jhbuild, and thus GTK+ to avoid build issues. Window themes aren't in line with
> the system's, which is kina ugly...

that can't be fixed with some environment variable?

at any rate, we're presumably going to have our own mutter theme soon anyway too
Comment 4 Milan Bouchet-Valat 2010-06-01 19:17:04 UTC
Created attachment 162492 [details] [review]
Migrate to GSettings

Use GSettings for all Shell configuration. GConf is kept to read configuration from external programs (Metacity and Nautilus) until they have moved to GSettings too. ShellGConf is removed because it's mostly useless for the few calls we still have.

GLib 2.25 is required. Schemas are converted to the new XML format.
Comment 5 Milan Bouchet-Valat 2010-06-01 19:17:45 UTC
Created attachment 162493 [details] [review]
Migrate to GSettings

Use GSettings for all Shell configuration. GConf is kept to read configuration from external programs (Metacity and Nautilus) until they have moved to GSettings too. ShellGConf is removed because it's mostly useless for the few calls we still have.

GLib 2.25 is required. Schemas are converted to the new XML format.
Comment 6 Milan Bouchet-Valat 2010-06-01 19:22:57 UTC
Here's a new version of the patch, which fixes most issues thanks to changes in the Shell and GLib. Introspection no longer forces us to install schemas at build time. GTK+ isn't built from jhbuild, and it seems to work fine without it.

Only problems left:
- Currently, I have to run the Shell with 'jhbuild run src/gnome-shell' for it to detect schemas. I guess we should set some environment variable, need to check which one.
- We still have the ugly g_settings_get_strv() returning a char** plus the length of the array, useless in JS. So we may have to wait for annotations to be added (but updating the patch all the time is annoying...).
Comment 7 Milan Bouchet-Valat 2010-06-03 10:58:57 UTC
Created attachment 162634 [details] [review]
Migrate to GSettings

Use GSettings for all Shell configuration. GConf is kept to read
configuration from external programs (Metacity and Nautilus)
until they have moved to GSettings too. ShellGConf is removed
because it's mostly useless for the few calls we still have.

GLib 2.25.8 is required. Schemas are converted to the new XML format,
and compiled at build time in data/ so that the Shell can be run from
the source tree. This also requires setting the GSETTINGS_SCHEMA_DIR
environment variable both when running installed or from source tree.
Comment 8 Milan Bouchet-Valat 2010-06-03 11:06:23 UTC
So here's a pretty solid version of the patch. Changes are:
- No need to run the shell from jhbuild to work, thanks to the GSETTINGS_SCHEMA_DIR variable it runs both from source tree and intalled prefix

- Update to new GSettings API for _strv() functions (bug 620312), getting rid of ugly length return values


So I think it's ready for review. I'm currently running with this patch and no problem. Note that this depends on unreleased GSettings API (GLib 2.25.8), but since we use jhbuild it's not really an issue.

One point I'd like to discuss is whether we need shell_global_get_settings(). I used it in ShellAppUsage to get the global GSettings object, but JS uses the GObject property instead of the accessor I guess. So we may either:
- use g_object_get() and remove the accessor (used 3 times in shell-app-usage.c)
- or use a specific GSettings object for ShellAppUsage, with a child schema; there's only one key, so that's not really needed, but that's not really costly either.
Comment 9 Milan Bouchet-Valat 2010-06-03 11:19:35 UTC
Created attachment 162637 [details] [review]
Migrate to GSettings

Silly me - I had forgotten to actually remove shell-gconf.[ch]. Hopefully, this patch is the right one. ;-)
Comment 10 Florian Müllner 2010-06-03 11:55:04 UTC
(In reply to comment #9)
> Created an attachment (id=162637) [details] [review]
> Migrate to GSettings
> 
> Silly me - I had forgotten to actually remove shell-gconf.[ch]. Hopefully, this
> patch is the right one. ;-)

You missed js/prefs/clockPreferences.js ;)
Comment 11 Milan Bouchet-Valat 2010-06-03 13:38:01 UTC
Created attachment 162650 [details] [review]
Migrate to GSettings

Indeed. Since it was not using ShellGConf I thought it was using desktop-wide settings when passing quickly on the file. I discovered I had to add to GSETTINGS_SCHEMA_DIR hack to src/gnome-shell-clock-preferences.in too. Not making my life easier! ;-)

BTW I also removed schemas for the sidebar, which were left by mistake I guess. And added the org.gnome.shell.gschema.xml and gschema.compile to CLEANFILES.


Another point to discuss: as-is, the patch does not allow settings to be stored anywhere, there's just a dummy backend. :-) We have to choose whether we install dconf from jhbuild (should be easy) or set the environment variable to use GConf as GSettings backend.

And yet another one: is /desktop/gnome/shell a good path, or do you prefer /apps/gnome-shell? I guess the latter makes more sense. Anyway, it would be cool to get this question sorted out for all modules, migration is a good occasion to clean this.
Comment 12 Milan Bouchet-Valat 2010-06-03 13:58:12 UTC
Created attachment 162652 [details] [review]
Migrate to GSettings

Rebase against master, last commit conflicted. This is going to be a nightmare, every new commit touches a file included in the patch! :-p
Comment 13 Owen Taylor 2010-06-03 20:52:27 UTC
Review of attachment 162652 [details] [review]:

Nice work here! - I have only minor quibbles with the code below.

The one problem I see here is that the Metacity key overrides (like /schemas/desktop/gnome/shell/windows/button_layout) will no longer work with this setup, since we no longer install GConf schemas with the correct default values.

So, either we need to:

 - Get Metacity and Mutter converted over to GSettings (Mutter shares schemas with Metacity, so both pretty much have to be done as a unit. (Or we could take this opportunity to create the hypothetical gnome-wm-data with schemas and themes)
 - Keep installing a skeleton gnome-shell.schemas with these keys

Your choice, though the latter is obviously a lot easier in the short term.

::: data/Makefile.am
@@ +53,3 @@
+# We need to compile schemas at make time
+# to run from source tree
+all:

Should use all-local: here not all:. I'd also do an extra indirection
local-compile-schemas:
       <rule>

all-local: local-compile-schemas

.PHONY: local-compile-schemas

@@ +54,3 @@
+# to run from source tree
+all:
+	$(bindir)/glib-compile-schemas --targetdir=. .

should be --targetdir=. $(srcdir)

@@ +66,3 @@
+	$(desktop_DATA)					\
+	$(gsettings_SCHEMAS)				\
+	gschemas.compiled

gschemas.compiled will need to be added to .gitignore as well

::: data/org.gnome.shell.gschema.xml.in
@@ +14,3 @@
+      <default>true</default>
+      <_summary>Whether to collect stats about applications usage</_summary>
+      <_description>The shell normally monitors active applications in order to present the most used ones (e.g. in launchers). While this data will be kept private, you may want to disable this for privacy reasons. Please note that doing so won't remove already saved data.</_description>

The descriptions in this file need to be lined-wrapped to reasonable width.

@@ +28,3 @@
+      <default>'single'</default>
+      <_summary>Overview workspace view mode</_summary>
+      <_description>The selected workspace view mode in the overview. Supported values are "single" and "grid".</_description>

Looks like this should be using <choices><choice value="..."/>...</choices> Not quite sure how that works but I see it in the XML schema (http://library.gnome.org/devel/gio/unstable/GSettings.html)

::: js/ui/appFavorites.js
@@ +29,2 @@
     _reload: function() {
+        let ids = global.settings.get_strv(this.FAVORITE_APPS_KEY, -1);

Stray -1 here

::: js/ui/lookingGlass.js
@@ +587,3 @@
+        gconf.add_dir('/desktop/gnome/interface', GConf.ClientPreloadType.PRELOAD_NONE);
+        gconf.add_notify('/desktop/gnome/interface/monospace_font_name',
+                         Lang.bind(this, this._updateFont), null);

The trailing , null looks extraneous

::: js/ui/panel.js
@@ +846,3 @@
         this._calendarPopup = null;
 
+        this._clock_settings = new Gio.Settings({ schema: 'org.gnome.shell.clock' });

_clockSettings not _clock_settings

::: js/ui/runDialog.js
@@ +184,1 @@
+        history = global.settings.get_strv(HISTORY_KEY, -1);

Stray -1

::: src/Makefile.am
@@ +17,3 @@
 	    -e "s|@libexecdir[@]|$(libexecdir)|" \
 	    -e "s|@libdir[@]|$(libdir)|" \
+	    -e "s|@datadir[@]|$(datadir)|" \

Please keep this list alphabetical

::: src/shell-global.c
@@ +277,3 @@
+                                   PROP_SETTINGS,
+                                   g_param_spec_object ("settings",
+                                                        "GSettings instance",

This should just be 'Settings' - this just the "human readable" form of the machine-readable name.
Comment 14 Milan Bouchet-Valat 2010-06-04 10:12:24 UTC
(In reply to comment #13)
> Review of attachment 162652 [details] [review]:
> 
> Nice work here! - I have only minor quibbles with the code below.
> 
> The one problem I see here is that the Metacity key overrides (like
> /schemas/desktop/gnome/shell/windows/button_layout) will no longer work with
> this setup, since we no longer install GConf schemas with the correct default
> values.
> 
> So, either we need to:
> 
>  - Get Metacity and Mutter converted over to GSettings (Mutter shares schemas
> with Metacity, so both pretty much have to be done as a unit. (Or we could take
> this opportunity to create the hypothetical gnome-wm-data with schemas and
> themes)
>  - Keep installing a skeleton gnome-shell.schemas with these keys
> 
> Your choice, though the latter is obviously a lot easier in the short term.
Ah, I had overlooked this part. I've started looking at Mutter, and fortunately GConf isonly used in one file, which makes migration easier, plus we can hope patching Metacity at the same time. The bad side is that the code uses many enums, whose support is still missing in GSettings.

I'll ask Ryan for details, but I think they plan to have something more robust than storing ints, and convenience functions for that. If it turns out this support is not coming soon, then solution 2) is the best one.

> ::: data/Makefile.am
> @@ +53,3 @@
> +# We need to compile schemas at make time
> +# to run from source tree
> +all:
> 
> Should use all-local: here not all:. I'd also do an extra indirection
> local-compile-schemas:
>        <rule>
> 
> all-local: local-compile-schemas
> 
> .PHONY: local-compile-schemas
No idea, you're reaching my limits WRT Automake here, but OK. :-)

> @@ +54,3 @@
> +# to run from source tree
> +all:
> +    $(bindir)/glib-compile-schemas --targetdir=. .
> 
> should be --targetdir=. $(srcdir)
We're compiling schemas to data/, so why do you want to change this?

> @@ +66,3 @@
> +    $(desktop_DATA)                    \
> +    $(gsettings_SCHEMAS)                \
> +    gschemas.compiled
> 
> gschemas.compiled will need to be added to .gitignore as well
Agreed, together with org.gnome.shellgschema.xml and org.gnome.shell.gschema.valid.

> ::: data/org.gnome.shell.gschema.xml.in
> @@ +14,3 @@
> +      <default>true</default>
> +      <_summary>Whether to collect stats about applications usage</_summary>
> +      <_description>The shell normally monitors active applications in order
> to present the most used ones (e.g. in launchers). While this data will be kept
> private, you may want to disable this for privacy reasons. Please note that
> doing so won't remove already saved data.</_description>
> 
> The descriptions in this file need to be lined-wrapped to reasonable width.
I hate doing this manually because this means the line length is fixed instead of the UI wrapping at a correct width. Are you sure that's The Right Way? What use case do you have in mind?

> @@ +28,3 @@
> +      <default>'single'</default>
> +      <_summary>Overview workspace view mode</_summary>
> +      <_description>The selected workspace view mode in the overview.
> Supported values are "single" and "grid".</_description>
> 
> Looks like this should be using <choices><choice value="..."/>...</choices> Not
> quite sure how that works but I see it in the XML schema
> (http://library.gnome.org/devel/gio/unstable/GSettings.html)
Hm, yes. This comes from automated conversion. ;-)

That's another place where enum support would be nice, but we'll manage without.

The rest is minor, will fix.
Comment 15 Owen Taylor 2010-06-04 11:51:56 UTC
(In reply to comment #14)
> > @@ +54,3 @@
> > +# to run from source tree
> > +all:
> > +    $(bindir)/glib-compile-schemas --targetdir=. .
> > 
> > should be --targetdir=. $(srcdir)
> We're compiling schemas to data/, so why do you want to change this?

srcdir here is not the 'src' subdir, but a standard automake thing. Say if I did:

 mkdir _build
 cd _build
 ../configure
 make

This would set up a srcdir != builddir build (you might do this if building for multiple architectures.) In that case srcdir is ../data - the sources aren't in the current directory. 'make distcheck' builds this way, which is probably the biggest reason I care...
 
> > ::: data/org.gnome.shell.gschema.xml.in
> > @@ +14,3 @@
> > +      <default>true</default>
> > +      <_summary>Whether to collect stats about applications usage</_summary>
> > +      <_description>The shell normally monitors active applications in order
> > to present the most used ones (e.g. in launchers). While this data will be kept
> > private, you may want to disable this for privacy reasons. Please note that
> > doing so won't remove already saved data.</_description>
> > 
> > The descriptions in this file need to be lined-wrapped to reasonable width.
> I hate doing this manually because this means the line length is fixed instead
> of the UI wrapping at a correct width. Are you sure that's The Right Way? What
> use case do you have in mind? 

My use case is keeping the schemas file as something that is easily edited with our standard programming tools: if you changed the spelling of a single word in a long description, would 'git diff' show meaningful output? Can you navigate in the file normally with emacs or vi?

My assumption was that XML whitespace normalization was being done on the description. It appears reading the code that gschemas-compiler is doing no normalization at all. I'll file a bug against GSettings about the topic, it's not 100% immediately obvious what is right, since whitespace normalization (all sequences of one-or-more-whitespace-characters converted to a single space) may be too radical.
Comment 16 Owen Taylor 2010-06-04 12:30:37 UTC
(In reply to comment #15)

> My assumption was that XML whitespace normalization was being done on the
> description. It appears reading the code that gschemas-compiler is doing no
> normalization at all. I'll file a bug against GSettings about the topic, it's
> not 100% immediately obvious what is right, since whitespace normalization (all
> sequences of one-or-more-whitespace-characters converted to a single space) may
> be too radical.

Filed:

 https://bugzilla.gnome.org/show_bug.cgi?id=620562

(Note that I was wrong about glib-compile-schemas - it just doesn't do anything with the description at all.... the description isn't part of the compiled schemas.) 

We'll see if if Ryan responds there, but for now I would assume that some sort of normalization of whitespace is being done. The alternative just isn't friendly to human editing of .schema files.
Comment 17 Milan Bouchet-Valat 2010-06-05 09:45:29 UTC
Created attachment 162791 [details] [review]
Migrate to GSettings

So, new patch fixing issues mentioned above. Noteworthy points:
- Bringing back GConf schema for Metacity. Actually, I could only find button_layout, which is somewhat minor. Am I missing something?

- Using GConf as GSettings backend for now (setting the GSETTINGS_BACKEND env var). Since Mutter still needs it, it's not really useful to run dconf at the same time: we will move when they are it's ported too.

- I've moved to <choices> where it makes sense to, but it's not yet supported, which means it works but I guess without checking for validity, and printing a warning. Not a big deal.

- I've marked the default hour format for translation, so that countries where 12-hour is weird can change it. But it doesn't seem to be detected, maybe it's not supported yet.

- Wrapped lines in the schema file so that you can edit it nicely in a terminal.

(In reply to comment #13)
> @@ +54,3 @@
> +# to run from source tree
> +all:
> +    $(bindir)/glib-compile-schemas --targetdir=. .
> 
> should be --targetdir=. $(srcdir)
I guess you meant --targetdir=$(srcdir) $(srcdir), based on your explanations. The schema is compiled into data/, which seems fine to me since it avoids moving them around the source tree.

Do you think it's better to compile it to src/, which will make settings the GSETTINGS_SCHEMA_DIR env var slightly simpler?

And I was wondering in comment 8:
One point I'd like to discuss is whether we need shell_global_get_settings(). I
used it in ShellAppUsage to get the global GSettings object, but JS uses the
GObject property instead of the accessor I guess. So we may either:
- use g_object_get() and remove the accessor (used 3 times in
shell-app-usage.c)
- or use a specific GSettings object for ShellAppUsage, with a child schema;
there's only one key, so that's not really needed, but that's not really costly
either.

The last question is the path we should use, /desktop/gnome/shell or something new, but that needs to be discussed to see whether the path system is to be changed or not.
Comment 18 Milan Bouchet-Valat 2010-06-05 10:03:13 UTC
Forgot to mention: glib from master fails compiling the schema ATM because it doesn't support <choices>. So you'll need the patch from bug 616102:
https://bugzilla.gnome.org/attachment.cgi?id=160189
Comment 19 Florian Müllner 2010-06-05 10:51:14 UTC
(In reply to comment #17)
> (In reply to comment #13)
> > @@ +54,3 @@
> > +# to run from source tree
> > +all:
> > +    $(bindir)/glib-compile-schemas --targetdir=. .
> > 
> > should be --targetdir=. $(srcdir)
> I guess you meant --targetdir=$(srcdir) $(srcdir), based on your explanations.

No, the above is the same as --targetdir=$(builddir) $(srcdir) - in a default build those are the same (data/), but autotools support using a different build directory. When building that way, the source tree is left untouched - all generated files are located in the build tree which mirrors the structure of the source tree (very handy for cross-compilation!).


> The schema is compiled into data/, which seems fine to me since it avoids
> moving them around the source tree.

Yes, but: the schema source is located in $(top_srcdir)/data and should be compiled to $(top_builddir)/data. I'm pretty sure you will break "make distcheck" otherwise ...


> And I was wondering in comment 8:
> One point I'd like to discuss is whether we need shell_global_get_settings(). I
> used it in ShellAppUsage to get the global GSettings object, but JS uses the
> GObject property instead of the accessor I guess. So we may either:
> - use g_object_get() and remove the accessor (used 3 times in
> shell-app-usage.c)
> - or use a specific GSettings object for ShellAppUsage, with a child schema;
> there's only one key, so that's not really needed, but that's not really costly
> either.

You are right, the JS code uses properties instead of the C convenience function. Nevertheless it's pretty common to have both - properties are often easier to use in bindings, while the accessor functions are nicer on the C side - so i don't see the problem here. If you absolutely insist, I guess using g_object_get is acceptable as well - using "per-language" objects is not: going back and forth between C and Javascript should be as seamless as possible.
Comment 20 Milan Bouchet-Valat 2010-06-05 11:46:52 UTC
(In reply to comment #19)
> (In reply to comment #17)
> > I guess you meant --targetdir=$(srcdir) $(srcdir), based on your explanations.
> 
> No, the above is the same as --targetdir=$(builddir) $(srcdir) - in a default
> build those are the same (data/), but autotools support using a different build
> directory. When building that way, the source tree is left untouched - all
> generated files are located in the build tree which mirrors the structure of
> the source tree (very handy for cross-compilation!).
> 
> 
> > The schema is compiled into data/, which seems fine to me since it avoids
> > moving them around the source tree.
> 
> Yes, but: the schema source is located in $(top_srcdir)/data and should be
> compiled to $(top_builddir)/data. I'm pretty sure you will break "make
> distcheck" otherwise ...
Ah, I see the point. Thanks for the explanation!


> You are right, the JS code uses properties instead of the C convenience
> function. Nevertheless it's pretty common to have both - properties are often
> easier to use in bindings, while the accessor functions are nicer on the C side
> - so i don't see the problem here. If you absolutely insist, I guess using
> g_object_get is acceptable as well - using "per-language" objects is not: going
> back and forth between C and Javascript should be as seamless as possible.
I won't "absolutely insist"! ;-)

The idea was just that we happen not to use the getter except in ShellAppUsage, which could use a separate object as several others do. But I've written the code that way, it's because I thought it made sense in the current way too.


BTW, I need to add the gschema.xml.in file to EXTRA_DIST instead of the gschema.xml file which is generated at make time.

When running make distcheck, $(bindir)/glib-compile-schemas gets substituted to _inst/bin/glib-compile-schemas, which fails. Weird. Removing the $(bindir) prefix works, but make will fail if run outside of jhbuild.

Last bug is that I have to use all-hook instead of all-local to compile schemas, because all-local is run before schemas are generate, meaning the first time it (and make distcheck) fails.

With all these hacks it works. (But note previous patch is broken because I forgot to re-add src/gnome-shell.schemas, getting the old one with git checkout should work).
Comment 21 Florian Müllner 2010-06-05 12:44:08 UTC
(In reply to comment #20)
> When running make distcheck, $(bindir)/glib-compile-schemas gets substituted to
> _inst/bin/glib-compile-schemas, which fails. Weird. Removing the $(bindir)
> prefix works, but make will fail if run outside of jhbuild.

Both ways are wrong. $(bindir) is the directory where _the currently build package_ installs its binaries - glib-compile-schemas is not part of the package, so assuming that it uses the same install prefix is wrong. Distcheck fails because it uses a prefix inside the package dir (iirc it's $(top_srcdir)/_inst).

Removing $(bindir) assumes that glib-compile-schemas is in your normal $PATH, which is not guaranteed either (as you noticed in the run-outside-jhbuild case).

The solution is pretty simple: use $(GLIB_COMPILE_SCHEMAS) instead - this variable is initialized to the full path of glib-compile-schemas by the GLIB_SETTINGS macro in configure.ac.
Comment 22 Milan Bouchet-Valat 2010-06-06 19:38:16 UTC
Created attachment 162879 [details] [review]
Migrate to GSettings

This one should be good.

'make distcheck' now works thanks to:
- use $(GLIB_COMPILE_SCHEMAS) instead of $(bindir)/glib-compile-schemas
- use all-hook instead of all-local to compile schemas so that the schema has been generated when we need it

Schemas path is changed from /desktop/gnome/shell to /apps/gnome-shell, because the settings are specific to the Shell and not meant to be accessed by other programs. (See http://mail.gnome.org/archives/desktop-devel-list/2010-June/msg00104.html)
I guess if we want to have desktop-wide settings, they should be moved to gnome-settings-daemon or the hypothetical gnome-wm-data.

New patch also removed remnants of GConf code in shell-app-system.c, which was not used at all and must have been forgotten when moving favorite apps management to JS.

L10n for the hour format doesn't seem to be used, but it doesn't hurt. I need to see with desrt whether this is supported ATM, or if support will be added to intltool at some point, in which case having the schemas ready is a good point.
Comment 23 Milan Bouchet-Valat 2010-06-16 18:15:43 UTC
Created attachment 163860 [details] [review]
Migrate to GSettings

Updated patch, fixing a few silly mistakes pointed by Florian (stale args from the old strv API...).

Also actually requires GLib 2.25.8 in configure.ac since it's been released.

What's the plan for this? Waiting for Mutter and Metacity to use GSettings may take a while, since it's not that easy and we may need to discuss some design and migration issues. So it might be good to commit it and remove GConf when possible (anyway, we need gnome-desktop and friends to move too).
Comment 24 Owen Taylor 2010-06-16 19:37:17 UTC
Review of attachment 163860 [details] [review]:

I'm happy with this approach of just installing a stripped down GConf schemas file for now. This looks almost ready to go - just a few comments below. Unless you have any questions, it's OK to commit with the small fixes described below, then I'd like to see follow-on patches for:

 - Restoring <choices/>
 - Switching the backend to DConf

::: data/Makefile.am
@@ +55,3 @@
+	$(GLIB_COMPILE_SCHEMAS) --targetdir=. $(srcdir)
+
+all-hook: local-compile-schemas

I don't have a strong opinion on whether all-local or all-hook is correct; either would likely work. But if local-compile-schemas depends on org.gnome.shell.gschema.xml, then the way to handle that is not to obscurely depend on automake's ordering, but to write:

  local-compile-schemas: org.gnome.shell.schemas.xml
          $(GLIB_COMPILE_SCHEMAS) --targetdir=. $(srcdir)

Or even better, to keep from compiling every time 'make' is run. Do:

  gschemas.compiled: org.gnome.shell.schemas.xml
          $(AM_V_GEN) $(GLIB_COMPILE_SCHEMAS) --targetdir=. $(srcdir)
 
  all-local: gschemas.compiled

(and get rid of the PHONY.) Note the addition of $(AM_V_GEN) to make it intergrate properly with AM_SILENT_RULES.

The fact that org.gnome.shell.schemas.xml is generated brings out another point - you were right with the original rule of:

          $(GLIB_COMPILE_SCHEMAS) --targetdir=. .

Rather than the version with $(srcdir). So, putting that together, the correct form is:

  gschemas.compiled: org.gnome.shell.schemas.xml
          $(AM_V_GEN) $(GLIB_COMPILE_SCHEMAS) --targetdir=. .
 
  all-local: gschemas.compiled

@@ +60,3 @@
+
+
+# GConf schemas to override Metacity keys' defaults

Comment doesn't quite make sense to me reading it. Maybe

# GConf schemas: provide defaults for keys from Metacity we are overriding

::: data/org.gnome.shell.gschema.xml.in
@@ +52,3 @@
+        <choice value="single"/>
+        <choice value="grid"/>
+      </choices>

This doesn't seem to work with current GLib -doesn't pass schema validation. There's apparently new stuff landing soon that might fix, but if we commit this before that lands, you should:

 - Commit this file without the <choices>
 - Put a patch in bugzilla that adds the <choices> where appropriate. So we remember to fix that.

::: src/gnome-shell.in
@@ +156,3 @@
     env = dict(os.environ)
     env.update({'GNOME_SHELL_JS'      : '@GJS_JS_DIR@:@GJS_JS_NATIVE_DIR@:' + js_dir,
+                'GSETTINGS_BACKEND'   : 'gconf',

I talked to Ryan on IRC and his opinion was that we should be using the DConf backend at this point.

I think we should commit this patch as is to keep from it growing and ever revising, but I'd like to do the switch to DConf immediately after. That means:

 * DConf should be added to the moduleset
 * The gnome-shell wrapper should:
   - Check if there is a bus owner for the DConf name
   - If not, run the DConf daemon passing --keep-alive

(--keep-alive is currently ignored, but Ryan promises that it will in the future keep the daemon from exiting after a period of inactivity.)

The reason for this complexity is that the DBus daemon will be the DBus daemon from the enclosing session, so it won't look in the JHBuild DBus activation directory, so the normal DBus activation of the DConf daemon won't work.

::: tools/build/gnome-shell.modules
@@ +104,3 @@
         <dep package="gjs"/>
         <dep package="gconf"/>
+        <dep package="glib"/>

You need to make the gconf module dep the glib module so that GConf gets built with the GSettings backend, and gobject-introspection dep the glib module so that Gio-2.0.gir has GSettings support.
Comment 25 Milan Bouchet-Valat 2010-06-18 18:32:42 UTC
Pushed with the suggested changes, except we use <choices> since it's supported in GLib 2.25.9 (which we now require). And I've had to add Magnifier settings to the GConf schema since they're installed in /desktop/gnome/a11y. To be moved when gnome-desktop moves too.
Comment 26 Dean Loros 2010-06-19 06:44:41 UTC
Greetings!!!

I have tried to build GS this evening 3 times--twice as a clean build (removing all parts of GS & re-downloading all of it) & I still get stuck at this point:

*** Building gnome-shell *** [12/13]
make  
make  all-recursive
make[1]: Entering directory `/home/dean/gnome-shell/source/gnome-shell'
Making all in data
make[2]: Entering directory `/home/dean/gnome-shell/source/gnome-shell/data'
  GEN    gnome-shell.desktop.in
  GEN    gnome-shell.desktop
  GEN    gnome-shell-clock-preferences.desktop.in
  GEN    gnome-shell-clock-preferences.desktop
  GEN    gschemas.compiled
No schema files found
make[2]: *** [gschemas.compiled] Error 1
rm gnome-shell-clock-preferences.desktop.in gnome-shell.desktop.in
make[2]: Leaving directory `/home/dean/gnome-shell/source/gnome-shell/data'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/dean/gnome-shell/source/gnome-shell'
make: *** [all] Error 2
*** Error during phase build of gnome-shell: ########## Error running make   *** [12/13]


Is this due to the work you are talking about?
Comment 27 Milan Bouchet-Valat 2010-06-19 10:27:25 UTC
(In reply to comment #26)
> Is this due to the work you are talking about?
Definitely. But fixes have been committed yesterday. See bug 622050.

And you know, the point of all those changes were not *only* to break your build...
Comment 28 Dean Loros 2010-06-19 15:28:48 UTC
I understand this--was just reporting that my build had failed--thought that maybe something had been missed....I'll re-build this morning.
Comment 29 Dean Loros 2010-06-19 20:18:10 UTC
Please--I am not a petulant child----"And you know, the point of all those changes were not *only* to break your build..." Where did you get the idea that I was complaining about anything?

I have been building GS from almost the very first & try to help find bugs...if this is not what you want--please tell me.
Comment 30 Milan Bouchet-Valat 2010-06-19 20:26:04 UTC
Hey, don't take it so bad... I tought there was a touch of irony in your first comment. Maybe I was not awake enough this morning, so I'm sorry if irony wasn't intended.

Anyway, a good way to tackle build issues is to ask on IRC: people can say whether the problem is known or not. Often it saves you time, so you don't debug something known.
Comment 31 Dean Loros 2010-06-19 22:52:06 UTC
Thank You---I'm not much of a IRC'er---suppose I ought to get in the habit---been playing the game solo for far too many years & am use to posting things instead..
Comment 32 Florian Müllner 2010-06-19 23:04:57 UTC
(In reply to comment #31)
> Thank You --- [...] I'm used to posting things instead..

That is perfectly fine, and there are places to do that - alas, bugzilla is not one of them. Comments here should focus on fixing the issue at hand - this implies that there is (usually) little sense in commenting on closed bug reports.
Comment 33 Dean Loros 2010-06-20 00:02:36 UTC
Not a problem--I did not realize that this was closed & the commit referred to this report---so I came here to add to it....I will look closer in the future.
Comment 34 Bastien Nocera 2010-10-20 18:01:42 UTC
I filed bug 632723 about some of the shell's usage of GConf.
Comment 35 Javier Jardón (IRC: jjardon) 2011-09-29 14:11:54 UTC
Reopening as you still require gconf in your configure.ac.

Please, set the blockers bugs to completely remove the gconf dependency on this module and then we can close this bug
Comment 36 Javier Jardón (IRC: jjardon) 2011-09-29 14:17:33 UTC
Reopening as even the module is already ported, you still require gconf in your configure.ac.

Please, set the blockers bugs to completely remove the gconf dependency on this module and then we can close this bug
Comment 37 Dan Winship 2011-09-29 14:27:25 UTC
looks like it's just metacity/mutter and evolution-data-server
Comment 38 Florian Müllner 2012-06-08 17:10:46 UTC
Both mutter and e-d-s have been ported to GSettings, so we can finally close this \o/