GNOME Bugzilla – Bug 635378
Port to GSettings
Last modified: 2011-11-11 19:32:01 UTC
http://live.gnome.org/GnomeGoals/GSettingsMigration $ git grep gconf-c src/core/prefs.c:#include <gconf/gconf-client.h>
This is for settings shared with metacity, so setting a dependency on bug 621204.
This will also let gnome-shell defines new keybindings hopefully as said by Florian. So it's blocking bug #652447.
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+.
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"?
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.
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.
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 ;)
(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! ;-)
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>['<Control><Shift><Alt>r']</default> Can we use CDATA here so that we don't have to do this?
(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.
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>['<Control><Shift><Alt>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.
(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" ...)
> > + <default>['<Control><Shift><Alt>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.
(In reply to comment #13) > > > + <default>['<Control><Shift><Alt>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 :)
Ah. You need G_MARKUP_TREAT_CDATA_AS_TEXT - thats a markup parser option, so a change to the schema parser is required.
I've fixed that in glib now
Created attachment 200880 [details] [review] Port preferences to GSettings Thanks!
(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.
(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())
(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.
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.
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?
(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)
Created attachment 201243 [details] [review] Port preferences to GSettings Adapt to some more schema changes.
Attachment 201243 [details] pushed as d0910da - Port preferences to GSettings We agreed on IRC to get this in now and fix potential fallout later.