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 635378 - Port to GSettings
Port to GSettings
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
3.2.x
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on: 662234 663425
Blocks: 622558 648700 651901 652447 663428 663429 663431
 
 
Reported: 2010-11-20 20:19 UTC by Javier Jardón (IRC: jjardon)
Modified: 2011-11-11 19:32 UTC
See Also:
GNOME target: 3.4
GNOME version: 3.1/3.2


Attachments
Port preferences to GSettings (163.00 KB, patch)
2011-11-04 19:00 UTC, Florian Müllner
reviewed Details | Review
Port preferences to GSettings (164.69 KB, patch)
2011-11-05 18:16 UTC, Florian Müllner
reviewed Details | Review
Port preferences to GSettings (164.71 KB, patch)
2011-11-05 18:51 UTC, Florian Müllner
reviewed Details | Review
Port preferences to GSettings (159.49 KB, patch)
2011-11-05 23:00 UTC, Florian Müllner
none Details | Review
Port preferences to GSettings (159.48 KB, patch)
2011-11-07 13:34 UTC, Florian Müllner
none Details | Review
Port preferences to GSettings (162.42 KB, patch)
2011-11-11 02:02 UTC, Florian Müllner
none Details | Review
Port preferences to GSettings (163.04 KB, patch)
2011-11-11 16:17 UTC, Florian Müllner
committed Details | Review

Description Javier Jardón (IRC: jjardon) 2010-11-20 20:19:55 UTC
http://live.gnome.org/GnomeGoals/GSettingsMigration

$ git grep gconf-c
src/core/prefs.c:#include <gconf/gconf-client.h>
Comment 1 Florian Müllner 2010-11-20 20:34:31 UTC
This is for settings shared with metacity, so setting a dependency on bug 621204.
Comment 2 Xavier Claessens 2011-09-01 13:00:22 UTC
This will also let gnome-shell defines new keybindings hopefully as said by Florian. So it's blocking bug #652447.
Comment 3 Florian Müllner 2011-11-04 19:00:43 UTC
Created attachment 200706 [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+.
Comment 4 Jasper St. Pierre (not reading bugmail) 2011-11-05 16:34:55 UTC
Review of attachment 200706 [details] [review]:

You seem to use "(MetaBasePreference *)cursor" a lot. Why not just access the struct field by name?

::: src/core/prefs.c
@@ +507,3 @@
   MetaEnumPreference *cursor = preferences_enum;
 
+  while (((MetaBasePreference*)cursor)->key != NULL)

cursor.base->key != NULL?

@@ +853,3 @@
                 meta_preference_to_string (pref));
 
+  /* add idle at priority below the gsettings notify idle */

does gsettings have a notify idle?

@@ +1107,3 @@
     {
+      /* Someone added a preference of an unhandled type */
+      g_assert_not_reached ();

OVERLAY_KEY is an unhandled type?

@@ +1360,3 @@
+ * This conversion is cannot be handled by GSettings since
+ * several values are stored in the same key (as a string).
+ */

I don't understand. The values that are passed to this function are after splitting.

@@ +2026,3 @@
+update_workspace_names (GSettings  *settings,
+                         const char *key,
+                         gpointer    data)

Why bother with "data"?
Comment 5 Florian Müllner 2011-11-05 18:16:14 UTC
Created attachment 200766 [details] [review]
Port preferences to GSettings

(In reply to comment #4)
> Review of attachment 200706 [details] [review]:
> 
> +  while (((MetaBasePreference*)cursor)->key != NULL)
> 
> cursor.base->key != NULL?

Sure (nitpick: it's actually cursor->base.key ;-)



> @@ +853,3 @@
>                  meta_preference_to_string (pref));
> 
> +  /* add idle at priority below the gsettings notify idle */
> 
> does gsettings have a notify idle?

Not really, I guess the comment is obsolete now ...


> @@ +1107,3 @@
>      {
> +      /* Someone added a preference of an unhandled type */
> +      g_assert_not_reached ();
> 
> OVERLAY_KEY is an unhandled type?

No. "I explicitly don't want to deal with this here" is not unhandled.


> @@ +1360,3 @@
> + * This conversion is cannot be handled by GSettings since
> + * several values are stored in the same key (as a string).
> + */
> 
> I don't understand. The values that are passed to this function are after
> splitting.

Right. But the comment refers to the value stored in GSettings (which is the unsplit string). I don't see how we could handle this entirely in GSettings - we could make MetaButtonFunction a flag and split the key into button-layout-left and button-layout-right, but we'd still miss the information about button ordering.


> @@ +2026,3 @@
> +update_workspace_names (GSettings  *settings,
> +                         const char *key,
> +                         gpointer    data)
> 
> Why bother with "data"?

It's the signature of the GSettings::changed signal, but yeah, could just use update_workspace_names (void) ...


Apart from the review changes, I also moved some keybinding keys to the mutter schema. The "run-command-N" and "command-N" keys will likely be removed as well, but while there are open questions in bug 663425, I left this out for the time being.
Comment 6 Jasper St. Pierre (not reading bugmail) 2011-11-05 18:26:36 UTC
Review of attachment 200766 [details] [review]:

Much cleaner :)

::: src/core/prefs.c
@@ +1099,3 @@
   else if (g_str_equal (key, KEY_OVERLAY_KEY))
     {
+      /* Someone added a preference of an unhandled type */

I still don't understand why this is here and not in an "else" block. If someone changes the OVERLAY_KEY while mutter/gnome-shell is active, won't we abort?

@@ +1356,2 @@
 static MetaButtonFunction
 button_function_from_string (const char *str)

The comment is above button_function_from_string, which takes the split string. I don't understand why button_function_from_string can't be handled with g_enum_value_get_value_by_nick.
Comment 7 Florian Müllner 2011-11-05 18:51:12 UTC
Created attachment 200768 [details] [review]
Port preferences to GSettings

(In reply to comment #6)
> ::: src/core/prefs.c
> @@ +1099,3 @@
>    else if (g_str_equal (key, KEY_OVERLAY_KEY))
>      {
> +      /* Someone added a preference of an unhandled type */
> 
> I still don't understand why this is here and not in an "else" block. If
> someone changes the OVERLAY_KEY while mutter/gnome-shell is active, won't we
> abort?

Gah, right - confusion with strcmp() syntax on my part. Fixed.


> 
> @@ +1356,2 @@
>  static MetaButtonFunction
>  button_function_from_string (const char *str)
> 
> The comment is above button_function_from_string, which takes the split string.
> I don't understand why button_function_from_string can't be handled with
> g_enum_value_get_value_by_nick.

Yes, we use that - but that's unrelated to the GSettings port, so I prefer to leave it out for now.

(I guess I could leave the old FIXME comment if it bothers you ;)
Comment 8 Milan Bouchet-Valat 2011-11-05 18:59:15 UTC
(In reply to comment #5)
> > @@ +853,3 @@
> >                  meta_preference_to_string (pref));
> > 
> > +  /* add idle at priority below the gsettings notify idle */
> > 
> > does gsettings have a notify idle?
> 
> Not really, I guess the comment is obsolete now ...
And can't we drop the whole idle callback thing? IIRC it was supposed to ensure we'll handle separate changes in one batch, but maybe this isn't needed with GSettings. That's something I didn't figure out.

BTW, thanks for finishing this one! ;-)
Comment 9 Jasper St. Pierre (not reading bugmail) 2011-11-05 19:39:21 UTC
Review of attachment 200768 [details] [review]:

::: src/core/prefs.c
@@ +1420,3 @@
+
+  string_value = g_variant_get_string (value, NULL);
+  new_layout = g_new (MetaButtonLayout, 1);

g_new0

@@ +1558,3 @@
+  int i;
+
+  g_free (value);

This isn't too pretty either.

@@ +2075,3 @@
+  /* FIXME: we need to generate the name on-the-fly sometimes,
+   * but requiring the caller to free it would be silly. */
+  static char *prev_name = NULL;

This is disgusting, sorry. Requiring users to g_free wouldn't be that bad. Looking through the code, I spot less than ten references of it down the call chain (meta_workspace_get_name, meta_core_get_workspace_name_with_index). It also seems that it's being freed sometimes already (maybe incorrectly?) -- see tabpopup.c, free_tab_entry and menu.c, callers of get_workspace_name_with_accel.

::: src/org.gnome.mutter.gschema.xml.in
@@ +81,3 @@
+
+    <key name="toggle-recording" type="as">
+      <default>['&lt;Control&gt;&lt;Shift&gt;&lt;Alt&gt;r']</default>

Can we use CDATA here so that we don't have to do this?
Comment 10 Jasper St. Pierre (not reading bugmail) 2011-11-05 19:40:08 UTC
(In reply to comment #7)
> > @@ +1356,2 @@
> >  static MetaButtonFunction
> >  button_function_from_string (const char *str)
> > 
> > The comment is above button_function_from_string, which takes the split string.
> > I don't understand why button_function_from_string can't be handled with
> > g_enum_value_get_value_by_nick.
> 
> Yes, we use that - but that's unrelated to the GSettings port, so I prefer to
> leave it out for now.
> 
> (I guess I could leave the old FIXME comment if it bothers you ;)

The comment confused me, but it's fine. I misread "GSettings" as "g_enum_value_get_value_by_nick", somehow, like the old comment said.
Comment 11 Florian Müllner 2011-11-05 23:00:00 UTC
Created attachment 200791 [details] [review]
Port preferences to GSettings

(In reply to comment #9)
> Review of attachment 200768 [details] [review]:
> 
> ::: src/core/prefs.c
> @@ +1420,3 @@
> +
> +  string_value = g_variant_get_string (value, NULL);
> +  new_layout = g_new (MetaButtonLayout, 1);
> 
> g_new0

Seems pointless to me - it is an error to access any array members past META_BUTTON_FUNC_LAST, so getting undefined behavior in that case looks perfectly fine to me (note that 0 does not mean "invalid" or "undefined", but represents the "menu" button) ...

Anyway, changes outlined below allow to use stack allocation, so not an issue either way.



> @@ +1558,3 @@
> +  int i;
> +
> +  g_free (value);
> 
> This isn't too pretty either.

Right (note that all handler/mapping function do that). I see two alternatives:

 - don't use g_settings_get_mapped (), but just port the existing
   handler functions - of course we would loose the inherit fallback
   provided by g_settings_get_mapped (), which is a rather nice feature

 - merge mapping function and handler, e.g. store the mapped value from within
   the mapping function and ignore the return value of g_settings_get_mapped();
   this means that the function should also do change notifications, but as the
   existing functions already need to check whether the value has been changed,
   this does not seem so bad ...

Unsurprisingly, I went with the second option :)


> @@ +2075,3 @@
> +  /* FIXME: we need to generate the name on-the-fly sometimes,
> +   * but requiring the caller to free it would be silly. */
> +  static char *prev_name = NULL;
> 
> This is disgusting, sorry. Requiring users to g_free wouldn't be that bad.

It *is* a rather intrusive change, sorry. I agree that the above is not elegant by any means, so here's a much less intrusive alternative - call g_intern_string() on the generated string, which means that we keep one (and exactly one) copy of each generated name around without the need to free it.


> Looking through the code, I spot less than ten references of it down the call
> chain (meta_workspace_get_name, meta_core_get_workspace_name_with_index). It
> also seems that it's being freed sometimes already (maybe incorrectly?) -- see
> tabpopup.c, free_tab_entry and menu.c, callers of get_workspace_name_with_accel.

No, you got that one wrong - free_tab_entry frees a copy, the workspace name would need to be freed in meta_screen_workspace_popup_create(), and the code would look like this:

i = 0;
while (i < len)
  {
    /* build entries */
  }
popup = meta_ui_tab_popup_new (entries, ...)
i = 0;
while (i < len)
  {
    if (entries[i]->title)
      g_free (entries[i]->title);
  }
g_free (entries);

I'd rather not go there ...


> ::: src/org.gnome.mutter.gschema.xml.in
> @@ +81,3 @@
> +
> +    <key name="toggle-recording" type="as">
> +      <default>['&lt;Control&gt;&lt;Shift&gt;&lt;Alt&gt;r']</default>
> 
> Can we use CDATA here so that we don't have to do this?

Not as far as I know - at least all tries to use CDATA have resulted in the value being completely ignored.
Comment 12 Florian Müllner 2011-11-05 23:21:35 UTC
(In reply to comment #8)
> (In reply to comment #5)
> > > @@ +853,3 @@
> > >                  meta_preference_to_string (pref));
> > > 
> > > +  /* add idle at priority below the gsettings notify idle */
> > > 
> > > does gsettings have a notify idle?
> > 
> > Not really, I guess the comment is obsolete now ...
> And can't we drop the whole idle callback thing? IIRC it was supposed to ensure
> we'll handle separate changes in one batch, but maybe this isn't needed with
> GSettings. That's something I didn't figure out.

I *guess* that the primary reason for deferring change notifications are keybindings - rebuilding the bindings table and regrabbing the key bindings is not cheap, so trying to do the work only once even when several bindings have been changed looks like a good idea.

GSettings will still emit one ::changed signal per key, so it still appears useful. (GSettings also has a ::change-event signal which could possibly be used to replace the idle handlers, but I'm not sure the replacement would be much nicer than the existing code)

(That said, I really have no idea how likely it is that multiple keys will change at once - I can't think of anything besides control-center, tweak-tool and gsettings/dconf-editor which would make changes to our schemas, and none of those will do "batch updates" ...)
Comment 13 Matthias Clasen 2011-11-06 16:11:54 UTC
> > +      <default>['&lt;Control&gt;&lt;Shift&gt;&lt;Alt&gt;r']</default>
> > 
> > Can we use CDATA here so that we don't have to do this?
> 
> Not as far as I know - at least all tries to use CDATA have resulted in the
> value being completely ignored.


CDATA should work - gmarkup does support it. If it doesn't work, I'd like to see a testcase.
Comment 14 Florian Müllner 2011-11-06 18:29:04 UTC
(In reply to comment #13)
> > > +      <default>['&lt;Control&gt;&lt;Shift&gt;&lt;Alt&gt;r']</default>
> > > 
> > > Can we use CDATA here so that we don't have to do this?
> > 
> > Not as far as I know - at least all tries to use CDATA have resulted in the
> > value being completely ignored.
> 
> 
> CDATA should work - gmarkup does support it. If it doesn't work, I'd like to
> see a testcase.

I tested two variants:

 <default><![CDATA[['<Control><Alt><Shift>r']]]></default>
 <default>['<![CDATA[<Control><Alt><Shift>r]]>']</default>

In the first case, glib-compile-schemas fails with the following error: "expected value.  --strict was specified; exiting".
The second case succeeds, but ends up with a value of [''].

So if it is supposed to work, it looks like we have a test case for a bug :)
Comment 15 Matthias Clasen 2011-11-07 03:49:49 UTC
Ah. You need G_MARKUP_TREAT_CDATA_AS_TEXT - thats a markup parser option, so a change to the schema parser is required.
Comment 16 Matthias Clasen 2011-11-07 12:32:32 UTC
I've fixed that in glib now
Comment 17 Florian Müllner 2011-11-07 13:34:12 UTC
Created attachment 200880 [details] [review]
Port preferences to GSettings

Thanks!
Comment 18 Jasper St. Pierre (not reading bugmail) 2011-11-07 20:08:27 UTC
(In reply to comment #11)
> Right (note that all handler/mapping function do that). I see two alternatives:
> 
>  - don't use g_settings_get_mapped (), but just port the existing
>    handler functions - of course we would loose the inherit fallback
>    provided by g_settings_get_mapped (), which is a rather nice feature
> 
>  - merge mapping function and handler, e.g. store the mapped value from within
>    the mapping function and ignore the return value of g_settings_get_mapped();
>    this means that the function should also do change notifications, but as the
>    existing functions already need to check whether the value has been changed,
>    this does not seem so bad ...
> 
> Unsurprisingly, I went with the second option :)

Why not store a pointer to the ButtonLayout? That is:

    static MetaButtonLayout *button_layout = NULL;

> No, you got that one wrong - free_tab_entry frees a copy, the workspace name
> would need to be freed in meta_screen_workspace_popup_create(), and the code
> would look like this:
> 
> i = 0;
> while (i < len)
>   {
>     /* build entries */
>   }
> popup = meta_ui_tab_popup_new (entries, ...)
> i = 0;
> while (i < len)
>   {
>     if (entries[i]->title)
>       g_free (entries[i]->title);
>   }
> g_free (entries);
> 
> I'd rather not go there ...

Whoops, yeah, I missed the copy. interning is a good solution.
Comment 19 Florian Müllner 2011-11-07 21:11:39 UTC
(In reply to comment #18)
> Why not store a pointer to the ButtonLayout? That is:
> 
>     static MetaButtonLayout *button_layout = NULL;

How is that better? Depending on whether the setting has actually changed, we'd need to either free the static variable or the passed in result (and we'd still need to do it in the handler function, as the generic update_string() function does not know the actual return type of g_settings_get_mapped())
Comment 20 Jasper St. Pierre (not reading bugmail) 2011-11-09 19:08:56 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > Why not store a pointer to the ButtonLayout? That is:
> > 
> >     static MetaButtonLayout *button_layout = NULL;
> 
> How is that better?

It's not copying around the struct by value three or four times in the course of the function.

> Depending on whether the setting has actually changed, we'd
> need to either free the static variable or the passed in result (and we'd still
> need to do it in the handler function, as the generic update_string() function
> does not know the actual return type of g_settings_get_mapped())

We can always free the old one and swap the new one. We just don't notify if it hasn't changed.
Comment 21 Florian Müllner 2011-11-11 02:02:11 UTC
Created attachment 201206 [details] [review]
Port preferences to GSettings

Remove custom keybindings - no point in duplicating functionality which is already in g-s-d/g-c-c.
Comment 22 Jasper St. Pierre (not reading bugmail) 2011-11-11 02:32:10 UTC
Review of attachment 201206 [details] [review]:

::: src/core/keybindings.c
@@ -3604,3 @@
 
-static void
-handle_run_terminal (MetaDisplay    *display,

Won't removing this mean that you cannot spawn a terminal in the overview?
Comment 23 Florian Müllner 2011-11-11 02:54:16 UTC
(In reply to comment #22)
> 
> Won't removing this mean that you cannot spawn a terminal in the overview?

Yes.

(Note that it is not enabled in _globalKeyPressHandler(), so it doesn't work right now either)
Comment 24 Florian Müllner 2011-11-11 16:17:44 UTC
Created attachment 201243 [details] [review]
Port preferences to GSettings

Adapt to some more schema changes.
Comment 25 Florian Müllner 2011-11-11 19:31:53 UTC
Attachment 201243 [details] pushed as d0910da - Port preferences to GSettings

We agreed on IRC to get this in now and fix potential fallout later.