Sorry for the delay.  The size of the patch scared me.

Generally, I feel this way:

I like the paramspec.  I particularly like that you added a GVariantType constraint.  This should go in but I don't feel as though I have the authority to ACK the patch (or, honestly the understanding of GParamSpec to properly review it).


I'm uncomfortable about the compare stuff.  The ordering you have picked seems a little bit arbitrary.  I'd actually probably prefer an ordering closer to how Python does it.

Really, though, I'd prefer for no defined ordering at all.

It seems like the only reason for having compare at all would be for GParamSpec -- and it's not really needed there.  I did some grepping around glib and gtk and it looks this this feature is really totally unused.  From a practical standpoint, I'm not sure the GVariant compare function would ever end up being called...


Some small nit-picks about the gobject patch:

  - your unrelated whitespace change should be separated out
  - it looks like you have some whitespace errors yourself (tabs vs. spaces) in the header

Another more substantial nit-pick: not sure if I like that you can have a NULL in the ParamSpec.  Might make sense to force the user to give a non-NULL default value.  Or maybe that's asking too much.

Thanks for the feedback!

(In reply to comment #1)
> I like the paramspec.  I particularly like that you added a GVariantType
> constraint. 

Any thoughts on whether an extra validation func should be supported, to check extra constraints that cannot be expressed merely by the GVariantType ?

> I'm uncomfortable about the compare stuff.  The ordering you have picked seems
> a little bit arbitrary.  I'd actually probably prefer an ordering closer to how
> Python does it.

I thought the order I defined was quite natural, at least the one for same-type variants. strcmp()-comparing the type strings for non-same-type variants is admittedly arbitrary.

> Really, though, I'd prefer for no defined ordering at all.
> 
> It seems like the only reason for having compare at all would be for GParamSpec
> -- and it's not really needed there.  I did some grepping around glib and gtk
> and it looks this this feature is really totally unused.  From a practical
> standpoint, I'm not sure the GVariant compare function would ever end up being
> called...

You're right, I just wrote the compare func for use in the param spec impl. I think I'll just make the paramspec cmp func simply compare the pointers, and drop the compare function.

>   - your unrelated whitespace change should be separated out

Hmm, I don't see any of these?

>   - it looks like you have some whitespace errors yourself (tabs vs. spaces) in
> the header

I'll fix the header.

(In reply to comment #2)
> Another more substantial nit-pick: not sure if I like that you can have a NULL
> in the ParamSpec.  Might make sense to force the user to give a non-NULL
> default value.  Or maybe that's asking too much.

Right, I wasn't sure either. In my use case, I think I'll always use a non-NULL default, but I don't know if there might be other uses that could use NULL... I'll change it to force non-NULL for now, with a g_return_if_fail(). It's always possible to drop this constraint later, whereas the opposite way to add it later isn't A[BP]I compatible.

Created attachment 155472 [details] [review]
updated patch

Created attachment 155473 [details] [review]
complete patch

(In reply to comment #3)
> Any thoughts on whether an extra validation func should be supported, to check
> extra constraints that cannot be expressed merely by the GVariantType ?

The only thing we could do here that would make everyone happy would be to provide a callback function that the user could use to provide their own validation checks.  That's a little bit unprecedented in terms of other GParamSpecs.


> >   - your unrelated whitespace change should be separated out
> Hmm, I don't see any of these?

Sorry.  Not whitespace -- spelling error. :)

- * @owner_type: #GType type that uses (introduces) this paremeter
+ * @owner_type: #GType type that uses (introduces) this parameter

I'm going to commit this tiny little piece now just to get it out of the way.

> Right, I wasn't sure either. In my use case, I think I'll always use a non-NULL
> default, but I don't know if there might be other uses that could use NULL...
> I'll change it to force non-NULL for now, with a g_return_if_fail(). It's
> always possible to drop this constraint later, whereas the opposite way to add
> it later isn't A[BP]I compatible.

I'm happy enough with this.  Glad you used ref_sink(), btw :)

One possibility for relaxing the API in the future for those creating the ParamSpec without violating the (non-NULL) expectations of those consuming the ParamSpec would be to have some support for getting the 'default value' for a given GVariant.  This notion is already well-defined internally in GVariant: it's what you get in the case of errors during deserialisation.  For numeric types it's zero.  For maybes, Nothing.  For arrays, the empty array.  For tuples or dictionary entries of a given type, it is the tuple where each of the values is equal to the default for its type.  For variants, it is equal to the variant containing triv -- the instance of the unit type: ().  This value is probably actually pretty close to what you want anyway...

Ping? Can this go in for 2.26 ?

Review of attachment 155473 [details] [review]:

Looks fine to me in general, once the few things below are fixed.

::: gobject/gparam.h
@@ +192,3 @@
  * @flags: #GParamFlags flags for this parameter
  * @value_type: the #GValue type for this parameter
+ * @owner_type: #GType type that uses (introduces) this parameter

Irrelevant hunk ?

::: gobject/gparamspecs.c
@@ +1115,3 @@
+  GParamSpecClass *parent_class = g_type_class_peek (g_type_parent (G_TYPE_PARAM_VARIANT));
+
+  g_variant_unref (vspec->default_value);

Need an "if (vspec->default_value)" here ?

@@ +1153,3 @@
+  GVariant *v1 = value1->data[0].v_pointer;
+  GVariant *v2 = value2->data[0].v_pointer;
+

Could use g_variant_compare here ?

@@ +2478,3 @@
+ * @blurb: description of the property specified
+ * @type: a #GVariantType, or %NULL
+ * @default:value: a #GVariant of type @type to use as the default value

Did you mean 'or NULL' to go the next line here ? The code certainly looks like it is trying to handle default_value == NULL, but not type == NULL.

Also, needs an (allow-non): annotation here, I guess.

@@ +2509,3 @@
+
+  vspec->type = g_variant_type_copy (type);
+  vspec->default_value = g_variant_ref_sink (default_value);

Needs a "if (default_value)" here, probably

(In reply to comment #8)
> Review of attachment 155473 [details] [review]:
> 
> Looks fine to me in general, once the few things below are fixed.
> 
> ::: gobject/gparam.h
> @@ +192,3 @@
>   * @flags: #GParamFlags flags for this parameter
>   * @value_type: the #GValue type for this parameter
> + * @owner_type: #GType type that uses (introduces) this parameter
> 
> Irrelevant hunk ?

That was just a typo fix that found its way into this commit. I just committed it independently.

> ::: gobject/gparamspecs.c
> @@ +1115,3 @@
> +  GParamSpecClass *parent_class = g_type_class_peek (g_type_parent
> (G_TYPE_PARAM_VARIANT));
> +
> +  g_variant_unref (vspec->default_value);
> 
> Need an "if (vspec->default_value)" here ?

Ryan seemed to prefer default_value to be guaranteed non-NULL (comment 2, comment 6), but after thinking about this a bit more, I think I lean towards allowing NULL.

> @@ +1153,3 @@
> +  GVariant *v1 = value1->data[0].v_pointer;
> +  GVariant *v2 = value2->data[0].v_pointer;
> +
> 
> Could use g_variant_compare here ?

Not possible, no. g_variant_compare only compares basic variants of the same type; it explicitly disallows comparing container types or variants of different types.
As comment 2 says, the compare feature of GParamSpec seems mostly unused in practise, so it should be ok to do strict pointer comparision here.

Created attachment 163853 [details] [review]
Add GParamSpecVariant

Bug #610863.

Created attachment 163870 [details] [review]
rebase

remove NULL as a default value.

The reason that I don't like that if you're giving a GVariantType (say uint32), the value should have that type.  NULL doesn't have a type of uint32.

Talking with Christian on IRC -- he is wondering if maybe this should have been a new fundamental type instead of a boxed type.  We're in a bit of a weird situation here with the type constraint.  That's somewhat odd for plain boxed types...

A fundamental type might be slightly more elegant, but is certainly a bit more work. You can't use g_value_set/get_boxed, then, and have to add new api for that, I guess: g_value_set/get_variant ?

Yes, you'd have to add g_value_set/get/take_variant. However, since G_TYPE_VARIANT is boxed in 2.24, I'm not sure if we can change this now; there might be code out there already doing g_value_init(&value, G_TYPE_VARIANT); g_value_set_boxed(&value, variant). (although google codesearch doens't return any hits for this pattern).

Actually, looking at gboxed.c code, I guess it _would_ be possible to fix this by cheating a little for G_TYPE_VARIANT :)

Created attachment 163912 [details] [review]
Add fundamental type for GVariant

Named G_TYPE_GVARIANT as to not clash with the to-be-deprecated
G_TYPE_VARIANT boxed type.

Add g_value_{set,take,get,dup}_variant.

Bug #610863.

Review of attachment 163912 [details] [review]:

::: gobject/gboxed.h
@@ -219,3 @@
- * Returns: %TRUE on success.
- *
- * Since: 2.26

Should this be deprecated, then ? together with the boxed type ?

::: gobject/gparamspecs.c
@@ -1582,3 @@
       param_variant_values_cmp,   /* values_cmp */
     };
-    pspec_info.value_type = G_TYPE_VARIANT;

VARIANT or GVARIANT here ?

::: gobject/gtype.h
@@ +186,3 @@
+ * 
+ * Since: 2.24
+ */

Again, mismatch with the docs

::: gobject/gvaluetypes.h
@@ +174,3 @@
+ * Since: 2.26
+ */
+#define G_VALUE_HOLDS_GVARIANT(value)    (G_TYPE_CHECK_VALUE_TYPE ((value), G_TYPE_GVARIANT))

What is it now, HOLDS_VARIANT or HOLDS_GVARIANT ?
Mismatch with the docs

(In reply to comment #16)
> Should this be deprecated, then ? together with the boxed type ?

This was actually new (in the pspec patch), and just removed. The new patch will deprecate G_TYPE_VARIANT boxed type.

As to GVARIANT vs. VARIANT: I _have_to_ use GVARIANT for the new type, but I'm wondering if all the others (esp. the pspec) should be GVARIANT too for consistency, or VARIANT for simplicity...

Created attachment 163933 [details] [review]
Add fundamental type and pspec for GVariant

Add G_TYPE_GVARIANT as fundamental type for GVariant, and
g_variant_{set,get,dup}_variant.
Deprecate the boxed type G_TYPE_VARIANT.

Add GParamSpecVariant.

Bug #610863.

Created attachment 163945 [details] [review]
Add fundamental type and pspec for GVariant

Make G_TYPE_VARIANT a fundamental type instead of boxed, and add
g_variant_{set,get,dup}_variant.

Add GParamSpecVariant.

Bug #610863.

I changed the G_TYPE_VARIANT type from boxed to fundamental, but kept the g_variant_get_gtype symbol as a deprecated function. I also changed the pspec back to allow NULL default values.

For the record, the advantages of a fundamental as opposed to a boxed type for GVariant are that pspec with a non-NULL default value, and with constraints seems very odd for a boxed type, but fits with what the other fundamental pspecs are doing; and also by adding the g_value_[sg]et_variant functions it allows for nice handling of floating variants.

Looks good to commit to me. But the changes will still go into .9, so ditch the extra NEWS section. It would be nice to mention the change from boxed to fundamental somewhere in the docs. Something like "Note that GLib 2.24 did include a boxed type with this name. It has been replaced by this type..."

Pushed to master with these changes, and a last-minute fix for NULL values in the pspec validation func.