Created attachment 300055 [details] [review]
gsettingsschema: support per-session overrides

Created attachment 300056 [details] [review]
glib-compile-schemas: support per-session overrides

Created attachment 300057 [details] [review]
add support for per-session overrides

Patch for dconf-editor.

Created attachment 300075 [details] [review]
gsettingsschema: support session-dependent defaults

Patch updated to use XDG_CURRENT_DESKTOP.

Created attachment 300076 [details] [review]
glib-compile-schemas: support session-dependent defaults

Patch updated to use XDG_CURRENT_DESKTOP.

Created attachment 300077 [details] [review]
dconf-schema: support session-dependent defaults

Patch updated to use XDG_CURRENT_DESKTOP.

Created attachment 300086 [details] [review]
glib-compile-schemas: support session-dependent defaults

hi,

Thanks for looking into this.

This approach will not work very well, though, and here's why:

Let's say we have some (10?) GNOME overrides and 1 MATE override.  For the sake of argument, they do not overlap (but we might imagine that they do).

We'll get the GNOME file compiled, with the 10 overrides and the MATE file compiled with 1.

Now, let's say XDG_CURRENT_DESKTOP=MATE:GNOME

This will find the MATE file with its one override and stop.  Meanwhile, we lose out on the GNOME overrides, even though MATE didn't further override them.

This turns out to be a surprisingly difficult problem to solve.  You have to figure out what schema content is included in each file.  We currently don't have any support for getting down to per-key granularity, and we also can't even get down to the level of per-schema without running into trouble with schemas that include each other (as children, for example).  If you consider that one desktop may override one key in a schema and another desktop a separate key, you can see how this gets to be a problem...

(In reply to Ryan Lortie (desrt) from comment #8)
> hi,
> 
> Thanks for looking into this.
> 
> This approach will not work very well, though, and here's why:

Agree, but would not this be better then current situation?

> Let's say we have some (10?) GNOME overrides and 1 MATE override.  For the
> sake of argument, they do not overlap (but we might imagine that they do).
> 
> We'll get the GNOME file compiled, with the 10 overrides and the MATE file
> compiled with 1.
> 
> Now, let's say XDG_CURRENT_DESKTOP=MATE:GNOME
> 
> This will find the MATE file with its one override and stop.  Meanwhile, we
> lose out on the GNOME overrides, even though MATE didn't further override
> them.

True if MATE wants to use GNOME overrides.

1. Just tell that this is unsupported and MATE should install their own versions of GNOME overrides even they are same. I doubt this would be big problem.

2. Compiling schemas we cannot use XDG_CURRENT_DESKTOP to get parent desktop, but if we want to use parent desktop overrides then session (MATE) could put extra file along with override files (for example xdg_current_desktop.txt) with parent desktop/desktops names in it (In your example - GNOME) or full XDG_CURRENT_DESKTOP. Then compiling schemas we could read parent desktop overrides too.

I would go for 1st. MATE most likely would want different overrides.

> This turns out to be a surprisingly difficult problem to solve.  You have to
> figure out what schema content is included in each file.  We currently don't
> have any support for getting down to per-key granularity, and we also can't
> even get down to the level of per-schema without running into trouble with
> schemas that include each other (as children, for example).  If you consider
> that one desktop may override one key in a schema and another desktop a
> separate key, you can see how this gets to be a problem...

This all sounds really unappealing to me. I was not a fan of these environment variable switches when they were reintroduced, and suggestions like this prove me right. 

Why is this needed in the first place ?

We have different mechanisms to do GNOME classic, so that doesn't really count as an example. Looking at Fedora, I find 3 overrides, 2 of which are generic distro branding, and the last one is gnome-terminal, which is a special case.

What do you want to override ?

User can install multiple desktops - Ubuntu, GNOME, GNOME Classic, GNOME Flashback, MATE and others. One default value does not work for all installed sessions...

User might like to switch between different session. Will you suggest to make new install with that desktop so it does not break currently installed desktop?

For example 'button-layout' in GNOME is 'appmenu:close', ubuntu use 'close,minimize,maximize:', but in GNOME Flashback I need it 'appmenu:minimize,maximze,close'.

User might like to test and/or simply use all these session. One default does not work - user will be forced to change it manually to value that will be best across all sessions.

What wrong with extending glib so each session can set their default values?

For example with my proposed patches third party app developers could develop application with different defaults for different session. App developed for GNOME could install override file for ubuntu to change something so it works best in both sessions.

Mutter:
'attach-modal-dialogs' is set to false by default. GNOME and GNOME Classic have schema files with overrides to set 'attach-modal-dialogs' to true by default. Extra schema, extra code to handle different defaults. Would not it be better to just have one override file for session which just changes default value?

GTK+:
There is extra code to handle GNOME Classic with comment that it is hack:
https://git.gnome.org/browse/gtk+/tree/gdk/wayland/gdkscreen-wayland.c#n723
Again, would not it be better to drop that hack?

Small changes to glib would allow to simplify code, remove hack/hacks in GNOME / GTK+. Plus it would allow other sessions to set their default values.

--

User might want to customize sessions - again problem. This could be solved by updating dconf and making it possible to save/use ./cache/dconf/gnome-classic/user instaed of /.cache/dconf/user. To make sure it does not break current installs dconf could watch for env variable - DCONF_STORE_SETTINGS_PER_SESSION. If not set or false dconf works just like before, if set to true it stores settings in session sub directory.

Now user can install, use and configure different sessions without breaking other sessions.

One more example with nautilus. Currently it hardcodes check for desktop environment:
https://git.gnome.org/browse/nautilus/tree/src/nautilus-desktop-canvas-view.c?id=c1f7d50f7ace6790169301d948b41efeb5ef63ca#n491

This works only with GNOME and Unity. What if MATE wants to use nautilus? Add more code?

With glib changes nautilus could be updated:
1. Add extra two extra settings:
org.gnome.nautilus.change-background with app and params settings with default values 'gnome-control-center' and 'background'.
2. Update code to get these values from settings.

Now ubuntu can simply override with their 'unity-control-center' and 'appearance'. If MATE wants they can set their defaults. And if user has custom application to change background he can change to use that application.

I think Alberts' arguments make sense. Having working overrides for the whole schemas is better than nothing.

Dear GLib developers: do you have some plan on how to implement overrides differently? If no, why not just apply these patches? (Or, if you don't want to do this at all, reject them and close the bug.)

Review of attachment 300075 [details] [review]:

(for the reasons stated in comment 8)

Review of attachment 300077 [details] [review]:

(for the reasons stated in comment 8)

Review of attachment 300086 [details] [review]:

(for the reasons stated in comment 8)

I don't want to close the bug because the bug remains, and I agree that this is something that we definitely want to support.

I just re-read comment 8...

What is wrong with my offered solution for parent desktop overrides in comment 9? Ok, will try again with different solution. I will use GNOME-Flashback as example.

Installing files
----------------

1. step - if desktop wants to use per session overrides then it must install key file 'gnome-flashback' in 'glib-2.0/desktops/' with content 'DesktopNames=GNOME-Flashback;GNOME;

2. step - applications installs .gschema.xml and/or .override files. If application wants different settings for different sessions it could install override file in 'glib-2.0/schemas/gnome-flashback/' directory.

Compiling schemas
-----------------

1. step - glib compiles main gschemas.compiled file. This should work exactly same way as it does not. read .gschema.xml and .override files from 'glib-2.0/schemas/' directory.

2. step - compile per session gschemas.compiled files.

2.1. step - glib reads key file from 'glib-2.0/desktops/' to get info about overrides to use.

2.2. step - glib reads gschema.xml and .override files from 'glib-2.0/schemas/' just like in 1. step. Then it use desktop desktop names from step 2.1 and reads overrides from 'glib-2.0/schemas/gnome/' and 'glib-2.0/schemas/gnome-flashback/' and created 'glib-2.0/schemas/gnome-flashback/gschemas.compiled' file.

Using per session schemas
-------------------------

1. step - glib checks current desktop (XDG_CURRENT_DESKTOP)

2. step - then it looks for gschemas.compilded in following order:
'glib-2.0/schemas/gnome-flashback/gschemas.compiled'
'glib-2.0/schemas/gnome/gschemas.compiled'
'glib-2.0/schemas/gschemas.compiled'

Saving settings per session
---------------------------

1. step - dconf could check for env variable DCONF_STORE_SETTINGS_PER_SESSION to determinate if settings should be saved also per session. By default it should be false.

2. step if above env variable is set to TRUE then dconf would use './cache/dconf/gnome-flashback/user' file not './cache/dconf/user' to store/read settings.

--

This does not include parent desktop problem from comment 8. Also I don't see how there could be problem overriding one schema keys from different desktops.

For example we have schema:
org.gnome.examaple with key1=value1, key2=value2, key3=value3.

gnome installs override
[org.gnome.example]
key1=gnome-value1
key3=gnome-value3

gnome-flashback installs override
[org.gnome.example]
key2=gnome-flashback-value2
key3=gnome-flashback-value3

Compiled schemas would have
gnome-flashback:
key1=gnome-value1, key2=gnome-flashback-value2, key3=gnome-flashback-value3
gnome:
key1=gnome-value1, key2=value2, key3=gnome-value3
default (main):
key1=value1, key2=value2, key3=value3

Dear Ryan,

Can you please comment on Alberts' proposal in comment 19?

I don't think saving settings per session makes any sense.

Settings are about configuring applications - don't make me configure the same application again just because I decided to use kde for a change, or be prepared for loud complaints.

The user won't have to reconfigure anything if (s)he changes the desktop.

The point is to allow the developers (not users) to change their applications' default settings to better match the desktop they are running in.

This is making developers' life easier and not affecting users in any way.

(In reply to Matthias Clasen from comment #21)
> I don't think saving settings per session makes any sense.
> 
> Settings are about configuring applications - don't make me configure the
> same application again just because I decided to use kde for a change, or be
> prepared for loud complaints.

To get such behaviour you would have to first set environment variable. So it is up to user to decide. You don't want configure again - you don't set environment variable. Simple, no?

But that is not goal of this bug - all I want is to get support for overrides per session. So I can set/install different defaults without affection other sessions.

My proposed changes does not change current behaviour. Does it? I am ready to update patches so they are acceptable by you, but all I have got is comment 8. I tried to propose solutions, but no one has told what wrong with them.

If they are so completely wrong then maybe give some hints / ideas how you would like to see this done and I could try to write patches.

(In reply to Alberts Muktupāvels from comment #23)

> If they are so completely wrong then maybe give some hints / ideas how you
> would like to see this done and I could try to write patches.

The thing is: I don't want to see it done... as I said at the outset, I think all this is leading in the wrong direction. Maybe Ryan is willing to accept patches - he doesn't share my opinion on this - he was the one that brought the XDG_CURRENT_DESKTOP thing upon us in the first place.

(In reply to Matthias Clasen from comment #24)
> The thing is: I don't want to see it done... as I said at the outset, I
> think all this is leading in the wrong direction

+1.  I think I'm going to lay this one to rest.

There is no nice solution here.  I don't want to create an explosion of directories.

(In reply to Allison Lortie (desrt) from comment #25)
> (In reply to Matthias Clasen from comment #24)
> > The thing is: I don't want to see it done... as I said at the outset, I
> > think all this is leading in the wrong direction
> 
> +1.  I think I'm going to lay this one to rest.
> 
> There is no nice solution here.  I don't want to create an explosion of
> directories.

What is nice solution? While waiting for your reply to email I have prepared few patches that I will attach.

This time:
- there will be no need to create new/extra directories
- there will be no extra gschemas.compiled files
- we will not loose overrides from parent desktop (comment #8)

Created attachment 348988 [details] [review]
gsettingsschema: get desktop names from XDG_CURRENT_DESKTOP

Created attachment 348989 [details] [review]
gsettingsschema: prepare to support multiple default values

Created attachment 348990 [details] [review]
glib-compile-schemas: store default value as array

Created attachment 348991 [details] [review]
glib-compile-schemas: allow to use prefix in gschema.override

With above patches now it is possible to create default value for desktop. For example GNOME Classic could have following override file to change button-layout (default in gschema.xml file is 'appmenu:close').

[gnome-classic:org.gnome.desktop.wm.preferences]
button-layout='appmenu:minimize,maximize,close'

Vendors still can change / override default values in both cases by installing override file that is sorted after GNOME Classic file. For example ubuntu could have following file:

[org.gnome.desktop.wm.preferences]
button-layout='close,minimize,maximize:'

[gnome-classic:org.gnome.desktop.wm.preferences]
button-layout='close,minimize,maximize:appmenu'

So now upstream GNOME would have two defaults:
- 'appmenu:close'
- 'appmenu:minimize,maximize,close'

But ubuntu could override these values to:
- 'close,minimize,maximize:'
- 'close,minimize,maximize:appmenu'

Glib can load new format and also old if schemas is not re-compiled.

Allison, can you please comment / review my new patches?

You've got the comments from the two maintainers:

  me: I don't want to see it done.
  Allison: There is no nice solution here.

I don't think anything else needs to be said

Matthias, I understand that you don't want to see this, but...

Allison did write this in comment #18:
"I don't want to close the bug because the bug remains, and I agree that this is something that we definitely want to support."

Also after "There is no nice solution here." he did wrote:
"I don't want to create an explosion of directories."

So I am offering other way to solve this problem. Is there any problems with my proposed solution, because I think new solution avoids problems that was raised with previous attempts?

Created attachment 349043 [details] [review]
gsettingsschema: avoid unneeded work

Created attachment 349069 [details] [review]
gsettingsschema: support multiple default values

Currently gschemas.compiled contains only one default value that
mostly works fine, but sometimes different desktops might want
different default values. For example GNOME has 'appmenu:close'
as default value for button-layout, but GNOME Classic has
different default - 'appmenu:minimize,maximize,close'.

Next commit will add needed changes to glib-compile-schemas that
will allow to override default value only for specific desktop.

For example GNOME Classic could install following override file:
[gnome-classic:org.gnome.desktop.wm.preferences]
button-layout='appmenu:minimize,maximize,close'

If glib-compile-schemas will detect prefix before schema id it
will not change default value, but will create extra default
value. In gschemas.compiled multiple defaults will be stored as
array - 'a(sv)' where 's' will be desktop name and 'v' actual
default value. Special desktop name 'all' is used to store
non-desktop specific default value.

Created attachment 349070 [details] [review]
glib-compile-schemas: support multiple default values

Allow to use prefix in gschema.override key file group to create
extra default value for desktop. Prefix must be desktop name as it
is found in XDG_CURRENT_DESKTOP, lowercased.

1) https://git.gnome.org/browse/gnome-shell/commit/?id=ae2751a68b20ab281ca74affd61562fb8869b639

"Now GSettings is expected to grow support for session specific defaults, which will render our entire override system obsolete (yay!). Given that, it seems acceptable to use a less generic (and uglier) approach in the meanwhile, in order to fix aforementioned problems. So move overrides back before core initialization and just hardcode the session-mode => override-schema relation."

2) https://bugzilla.gnome.org/show_bug.cgi?id=727546

"I also want to use it from gsettings for allowing changes to default values on a per-desktop basis.  This is something that Debarshi wanted for gnome-classic mode and something that distributors have been asking about for a while as well (ie: Ubuntu wants some things different in Unity desktops vs. GNOME desktops -- right now they have to break GNOME in order to make the changes in Unity)."

---

This might not be needed for GNOME Classic these days because there are override schemas for same task, but I think this is still valid for Ubuntu.

OK.  This approach shows a bit of promise.

Some notes:

 - adding O(n^3) lookup [len(n_desktops) * len(n_overrides) * strcmp] is bad

 - don't store the new format unconditionally.  only do it if overrides for
   that key actually exist.

 - this will explode if someone has a schema that has a key with a type of a(sv)

 - use the same scheme is as used for storing the untranslated defaults, instead

 - why store the xdg desktop list into each schema object?

 - i don't like the part where you add a new layer of syntax to keyfiles, but
   it might be better than any other options (like per-desktop files)

(In reply to Allison Lortie (desrt) from comment #39)
> OK.  This approach shows a bit of promise.
> 
> Some notes:
> 
>  - adding O(n^3) lookup [len(n_desktops) * len(n_overrides) * strcmp] is bad

Any suggestion? Should I use g_quark_from_string to avoid strcmp?

>  - don't store the new format unconditionally.  only do it if overrides for
>    that key actually exist.

Not sure if I understand this. :( a(sv) to store default value is used only if there are at least one desktop specific override.

>  - this will explode if someone has a schema that has a key with a type of
> a(sv)

Explode in what way? overrides key in org.gnome.settings-daemon.plugins.xsettings has this type... I just tested and it works, default value changes just like any other key/type.

Or did you mean something like this:
- default in schema - {}
- default for gnome - {'Gtk/ShellShowsAppMenu': <1>}
- default for gnome-classic - {'Gtk/DecorationLayout': 'appmenu:close'}

In this case I think GNOME Classic should include `'Gtk/ShellShowsAppMenu': <1>` in their own override file if that is needed. Or in other words - values should not be merged.

>  - use the same scheme is as used for storing the untranslated defaults,
> instead

Sorry, did not understand. :(

>  - why store the xdg desktop list into each schema object?

To not use static variable. I can redo this if needed.

What is your opinion? Should desktop names be cases sensitive or not? Currently values are changed to lowercase. 

>  - i don't like the part where you add a new layer of syntax to keyfiles, but
>    it might be better than any other options (like per-desktop files)

Specification allows all ASCII characters except [ and ] in group name. This is best thing I was able to come up... At least currently I have no better idea how to specify desktop specific default value.

Created attachment 349313 [details] [review]
gsettingsschema: add support for multiple default values

- xdg desktop list removed from schema object.
- both patches merged in one.

Created attachment 349318 [details] [review]
gsettingsschema: add support for multiple default values

Created attachment 349591 [details] [review]
gsettingsschema: add support for multiple default values

To avoid `adding O(n^3) lookup [len(n_desktops) * len(n_overrides) * strcmp] is bad` default values first are put in hash table.

hi,

Didrocks approached me at GUADEC to discuss this patch and I worked on a version of it that I think is acceptable.  Note: I think it actually *might* makes sense to switch this on the session variable (as originally suggested) rather than XDG_CURRENT_DESKTOP.  I'm not sure.  In any case, here is the patch with XDG_CURRENT_DESKTOP.

Created attachment 356938 [details] [review]
gsettingsschema: fix weird const-correctness issue

I'm not sure how this went unnoticed for so long...

Created attachment 356940 [details] [review]
gsettings: cleanup default value lookup

There are a couple of different ways (and soon one more) to access the
default value of a key.  Clean up the various places that access this to
avoid duplication.

Created attachment 356941 [details] [review]
gsettingsschema: Allow per-desktop overrides

Recognise a new 'd' option in schema keys which gives a dictionary of
per-desktop default values.  This dictionary is searched for the items
found in XDG_CURRENT_DESKTOP, in the order.  If nothing matches (or if
the option is missing) then the default value is used as before.

This feature was requested by Alberts Muktupāvels and this patch is
based on an approach devised by them.

Created attachment 356942 [details] [review]
glib-compile-schemas: Handle per-desktop overrides

Add a new syntax to override files: if the group name has a ':' in it,
it indicates that we want to override the default values of keys for
only one desktop.  For example:

[org.gnome.desktop.interface:Unity]
font-name='Ubuntu 12'

Will override the settings, only if "Unity" is found in
XDG_CURRENT_DESKTOPS.  Multiple per-desktop overrides can be specified
for a given key: the one which comes first in XDG_CURRENT_DESKTOPS will
be used.

Review of attachment 356941 [details] [review]:

::: gio/gsettingsschema.c
@@ +1423,3 @@
+  GVariant *value = NULL;
+  gint i;
+

if (!key->desktop_overrides)
    return NULL;

Review of attachment 356938 [details] [review]:

Already fixed in master.

Review of attachment 356940 [details] [review]:

This patch broke dconf-editor for me.

::: gio/gsettings.c
@@ +1204,3 @@
   value = g_settings_read_from_backend (settings, &skey, FALSE, FALSE);
 
+  value = g_settings_schema_key_get_default_value (&skey);

`if (value == NULL)` still needed here and in other places too.

(In reply to Allison Lortie (desrt) (extended vacation) from comment #44)
> hi,
> 
> Didrocks approached me at GUADEC to discuss this patch and I worked on a
> version of it that I think is acceptable.  Note: I think it actually *might*
> makes sense to switch this on the session variable (as originally suggested)
> rather than XDG_CURRENT_DESKTOP.  I'm not sure.  In any case, here is the
> patch with XDG_CURRENT_DESKTOP.

On XDG_SESSION_DESKTOP? I think that XDG_CURRENT_DESKTOP is better.

XDG_SESSION_DESKTOP is set from desktop file name, right? I have jhbuild-gnome-shell-3.26-wayland.desktop and jhbuild-gnome-shell-3.26-x11.desktop that I am using to test GNOME. It is still GNOME desktop, but overrides that would be installed for XDG_SESSION_DESKTOP=gnome would not work, because in my case it would be XDG_SESSION_DESKTOP=jhbuild-gnome-shell-3.26-x11.

Review of attachment 356942 [details] [review]:

::: gio/glib-compile-schemas.c
@@ +2090,3 @@
+                  g_variant_unref (state->default_value);
+                  state->default_value = value;
+                  g_free (string);

Should not `g_free (string);` be moved out of else statement? Otherwise it is not freed in `if (desktop_id != NULL)` case.

But... It seems more logical to free string after g_variant_parse, because it is only place where it is used.

(In reply to Allison Lortie (desrt) (extended vacation) from comment #48)
> Will override the settings, only if "Unity" is found in
> XDG_CURRENT_DESKTOPS.  Multiple per-desktop overrides can be specified
> for a given key: the one which comes first in XDG_CURRENT_DESKTOPS will
> be used.

I think the commit message needs s/_DESKTOPS/_DESKTOP/g.

Comment on attachment 356938 [details] [review]
gsettingsschema: fix weird const-correctness issue

Commit 5eededccda6236cd307b0f0bcc852e495e9fd8f8 already fixed this.

Created attachment 357056 [details] [review]
gsettings: cleanup default value lookup

There are a couple of different ways (and soon one more) to access the
default value of a key.  Clean up the various places that access this to
avoid duplication.

Created attachment 357057 [details] [review]
gsettingsschema: Allow per-desktop overrides

Recognise a new 'd' option in schema keys which gives a dictionary of
per-desktop default values.  This dictionary is searched for the items
found in XDG_CURRENT_DESKTOP, in the order.  If nothing matches (or if
the option is missing) then the default value is used as before.

This feature was requested by Alberts Muktupāvels and this patch is
based on an approach devised by them.

Created attachment 357058 [details] [review]
glib-compile-schemas: Handle per-desktop overrides

Add a new syntax to override files: if the group name has a ':' in it,
it indicates that we want to override the default values of keys for
only one desktop.  For example:

[org.gnome.desktop.interface:Unity]
font-name='Ubuntu 12'

Will override the settings, only if "Unity" is found in
XDG_CURRENT_DESKTOP. Multiple per-desktop overrides can be specified
for a given key: the one which comes first in XDG_CURRENT_DESKTOP will
be used.

Hey Allison! Thanks again for your work here (and thanks to Alberts for the initial patch).

After a good week-end sleeping on that, I agree that the approach with XDG_CURRENT_DESKTOP is the best one: enable to have multiple sessions sharing defaults, like Xorg/Wayland flavors without duplicating tem. We can even envision distro-wide default by a vanilla session (like using that font, apart on that session) which is really a nice advantage on basing on desktop name.


However, I just testing this set of patch and have an incorrect behavior and side-effect, even without using any per-desktop override.
One of the impacted key is the background (but that applies to most of them)

[org.gnome.desktop.background]
picture-uri

On my session (without the patch):
$ gsettings get org.gnome.desktop.background picture-uri
'file:///home/didrocks/Images/Wallpapers/montfuji.jpeg'

xml default file:
/usr/share/glib-2.0/schemas/org.gnome.desktop.background.gschema.xml:
      <default>'file:///usr/share/backgrounds/gnome/adwaita-timed.xml'</default>

We have an override being:
/usr/share/glib-2.0/schemas/10_ubuntu-settings.gschema.override:picture-uri='file:///usr/share/backgrounds/warty-final-ubuntu.png'

So, all is good, the montfuji.jpeg is the default gsettings (and GNOME desktop apps are gettings)


Adding the set of patches, rebuilding glib and restarting the session, I see a lot of values being reverted to the overriden default:
$ gsettings get org.gnome.desktop.background picture-uri

(process:18821): GLib-CRITICAL **: g_variant_get_type: assertion 'value != NULL' failed

(process:18821): GLib-CRITICAL **: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:18821): GLib-CRITICAL **: g_variant_get_type: assertion 'value != NULL' failed

(process:18821): GLib-CRITICAL **: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:18821): GLib-CRITICAL **: g_variant_lookup_value: assertion 'g_variant_is_of_type (dictionary, G_VARIANT_TYPE ("a{s*}")) || g_variant_is_of_type (dictionary, G_VARIANT_TYPE ("a{o*}"))' failed

(process:18821): GLib-CRITICAL **: g_variant_get_type: assertion 'value != NULL' failed

(process:18821): GLib-CRITICAL **: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:18821): GLib-CRITICAL **: g_variant_get_type: assertion 'value != NULL' failed

(process:18821): GLib-CRITICAL **: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:18821): GLib-CRITICAL **: g_variant_lookup_value: assertion 'g_variant_is_of_type (dictionary, G_VARIANT_TYPE ("a{s*}")) || g_variant_is_of_type (dictionary, G_VARIANT_TYPE ("a{o*}"))' failed
'file:///usr/share/backgrounds/warty-final-ubuntu.png'

-> see the warnings + spawning the default.

$ dconf dump /org/gnome/desktop/background/
[/]
picture-uri='file:///home/didrocks/Images/Wallpapers/montfuji.jpeg'

-> the user setting is thus the correct one in dconf database. Reverting glib as well has shown that none of the impacted keys have not been overwritten.

Tell me if you need more logs.

Just used Alberts's fix for the NULL check and I can testify it works like a charm.

I tried some keys without per-desktop override (a simple override), did try as well another key with a desktop override, and the 2 desktop are setting, reverting and behaving with user's configuration as expected.

Thanks to both of you!

Review of attachment 357058 [details] [review]:

::: gio/glib-compile-schemas.c
@@ +1960,3 @@
                     {
                       fprintf (stderr, _("; ignoring override for this key.\n"));
+                      g_strfreev (pieces);

This will cause double free when it will find next key that does not exist or when pieces are freed after `for (j = 0; keys[j]; j++)` loop.

@@ +1986,3 @@
+                    {
+                      fprintf (stderr, _("; ignoring override for this key.\n"));
+                      g_strfreev (pieces);

And here.

@@ +2018,3 @@
                     {
                       fprintf (stderr, _("Ignoring override for this key.\n"));
+                      g_strfreev (pieces);

And here.

@@ +2048,3 @@
                         {
                           fprintf (stderr, _("; ignoring override for this key.\n"));
+                          g_strfreev (pieces);

Same.

@@ +2078,3 @@
                         {
                           fprintf (stderr, _("; ignoring override for this key.\n"));
+                          g_strfreev (pieces);

And also here.

Created attachment 358665 [details] [review]
glib-compile-schemas: Handle per-desktop overrides

Add a new syntax to override files: if the group name has a ':' in it,
it indicates that we want to override the default values of keys for
only one desktop.  For example:

[org.gnome.desktop.interface:Unity]
font-name='Ubuntu 12'

Will override the settings, only if "Unity" is found in
XDG_CURRENT_DESKTOP. Multiple per-desktop overrides can be specified
for a given key: the one which comes first in XDG_CURRENT_DESKTOP will
be used.

There is an issue there with override plugins key per desktop, unsure if that's due to glib change or something specific rhythmbox/gedit/(libpeas?) do

trying to
[org.gnome.gedit.plugins:ubuntu]
active-plugins = ['modelines', 'docinfo', 'filebrowser', 'drawspaces', 'spell', 'time']

after starting gedit the key is reset to the upstream default (missing 'drawspaces')

same issue with [org.gnome.rhythmbox.plugins] active-plugins

It removes drawspaces for me even without these patches. Maybe that plugin does not work / does not load and libpeas removes it?

https://git.gnome.org/browse/gedit/tree/gedit/gedit-plugins-engine.c#n98

Add this before above line:
gchar **test = g_settings_get_strv (engine->plugin_settings, "active-plugins");
int i; for (i = 0; test[i] != NULL; i++) g_warning ("%s", test[i]);

Rebuild gedit, reset active-plugins and start it from terminal. That way you will know if override works and plugin is in list.

Or might be even easier to test:
gsettings get org.gnome.gedit.plugins active-plugins

Created attachment 360454 [details] [review]
gsettings: cleanup default value lookup

There are a couple of different ways (and soon one more) to access the
default value of a key. Clean up the various places that access this to
avoid duplication.

Created attachment 360455 [details] [review]
gsettingsschema: Allow per-desktop overrides

Recognise a new 'd' option in schema keys which gives a dictionary of
per-desktop default values. This dictionary is searched for the items
found in XDG_CURRENT_DESKTOP, in the order. If nothing matches (or if
the option is missing) then the default value is used as before.

This feature was requested by Alberts Muktupāvels and this patch is
based on an approach devised by them.

Created attachment 360456 [details] [review]
glib-compile-schemas: Handle per-desktop overrides

Add a new syntax to override files: if the group name has a ':' in it,
it indicates that we want to override the default values of keys for
only one desktop. For example:

[org.gnome.desktop.interface:Unity]
font-name='Ubuntu 12'

Will override the settings, only if "Unity" is found in
XDG_CURRENT_DESKTOP. Multiple per-desktop overrides can be specified
for a given key: the one which comes first in XDG_CURRENT_DESKTOP will
be used.

Created attachment 360457 [details] [review]
gsettings: cleanup default value lookup

There are a couple of different ways (and soon one more) to access the
default value of a key. Clean up the various places that access this to
avoid duplication.

Created attachment 360458 [details] [review]
gsettingsschema: Allow per-desktop overrides

Recognise a new 'd' option in schema keys which gives a dictionary of
per-desktop default values. This dictionary is searched for the items
found in XDG_CURRENT_DESKTOP, in the order. If nothing matches (or if
the option is missing) then the default value is used as before.

This feature was requested by Alberts Muktupāvels and this patch is
based on an approach devised by them.

Created attachment 360459 [details] [review]
glib-compile-schemas: Handle per-desktop overrides

Add a new syntax to override files: if the group name has a ':' in it,
it indicates that we want to override the default values of keys for
only one desktop. For example:

[org.gnome.desktop.interface:Unity]
font-name='Ubuntu 12'

Will override the settings, only if "Unity" is found in
XDG_CURRENT_DESKTOP. Multiple per-desktop overrides can be specified
for a given key: the one which comes first in XDG_CURRENT_DESKTOP will
be used.

Created attachment 360461 [details] [review]
gsettings: use per-desktop default in g_settings_binding_key_changed

This is a very useful feature for distros since it allows things like a MATE "spin" or "flavor" to have different theme settings than GNOME without affecting GNOME for users who have multiple desktops installed.

Since Allison hasn't been around lately, could anyone else review this for GNOME 3.28?

Philip asked me if I'd comment ---

for Ubuntu we'd like this merged; we are carrying these patches downstream - that's how the bugs introduced by some of the earlier iterations were discovered - and I think it'd be much better if we weren't diverged on the syntax of GSettings override files

FWIW, Florian's got some patches in bug #786496 which would make use of this feature if it were to be merged.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1013.