[Bug 77189] [next] make TpBaseConnection GVariant-based

[bugzilla-daemon at freedesktop.org]

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

--- Comment #14 from Xavier Claessens <xclaesse at gmail.com> ---
(In reply to comment #9)
> In the commit "TpBaseContactList: Use gdbus-codegen's skeleton to implement
> ContactList1" you seem to have also redone TpWeakRef to wrap a GWeakRef. Was
> that meant to be a separate commit? Is it required by that commit for some
> reason? The implementation looks fine though, so I'm happy to just write a
> separate commit message for it and call it fixed.
> 
> > _tp_base_connection_dup_contact_attributes_hash
> 
> This should really be called something without "hash" in it, since it
> returns a variant :-)
> 
> There's a static function in the presence mixin with the same problem.
> 
> I also think it's confusing that this function returns a floating ref. _dup_
> ought to return (transfer full), I think.

Hm, we usually return floating variants but in this case I don't know what's
best... So I would call it _get_ and return floating, or _dup_ and return
strong ref. As you wish.

> >    /* TRUE if @conn implements TpSvcConnectionInterface$FOO - used to
> >     * decide whether to emit signals on these new interfaces. Initialized in
> >     * the constructor and cleared when we lose @conn. */
> > -  gboolean svc_contact_list;
> > +  _TpGDBusConnectionInterfaceContactList1 *contact_list_skeleton;
> 
> "Non-NULL if @conn...", surely.

At that point in my branch there are still other gboolean, but ultimately
they'll all become skeletons in that branch. So +1 for changing the comment to
Non-NULL directly, it will become correct once everything lands.

> The most minor of nitpicking: you seem to be a little inconsistent about
> whether C -> GVariant format strings (g_variant_new(),
> g_variant_builder_add(), g_variant_dict_insert()) use "s" or "&s". There is
> no difference (a copy always needs to be made), so I think we should
> standardize on one, just to avoid readers having to think about whether it
> makes a difference. I'd prefer the simpler form with just "s".

As said on IRC, I though "&s" and "&o" in the setter side meant "no copy" just
like it does in the getter side. So now we have to check every code I wrote for
leaks. Clearly +1 for never using "&s" in the setter case.

> > +static GArray *
> > +handles_variant_to_array (GVariant *variant)
> 
> This deserves the same static assertion about guint vs. guint32 as in my
> branch, IMO. Static assertions are cheap (no runtime cost, minimal
> compile-time cost).

Instead of having that static assert everywhere, I would just put it just next
to the TpHandle typedef. We are never going to make TpHandle anything else than
32 bits. TpHandle only survived because it's too much work to remove them from
everywhere.

> I think we could benefit from a tp_handle_set_new_from_variant, which might
> get rid of that function entirely.

+1

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