Re: GVariant for prez!

[Simon McVittie <simon mcvittie collabora co uk>]

On Mon, 04 May 2009 at 10:25:06 -0400, David Zeuthen wrote:
> In contrast, most D-Bus clients that I write these days are not based on
> the "stick GValue in GHashTable, use GValueArray as a struct" design
> because it's just incredibly awkward to work with.

You're clearly not happy with the representations chosen by dbus-glib
(and I agree that they're not so good - they rarely match an existing
API, and they don't match the real D-Bus data model either).

I hope I'm misunderstanding your position somewhere: despite that, you seem to
be advocating having the object-mapping output such types, in the absence of
any other indication of what to return?

(This "default mapping" will always have to happen for variants, as far as I
can see; a default choice also needs to be made for the code generator when
run without special options or annotations.)

> Instead, I marshal
> these things into native concepts that every C programmer using GLib and
> GObject is intimately familiar with.
> 
> For example, see gnome-disk-utility which is a client of DeviceKit-disks
> (fairly complex D-Bus types being passed around) - e.g. when retriving
> historical data on a disk, a(tdtsba(isiiiis)) gets returned as a
> GPtrArray of GduAtaSmartAttribute objects.

Er, is a(tdtsba(isiiiis)) *really* the D-Bus signature you want? Perhaps that's
the best match for your representation in memory, but if that's your D-Bus
representation, then every time you add a field to the struct, you change
your D-Bus API. In Telepathy we're quite averse to APIs this complex without
some sort of extension point, to avoid having to change D-Bus API
if/when needed.

(Obviously, sometimes the contents of a struct clearly aren't going to get more
complicated - e.g. we model someone's presence as (u: general class of
presence, s: more specific (extensible) type, s: user-specified message) and
accept that it's not extensible beyond that - but for something with 7 struct
entries, I find it hard to believe there isn't an 8th on the way :-)

Indeed, most things we've done that get anywhere near this complicated ends
up with some extensible (~= vague) data structure like an a{sv} on D-Bus,
backed by an opaque object that does the "marshalling"/"unmarshalling" at both
the service and client end. The opaque client object (GduAtaSmartAttributes or
TpMessage or whatever) would populate its internal data structure (which might
very well be a GPtrArray of struct { gint, gchar *, gint, gint, gint, gint,
gchar * }) with whatever information was both supplied and understood, and
expose that through an API.

This lets our multiple implementations (GObject, Qt, Python) support not being
updated simultaneously, via clients coping gracefully with messages from more
or less capable implementations. I would urge the developers of any
non-trivial D-Bus API to have similar considerations.

This may explain why I'm so keen for variants to have a good
representation; using mechanisms like this, the stable parts of the Telepathy
specification haven't broken D-Bus API in 3 years, despite extensive
enhancements (we're heading towards our first API break since then).

If the internal representation in the D-Bus binding is good enough (GVariant or
whatever), then GduAtaSmartAttributes or TpMessage could easily use *that*
as the internal representation that is accessed through its API (particularly
since a GVariant can point to an individual argument halfway through a message
buffer, usage which DBusMessage only supports with a DBusMessage +
DBusMessageIter pair).

Regardless of such considerations, how do you propose to tell GDBusProxy to
return a GPtrArray of GduAtaSmartAttribute objects, as opposed to a GPtrArray
of any other object? G_TYPE_PTR_ARRAY isn't enough information for it to know
you want GduAtaSmartAttribute, so if this is the direction you want to go in,
I think the only way is to do dbus-glib's synthetic-GTypes trick, and go for
things like
dbus_g_type_get_collection ("GPtrArray", GDU_TYPE_ATA_SMART_ATTRIBUTE).
(I don't advocate doing that.)

> at least it cuts down the learning curve
> insofar that you can use existing data types and integrate with existing
> code without you having to rewire the data model of your application or
> write custom code to marshal things (which is the whole point of a type
> mapping)

As far as I can see, if there's no "custom code to marshal things", then you
have to accept whatever types the object mapping wants to give you (perhaps
modulo some simple choices between multiple alternatives that it knows about,
like G_TYPE_STRV vs G_TYPE_PTR_ARRAY), because it can't know how to convert
from D-Bus into any other types than the ones it already knows about. What is
it that I'm missing here?

If there *does* need to be custom code to map between D-Bus types and GLib
types, it seems sensible for something like GVariant to be the "D-Bus types"
end of that pipeline, because its data model *is* the D-Bus data model, so
no transformation is involved; as a side benefit, anyone who is happy with
GVariant as an representation for their variants could use it without
any automatic conversion.

Similarly, if things are being remapped, I don't think this:

static void
my_printer_manager_init (void)
{
  ...
  /* FIXME: what happens if someone else has their own ideas how to make
   * a MyDisasterDescription from a (siiiis), and registers another
   * type-mapping with a different conversion function? An assertion? */
  gdbus_add_type_mapping ("(siiiis)", MY_TYPE_FIRE_DESCRIPTION,
      my_disaster_description_from_gvariant, g_object_unref);
  ...
  proxy_connect_to_signal (proxy, "PrinterOnFire", printer_on_fire,
      "u(siiiis)", G_TYPE_UINT, MY_TYPE_FIRE_DESCRIPTION);
  ...
}

static void
printer_on_fire (guint printer_id, MyFireDescription *details,
                 gpointer user_data)
{
  MyPrinterManager *self = user_data;
  ...
}

is any clearer than this:

static void
my_printer_manager_init (void)
{
  ...
  proxy_connect_to_signal (proxy, "PrinterOnFire", printer_on_fire,
      "u(siiiis)");
  ...
}

static void
printer_on_fire (guint printer_id, GVariant *details, gpointer user_data)
{
  MyPrinterManager *self = user_data;
  MyFireDescription *fire = my_fire_description_from_gvariant (details);
  ...
  g_object_unref (fire);
}

(and it's only in the latter case that the user_data can influence the
construction of the MyFireDescription object, e.g. by a parent/child
relationship between objects, or relating the MyFireDescription to
my_printer_manager_get_printer (user_data, printer_id), or whatever - I think
this is likely to be useful, in practice.)

    Simon