GNOME Bugzilla – Bug 616102
GSettings ignores <choice> and <range>
Last modified: 2010-06-16 22:32:06 UTC
They're in the DTD, but the schema compiler doesn't process them. Attached patch fixes this.
Created attachment 159014 [details] [review] Implement <choices> and <range> Bug 616102
i'll look at this today.
Review of attachment 159014 [details] [review]: ::: gio/gschema-compile.c @@ +63,1 @@ return FALSE; please one fix at a time. @@ +116,3 @@ +{ + if (g_variant_type_is_container (type)) + return is_type_valid_for_choices (g_variant_type_element (type)); you can only use type_element() with maybe and arrays, whereas is_container() returns true also for tuples, dictionary entries, variants in any case, i think it's reasonable to only allow this for strings, maybe strings, and array-of-strings. possibly maybe-array-of-string, but probably not array-of-maybe-string. :) @@ +170,3 @@ +} + +/* Maybe there should be a g_variant_compare() after all? :) */ i would support a g_variant_compare() that only operates with two items of the same basic type (ie: strings are OK too). it's pretty clear that you will need this logic in both the schema compiler and in gsettings itself. @@ +183,3 @@ + { +#define COMPARE(type_class,ctype,func,var_a,var_b) \ + case type_class: \ ouch :) @@ +407,3 @@ + g_set_error (error, G_MARKUP_ERROR, + G_MARKUP_ERROR_INVALID_CONTENT, + "Element <%s> must occur before element <default>", element_name); there's no good reason for this restriction. @@ +419,3 @@ + !g_variant_type_equal (state->type, G_VARIANT_TYPE ("t")) && + !g_variant_type_equal (state->type, G_VARIANT_TYPE ("d"))) + { yes. even if we do string compares in g_variant_compare(), it probably doesn't make sense to support string compares for <range>. @@ +422,3 @@ + gchar *type = g_variant_type_dup_string (state->type); + g_set_error (error, G_MARKUP_ERROR, + G_MARKUP_ERROR_INVALID_CONTENT, i guess it's actually better to throw do this check and throw this error on entry to <range> itself. @@ +477,3 @@ + if (strcmp (element_name, "choice") == 0) + { + state->string = g_string_new (NULL); the intention was actually to have like <choices> <choice value='foo'/> </choices> and then use the string directly rather than parsing it in the future i want to do something like <choices> <choice value='foo'> <alias value='bar'/> </choice> </choices> to deal with the case that your application used to support the "bar" option but no longer does and you want to migrate users to a similar "foo" setting instead of the default. @@ +505,3 @@ NULL, NULL, error); + if (state->value != NULL && state->l10n) good catch. i'll commit this one straight away. @@ +604,3 @@ + g_set_error_literal (error, G_MARKUP_ERROR, + G_MARKUP_ERROR_INVALID_CONTENT, + "Both <min> and <max> elements are required"); good. @@ +616,3 @@ + + g_variant_builder_add (&state->key_options, "{sv}", "range", + g_variant_new ("(@?@?)", state->min, state->max)); (??) is slightly less-awkward-looking and means exactly the same. the note in the docs about preferring @ to ?/* is only meant if you have extra type information (which you don't). @@ +636,3 @@ + else if (strcmp (element_name, "choices") == 0) + { + state->choices = g_variant_ref_sink (g_variant_builder_end (&state->choices_builder)); no need to ref_sink before passing into builder_add. you could easily use builder_open/close() here. the recursive structure of those calls matches nicely with the recursive structure of the sax callbacks.
actually, we need to have more thoughts about <choice> and <alias> so ignore what i said there for now.....
also: i'm adding g_variant_compare() for your convenience right now :)
I also have a patch for g_variant_compare() ready. So you want me to update the patch for <choice value="..."> but do nothing for <alias>, or hold off on the whole thing ?
hold off. this would be a lot easier if you were on irc :)
Updated the patch as discussed on IRC. (In reply to comment #3) > in any case, i think it's reasonable to only allow this for strings, maybe > strings, and array-of-strings. possibly maybe-array-of-string, but probably > not array-of-maybe-string. :) I guess so. However the check does need to descend into the container, so it's not any extra work to do it recursively, so... :) > @@ +407,3 @@ > + g_set_error (error, G_MARKUP_ERROR, > + G_MARKUP_ERROR_INVALID_CONTENT, > + "Element <%s> must occur before element <default>", > element_name); > > there's no good reason for this restriction. Removed this; the range and choices check is now done in </key> handler. > @@ +616,3 @@ > + > + g_variant_builder_add (&state->key_options, "{sv}", "range", > + g_variant_new ("(@?@?)", state->min, > state->max)); > > (??) is slightly less-awkward-looking and means exactly the same. the note in > the docs about preferring @ to ?/* is only meant if you have extra type > information (which you don't). The compiler doesn't really like "(??)": gschema-compile.c:417:51: warning: trigraph ??) ignored, use -trigraphs to enable > you could easily use builder_open/close() here. the recursive structure of > those calls matches nicely with the recursive structure of the sax callbacks. I don't understand this remark.
Created attachment 159154 [details] [review] Add support for <range> and <choices> to the schema compiler Bug #616102.
Created attachment 159158 [details] [review] Add testcases for <range> and <choices> Depends on bug #616276. Bug #616102.
Comment on attachment 159154 [details] [review] Add support for <range> and <choices> to the schema compiler Committed this patch with some changes: biggest change is that instead of storing an array of strings in the schema file I'm storing an array of bytes containing each possible choice, delimited by "\xff" characters. This allows us to do a very fast strstr() (or even faster memmem() on GNU systems) instead of constructing and destructing a bunch of GVariant instances in order to check.
Comment on attachment 159158 [details] [review] Add testcases for <range> and <choices> Committed with minor conflict fixups
Ok. I committed these two patches to prevent further rotting (it was already a bit annoying to get the first one to apply). Thanks for the work, Christian. Changing the summary of this bug to reflect the missing piece of the puzzle.
Created attachment 159618 [details] [review] Allow <range> also for arrays of numeric types Bug #616102.
Created attachment 159619 [details] [review] Fix handling of <choices> in the schemas compiler Don't read freed memory, and don't go into an infinite loop. Bug #616102.
Created attachment 159620 [details] [review] Implement <choices> and <range> checks Bug #616102.
Comment on attachment 159619 [details] [review] Fix handling of <choices> in the schemas compiler Looks right to me.
Comment on attachment 159620 [details] [review] Implement <choices> and <range> checks Looks good to me in general. I'll let Ryan comment on the check_value api as well, but if we go with the GError, the quark and the enum need to move into the header, obviously.
Comment on attachment 159618 [details] [review] Allow <range> also for arrays of numeric types according to desrt, this needs work
Comment on attachment 159619 [details] [review] Fix handling of <choices> in the schemas compiler sorry, this is the one that needs-work -- the g_strstr_len check is buggy, as discussed on IRC.
Created attachment 160189 [details] [review] Fix handling of <choices> in the schemas compiler Don't read freed memory, and don't go into an infinite loop. Fix default value checks where one of the choices is a prefix/suffix of another of the choices. Bug #616102.
Created attachment 160190 [details] [review] Implement <choices> and <range> checks Bug #616102.
*** Bug 620567 has been marked as a duplicate of this bug. ***
big GSettings refactor that just landed fixes this