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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Looking at your gdbus-all branch as far as "TpBaseContactList: Use
gdbus-codegen's skeleton to implement ContactList1":

I will probably just fix the things below myself, if I have time - they are all
minor.

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.

>    /* 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.

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".

> +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).

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

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