[Bug 30423] Provide convenience API for GVariant-based a{sv}s

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=30423

Danielle Madeley <danielle.madeley at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |danielle.madeley at collabora.
                   |                            |co.uk

--- Comment #3 from Danielle Madeley <danielle.madeley at collabora.co.uk> 2010-10-13 17:57:42 PDT ---
(In reply to comment #2)
> (In reply to comment #1)
> > Review thoughts:
> > 
> > + * If @dict is a floating reference, ownership will be taken as if via
> > + * g_variant_ref_sink(). Otherwise, ownership will not be taken.
> > 
> > This is very confusingly worded. As far as I know, it always holds a reference,
> > but seems to imply it will not.
> 
> If @dict is floating (see the GVariant docs for what that means), the caller no
> longer owns it after the call; if the caller previously had a "real reference",
> the caller gets to keep it. It's the same as g_variant_new_variant() and
> g_variant_ref_sink(). Any suggestions for better wording?

> g_variant_new_variant says "If @child is a floating reference (see
> g_variant_ref_sink()), the new instance takes ownership of @child." Would that
> (with s/child/dict/) be clearer?

Perhaps: A reference to @dict is taken via g_variant_ref_sink(). If @dict is a
floating reference, the new #TpVariantMap takes ownership of @dict.

> > There's set() and set_value(), and get_value(), but no get().
> 
> set() is safe, because having the wrong varargs would be programming error.
> 
> get() with varargs and format strings is either unsafe, because you're failing
> to check the types, or heavily limited, because we can only do a type-checked
> get() for types we specifically understand (in which case you probably want to
> do some limited type coercion, like get_uint32 does, to be nice to deficient
> bindings like dbus-glib).

Reasonable.

> > +TpVariantMap *
> > +tp_variant_map_copy (TpVariantMap *self)
> > 
> > This is the only way to unfreeze a variant map, which is potentially a pain in
> > the neck. It seems like there's also use for an unfreeze() method that works
> > similarly to this method... but dropping the ref on the variant. If that
> > exists, then perhaps copy() should just make an exact copy, which would be less
> > confusing.
> 
> I deliberately didn't add an unfreeze method: once frozen, a TpVariantMap is
> just as immutable as the underlying GVariant. The general idea was that things
> like tp_channel_borrow_immutable_properties() would return a frozen map; if you
> want to modify it, you make your own copy and do things with that instead.
> 
> Perhaps the frozen and thawed maps should in principle be different types, but
> I'd rather not have to have two copies of all the getters...
> 
> See also discussion with desrt on GNOME bug 625408, linked above. I implemented
> this myself because it seemed likely to be quicker than waiting for him.

Ok, that's reasonable. What about cases where you want to pass a TpVariantMap
to a function, but then continue to make changes to it (e.g. launching several
channels with the same TargetID), which is something that's possible with a{sv}
maps at the moment.

You might have something like:

map = tp_variant_map_new ();
map.set (TP_PROP_CHANNEL_TARGET_HANDLE, "u", TP_HANDLE_TYPE_ROOM,
         TP_PROP_CHANNEL_TARGET_ID, "s", "kittens at conf.litter.cat",
         NULL);

map.set (TP_PROP_CHANNEL_CHANNEL_TYPE, "s", TP_IFACE_CHANNEL_TYPE_TEXT, NULL);
ensure_channel (map);

map.set (TP_PROP_CHANNEL_CHANNEL_TYPE, "s", TP_IFACE_CHANNEL_TYPE_DBUS_TUBE,
         TP_PROP_CHANNEL_TYPE_DBUS_TUBE_SERVICE_NAME, "s",
"cat.litter.Catlaboration",
         NULL);
create_channel (map);

Is any function that accepts a TpVariantMap going to freeze your map on you?
Perhaps immutability needs to be separated from building the variant?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.