[Bug 77194] [next] avoid dbus-glib type for TpBaseChannelFillPropertiesFunc

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Xavier Claessens <xclaesse at gmail.com> ---
(In reply to comment #1)
> Xavier's gdbus-all branch fixes this, although it needs rebasing onto my
> branch from Bug #77189, and all the CMs need to catch up.

Rebased my branch.

> 805dfb02:
> 
> +static void
> +channel_died_cb (gpointer data,
> +    GObject *deceased_channel)
> 
> How can this happen, now that we take a strong ref to the channel? (I think
> that strong ref is probably a mistake - isn't it circular?)

The code actually leaked priv->channel, set_property() shouldn't use
g_value_dup_object(). I added a fixup commit that change it to be a GWeakRef.

> e664dd8b:
> 
> +  if (klass->fill_immutable_properties)
> +    klass->fill_immutable_properties (chan, &properties);
> 
> No need for the conditional, the base class implements it. Not a blocker.

Right, added a fixup.

> This commit does part of Bug #77197 too - it makes TpBaseProtocol's
> immutable properties GVariant-based. I can see that you wanted to avoid
> having to have both GHashTable and GVariantDict versions of
> tp_dbus_properties_mixin_fill_properties_hash(), but it still seems odd to
> me. Still, we want to do both eventually...

I did both because it's pretty trivial renaming and we have to stop
TpBaseProtocol:immutable-properties from being a
TP_HASH_TYPE_QUALIFIED_PROPERTY_VALUE_MAP anyway.

Are you ok leaving both in the same commit or do you want to split by having
temporally both GVariantDict and GHashTable versions in properties mixin ?

> +          gchar *key = g_strdup_printf ("%s.%s", iface, property);
> +
> +          g_variant_dict_insert_value (dict, key,
> +              dbus_g_value_build_g_variant (&value));
> +          g_value_unset (&value);
> 
> key is leaked

Added fixup.

> > tp_dbus_properties_mixin_fill_properties_hash
> 
> This should have a different name now. No hash table is involved.
> fill_properties_dict?

I ran a s/_hash// on all files.

> ----
> 
> General:
> 
> I think we should make TpBaseChannel and TpBasePasswordChannel use a
> non-TpSvc* skeleton on this bug too (or clone a bug), to keep all the
> channel stuff vaguely together. It can be a follow-up branch if that's
> easier.

Yep, that's my goal :)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.