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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #21 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
(In reply to comment #7)
>  - tp_dbus_connection_try_register_object() is starting to be really long
> and pretty sequential. I would like to factor out at least 3 functions:
> dup_skeletons_from_dbus_object(); dup_skeletons_from_svc_interfaces(); and
> export_skeletons().

I split out the first two. The last is still part of try_register_object(),
which makes that function retain all uses of the Registration struct - I think
that makes sense.

>  - _register_object should check for GDBusObjectSkeleton instead of just
> GDBusObject iface. You're not going to get GDBusInterfaceSkeletons out of a
> GDBusObjectProxy. I would warn early if it's not a skeleton instead of
> pretending everything is fine.

Added a return-if-fail, but the implementation should work fine with any
third-party implementation of GDBusObject (as long as each interface is a
GDBusInterfaceSkeleton), so we can easily remove the return-if-fail later if we
want to.

>  - "TpSvcInterfaceSkeleton: move to the -dbus library" --> Hmm, that seems
> pretty complex juggling with the properties mixin. I would move the whole
> TpDBusPropertiesMixin to -core since ultimately we won't need it.

Done. I had to do some annoying juggling with errors and the Registration
struct to avoid crossing library boundaries, but what we have now should work.

>  - _tp_g_dbus_object_dup_interface_names(): I would take a varargs of
> interfaces to ignore

Done (and gave it a better name to hint at the purpose of the varargs). I'd use
const gchar * const * if this was public API, but varargs are fine for internal
code.

>  - "tp_base_connection_change_status: emit status-changed before
> StatusChanged": Yes, and the update_rcc_property(self); should be moved
> earlier as well for the same reason.

*** not done yet ***

>  - "tp_base_connection_set_self_handle: consolidate GObject setters": I
> don't think it is needed

Reverted; on closer inspection, the codegen seems fine. I can drop both the
commit and the revert if you want.

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

I separated this out.

> > _tp_base_connection_dup_contact_attributes_hash
> 
> This should really be called something without "hash" in it

Removed the _hash suffix.

> There's a static function in the presence mixin with the same problem.

Likewise.

> I also think it's confusing that this function returns a floating ref. _dup_
> ought to return (transfer full), I think.

Done. I think it's OK for _new_ to return a floating ref, but unexpected for
_dup_.

> "Non-NULL if @conn...", surely.

Done.

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

Consistently used "s" (or "o"), and fixed some leaks.

> This deserves the same static assertion about guint vs. guint32 as in my
> branch, IMO.

Putting it next to TpHandle will do.

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

*** not done yet *** (but also not on the critical path)

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