Re: A few comments on GVariant

[Christian Persch <chpe gnome org>]

Hi;

Am Mon, 30 Nov 2009 11:31:10 -0500
schrieb Ryan Lortie <desrt desrt ca>:
> On Mon, 2009-11-30 at 13:27 +0100, Christian Persch wrote:
> > - There really should be a GVariant tutorial, not just the API
> > docs. If I hadn't at least had a vague idea what GVariant does, I
> > would have been completely helpless with the API docs. Even so, I
> > had to proceed by trial-and-error by copying what the tests do...
> 
> I totally agree with this.  I've even considered using
> example/tutorial style documentation in place of full proper
> reference documentation in some places since it's difficult to
> formally describe, for example, GVariant format strings, but quite
> easy to show how to use them by example.

Agreed. The examples in tests/ helped me greatly to understand how it's
supposed to work.

> > - Type strings: to construct them, you need to remember which
> > character is which type. There are the G_VARIANT_TYPE_* defines,
> > but they are non-concatenable, i.e. you can't construct a type
> > string like G_VARIANT_TYPE_A G_VARIANT_TYPE_B but have to use "ab"
> > which makes typos uncaught at the compiler stage. Could there also
> > be concatenable #defines for these?
> 
> My attitude about this is that you should "just deal".  Basically,
> reserve some extra brain cells for it and memorise it -- just like
> printf.  The fact that it's the same as the DBus type system means
> that you can even share those brain cells. :)
> 
> Consider if we had to write things like:
> 
>   g_strdup_print ("int: " G_PRINTF_TYPE_INTEGER ", "
>                   "float: " G_PRINTF_TYPE_FLOAT "\n", 234, 2.5);

We do have that already with G_GINT*_FORMAT, G_GS[S]IZE_FORMAT,
G_GOFFSET_FORMAT, and at least the G[S]SIZE one is useful.

Personally, I find it much easier to type more
(maybe "G_VARIANT_TYPE_XYZ_S") instead of thinking more ("which
character correspond type XYZ to, again")? Plus, it's typo-safe at
compile-time. 

Then again, thanks to GVariant's liberal use of g_error(), you find out
quickly about typos at runtime.

> I think this really injures the readability.

True.

> > - g_variant_builder_add[_value]: should this return the
> >   passed-in GVariantBuilder* instead of void, so one can chain
> > calls?
> 
> That's an interesting proposal.  It's very evil in terms of refcounts,
> but the refcounting semantics of GVariantBuilder are fairly unbindable
> at this point anyway, so who cares about some extra magic? :)
> 
> Can you give me a good example of a use case you have here?

Since the g_variant_new ("aX", N, <v1>, <v2>, ..., <vN>) way didn't
exist anymore, my first idea was to do something like this:

v = g_variant_builder_end (
      g_variant_builder_add (
        ...
          g_variant_builder_add (
            g_variant_builder_add (
              g_variant_builder_new (...),
              <v1>)
            <v2>)
          <v3>)
          ...
        <vN>)

Not very elegant, and backwards, but works for short arrays.

> Do you have another method you'd prefer for array construction?  Maybe
> one that collects each array item from the varargs list?  Is that
> evil?
> 
> >   This appears to have been changed from earlier code; in the
> > removed glib/tests/gvariant-complex.c test, it was done this way:
> > 
> >   variant = g_variant_new ("(a{si}s)",
> >                            3,
> >                              "zero", 0,
> >                              "one",  1,
> >                              "two",  2,
> >                            "");
> > 
> >   IMHO a vastly more convenient way to build an array in one step,
> > in case one already knows how many elements one has and what they
> > are.
> > 
> >   Could there again be a convenience method like this?
> 
> mmm.  Yes.  It used to be like that, indeed.  The reason that I
> removed it was because it was not possible to have a dual of it for
> g_variant_get() and I'm trying to keep g_variant_new() as similar to
> g_variant_get() as is possible.

Ok. 

> Note that you can do in-place parsing of:
> 
>   "( { 'zero': 0, 'one': 1, 'two': 2 }, '' )"
> 
> if you prefer.

If I have to build the string at runtime, I can just as well use
GVariantBuilder directly. 

On the other hand, if the string is known at compile time, that brings
me to one point I forgot to mention in the original mail. It'd be great
if there was a standalone gvariant compiler (à la glib-genmarshal /
glib-mkenums) that takes gvariant markup input, and writes, chosen by a
command line option, one of

- a binary blob that I can then use with g_variant_from_file(), and

- source code (static const guint8 myvariant_data[] = { ... }), that
  I can use with g_variant_from_data().

> > - space efficiency: I was trying to build a variant containing some
> >   data, and multiple dicts to get at the data from various keys. i.e
> >   "a{sv}a{sv}a{sv}". I though that if I constructed the data
> >   GVariants first, and then added these to the dicts (i.e.
> >   v=g_variant_new(), add (key1, v) to dict1, add (key2, v) to dict2
> >   etc), the resulting blob (g_variant_flatten
> >   + get the data out) would contain that data only once instead of
> > once for each dict it was in. Well, it didn't work :-) 
> >   Was that a naive/dumb thing to expect, or can this be made to
> > work? 
> > 
> >   I ended up doing "ava{su}a{su}a{su}", first putting the data into
> >   an array, then the index of the data in the array into the dicts.
> >   Is that the best way for this?
> 
> This is the recommended way, yes.  If you have the data appearing
> multiple times then it will appear multiple times.  There is no
> compression here.

Ok.

> > - gvariant-*.c fail to #include "config.h"
> 
> What's the policy on this sort of thing?  Include everywhere or only
> include if needed?

Afaik it's "everywhere".

> > - the code uses literal 'X' / "X" in many places; IMHO using
> >   G_VARIANT_TYPE_CLASS_X / G_VARIANT_TYPE_X would make the code more
> >   readable / informative
> 
> All I can say to this is that it's easier to type this:
> 
>   case 'a': case 'b': case 'c': case 'd':
> 
> than:
> 
>   case G_VARIANT_TYPE_CLASS_ALPHA:
>   case G_VARIANT_TYPE_CLASS_BETA:
>   case G_VARIANT_TYPE_CLASS_GAMMA:
>   case G_VARIANT_TYPE_CLASS_DELTA:

Indeed, it's easier to type, but the long form is easier to read and
for code review. Anyway, it's not that important; just a personal
preference.

> > - g_variant_parse_full is rather unusual in that it returns an error
> >   not in a GError but a GVariantParseError. Could this be
> >   implemented instead by fixing bug 124022
> >   [https://bugzilla.gnome.org/show_bug.cgi?id=124022] and then using
> >   that to put the extra data into a GError? 
> 
> Does this bug have any hope of being fixed any time soon?

I just attached a patch there, which should at least raise the
probability a bit :-)

> The GSettings schema file parser calls the _full variant to parse
> default values out of schema files.  The extra information is used to
> tie-in to the error reporting system of that parser which does
> Vala-style reporting of the lcation of errors.
>                             ^^^^^^^

I see. In that case, it's indeed good to have more info.

Regards,
	Christian