GNOME Bugzilla – Bug 671667
provide capability to override arbitrary settings
Last modified: 2012-06-12 11:42:52 UTC
Sometimes a user may wish to override what would otherwise be an algorithmically determined xsetting (for example, the setting indicating if the shell shows menus) or introduce new settings for testing purposes. We could have an 'override' key in GSettings (as a dictionary) to support this. I've done some work on a branch toward that goal: http://git.gnome.org/browse/gnome-settings-daemon/log/?h=wip/xsettings-manager
Could you please attach a single squashed patch for review?
Created attachment 209320 [details] [review] squashed patch doesn't everyone always want beautifully split-into-a-million-commits patches? :)
(In reply to comment #2) > Created an attachment (id=209320) [details] [review] > squashed patch > > doesn't everyone always want beautifully split-into-a-million-commits patches? > :) Unless you plan on uploading every patch here for me to review, I'd rather get a huge patch to review, and double-check using the small commits. You would obviously commit the small patches.
Review of attachment 209320 [details] [review]: Looks fine to me, couple of style issues, and I'd like a regression suite that checks on the GVariant serialisation being added before this patch. (the patch relies on the the GVariant and XSetting representations being the same) ::: data/org.gnome.settings-daemon.plugins.xsettings.gschema.xml.in.in @@ +39,3 @@ + <default>{}</default> + <_summary>A dictionary of XSETTINGS to override</_summary> + <_description>This dictionary maps XSETTINGS names to overrides values. The values must be either strings, signed int32s or (in the case of colors), 4-tuples of uint16 (red, green, blue, alpha; 65535 is fully opaque).</_description> Do you have any documentation for the feature anywhere? It should explain how to set overrides from the command-line, or for system administrators through files, and how the precedence works. ::: plugins/xsettings/Makefile.am @@ +25,3 @@ $(AM_CPPFLAGS) +test_xsettings_SOURCES = \ Add the .h files to the SOURCES too. ::: plugins/xsettings/test-xsettings.c @@ +8,3 @@ + GError *error = NULL; + + gtk_init (NULL, NULL); Call i18n stuff too, pur-lease. ::: plugins/xsettings/xsettings-manager.c @@ +245,1 @@ + tmp = g_variant_new ("(qqqq)", value->red, value->green, value->blue, value->alpha); Use #define. @@ +291,1 @@ + while (buffer->len & 3) Could I have a for loop instead? This isn't very readable as a condition. @@ +305,3 @@ + g_string_append (buffer, string); + + while (buffer->len & 3) Ditto. Maybe move the loop in another function add_padding() or somesuch. @@ +376,3 @@ + if (!g_variant_is_of_type (value, G_VARIANT_TYPE_STRING) && + !g_variant_is_of_type (value, G_VARIANT_TYPE_INT32) && + !g_variant_is_of_type (value, G_VARIANT_TYPE ("(qqqq)"))) Use a #define for the colour.
Review of attachment 209320 [details] [review]: GVariant has an extensive test suite inside of GLib itself to verify that the serialisation is as expected for many different types. See these tests: /gvariant/serialiser/maybe /gvariant/serialiser/array /gvariant/serialiser/tuple /gvariant/serialiser/variant /gvariant/serialiser/strings /gvariant/serialiser/byteswap ::: data/org.gnome.settings-daemon.plugins.xsettings.gschema.xml.in.in @@ +39,3 @@ + <default>{}</default> + <_summary>A dictionary of XSETTINGS to override</_summary> + <_description>This dictionary maps XSETTINGS names to overrides values. The values must be either strings, signed int32s or (in the case of colors), 4-tuples of uint16 (red, green, blue, alpha; 65535 is fully opaque).</_description> Where do you recommend I put this documentation? ::: plugins/xsettings/Makefile.am @@ +25,3 @@ $(AM_CPPFLAGS) +test_xsettings_SOURCES = \ Didn't do that since the only advantage of that is getting them into the dist and they're already there on account of being included in the _SOURCES of other binaries. Will update, though, as a matter of style. ::: plugins/xsettings/test-xsettings.c @@ +8,3 @@ + GError *error = NULL; + + gtk_init (NULL, NULL); Sure. ::: plugins/xsettings/xsettings-manager.c @@ +245,1 @@ + tmp = g_variant_new ("(qqqq)", value->red, value->green, value->blue, value->alpha); This would be extremely unusual and lead to substantially less readability. Imagine using a #define for a printf format string. @@ +291,1 @@ + while (buffer->len & 3) Will move the padding-addition to a separate function. I've actually wanted this as an API on GString for a while... g_string_pad_to (string, 4); Will fix it here for now, though. @@ +376,3 @@ + if (!g_variant_is_of_type (value, G_VARIANT_TYPE_STRING) && + !g_variant_is_of_type (value, G_VARIANT_TYPE_INT32) && + !g_variant_is_of_type (value, G_VARIANT_TYPE ("(qqqq)"))) A #define could make sense here. Will do that.
These patches have been pushed to the branch: xsettings: i18nify the test-xsettings program xsettings: put headers in test_xsettings_SOURCES xsettings: add XSETTINGS_VARIANT_TYPE_COLOR macro xsettings: make the alignment padding clearer I dealt with your concerns about the g_variant_new() format string in the COLOR macro commit by adding an extra check right after the value is created.
(In reply to comment #5) <snip> > ::: data/org.gnome.settings-daemon.plugins.xsettings.gschema.xml.in.in > @@ +39,3 @@ > + <default>{}</default> > + <_summary>A dictionary of XSETTINGS to override</_summary> > + <_description>This dictionary maps XSETTINGS names to overrides values. > The values must be either strings, signed int32s or (in the case of colors), > 4-tuples of uint16 (red, green, blue, alpha; 65535 is fully > opaque).</_description> > > Where do you recommend I put this documentation? README.xsettings in plugins/xsettings would be good enough I think. Don't forget to add to EXTRA_DIST. <snip> > This would be extremely unusual and lead to substantially less readability. > Imagine using a #define for a printf format string. FWIW, I regularly use macros for printf formats when there's more than one use. > @@ +291,1 @@ > + while (buffer->len & 3) > > Will move the padding-addition to a separate function. > > I've actually wanted this as an API on GString for a while... > > g_string_pad_to (string, 4); > > Will fix it here for now, though. Feel free to CC: me on the bug requesting that :) Looks good with the patches on top, just missing the docs.
Now on the branch: xsettings: add README.xsettings about overrides
Looks good.
*** Bug 671144 has been marked as a duplicate of this bug. ***
(In reply to comment #9) > Looks good. Pushed then. Thanks for the quick review.
The patch broke the string freeze: #: ../data/org.gnome.settings-daemon.plugins.xsettings.gschema.xml.in.in.h:15 msgid "A dictionary of XSETTINGS to override" #: ../data/org.gnome.settings-daemon.plugins.xsettings.gschema.xml.in.in.h:16 msgid "" "This dictionary maps XSETTINGS names to overrides values. The values must be " "either strings, signed int32s or (in the case of colors), 4-tuples of uint16 " "(red, green, blue, alpha; 65535 is fully opaque)." Please revert or ask on gnome-i18n@ for an exception. You'd need two OKs. See https://live.gnome.org/TranslationProject/HandlingStringFreezes
When will we stop translating non-user visible options? commit d44610d23e30968c1f7fa9a38b8b25cfb7c5aa4d Author: Bastien Nocera <hadess@hadess.net> Date: Sat Mar 10 14:03:34 2012 +0100 data: Revert string addition https://bugzilla.gnome.org/show_bug.cgi?id=671667#c12
Maybe after you ask the question in the right place (gnome-i18n@) with the right audience and not in some random bug report.
(In reply to comment #14) > Maybe after you ask the question in the right place (gnome-i18n@) with the > right audience and not in some random bug report. Do you know what a rhetorical question is? :)
(In reply to comment #15) > Do you know what a rhetorical question is? :) Sure, I just once tried to take you seriously. :P
(In reply to comment #16) > (In reply to comment #15) > > Do you know what a rhetorical question is? :) > > Sure, I just once tried to take you seriously. :P I've been here before... http://thread.gmane.org/gmane.comp.gnome.internationalization.general/25747
Looks like this was all pushed before 3.4.0, in March. Closing