Bug 77189

(In reply to comment #0)
> tp_contact_attribute_map_take_sliced_gvalue takes a GValue.
> 
> tp_base_connection_dup_contact_attributes_hash returns a
> GHashTable<string,GValue>.

That's part of http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-codegen-svc

(In reply to comment #1)
> That's part of
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-
> codegen-svc

Noted. I might try to pull it in earlier, because cutting over to the GDBus codegen seems more controversial/hard-to-design/likely-to-go-wrong than fixing up individual functions.

(In reply to comment #1)
> That's part of
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-
> codegen-svc

Review comments so far on that branch: either TpBaseConnection doesn't export the correct Interfaces property, or it does but I don't see how it works.

(In reply to comment #3)
> Review comments so far on that branch: either TpBaseConnection doesn't
> export the correct Interfaces property, or it does but I don't see how it
> works.

Oh, my mistake, there's explicit glue in tp_base_connection_add_interfaces() and tp_base_connection_create_interfaces_array(), both of which I deleted in the GDBusObjectSkeleton branch that I'm trying to rebase onto. (I don't really consider that branch to be ready until I've got your TpBaseConnection-without-TpSvcConnection working.)

Here is a rebase of some of your commits onto making TpBaseConnection (and TpBaseProtocol, because it was simpler) into GDBusObjectSkeletons:

http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus-object
http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=gdbus-object

Haze, Salut, Idle, Rakia are also going to need updates.

I think it'd be good to get this ready to commit before proceeding with the rest of your branches?

Sorry, make that:
http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus-object2

I'm happy with your commits in that branch if you're OK with my commits in the same branch :-)

I fixed a few minor things I noticed while reviewing.

One things I'm less sure about:

+ _tp_gdbus_connection_set_self_handle (self->priv->connection_skeleton,
+ self->priv->self_handle);
+ _tp_gdbus_connection_set_self_id (self->priv->connection_skeleton,
+ self->priv->self_id);

This could use g_object_set() like I did, or freeze + thaw notifications, so it wouldn't emit two separate PropertiesChanged signals (although arguably we should change the spec XML so it doesn't emit any at all).

We should probably go through the spec and classify our interfaces into "could use PropertiesChanged", "needs some properties marked as 'invalidates'", "not suitable for PropertiesChanged", then add appropriate annotations (and maybe even delete some unnecessary signals).

Overall, I think it is a good idea and I'm happy to see my commits can be included into your plan :)

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

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

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

 - _tp_g_dbus_object_dup_interface_names(): I would take a varargs of interfaces to ignore (or const gchar * const *ignore). Having just 2 is arbitrary and "skip_type" is fine for a channel but TpBaseConnection abuse of it for Requests.

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

 - "tp_base_connection_set_self_handle: consolidate GObject setters": I don't think it is needed, the gdbus-codegen emits PropertiesChanged in an idle callback and aggregates changes. It's a bit complex with threading, though... do you confirm the codegen is fine for our usage?

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

Fine, I'll do that.

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

Rationale: if we check for it being a GDBusObject containing GDBusInterfaceSkeletons instead of a GDBusObjectSkeleton, then it is easy to use a reimplementation of the GDBusObject interface.

Possible uses include:

* an object that has-a (as opposed to is-a) GDBusObjectSkeleton, which can easily implement GDBusObject by proxing the GDBusObjectSkeleton's interface

* retrofitting GDBusObject functionality onto an object whose superclass cannot change without breaking ABI

I think I'd be OK with adding a g_return_if_fail (!G_IS_DBUS_OBJECT (obj) || G_IS_DBUS_OBJECT_SKELETON (obj)) for now, so that every GDBusObject must also be a GDBusObjectSkeleton, but leaving the underlying implementation capable of dealing with any GDBusObject<GDBusInterfaceSkeleton> so that it's easy to relax the check in future. Does that sound OK?

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

When we spoke about this on IRC I said "yes, good point, we can do that". Unfortunately, now I've remembered why I did it this way: TpDBusPropertiesMixin depends on TP_ERROR, which is a symbol in the main library.

I would prefer not to have to bring back the -core library (3-level hierarchy) - there is a nonzero cost to library proliferation, and two levels ought to be enough.

We could just change the errors to be in the org.freedesktop.DBus.Error domain (probably InvalidArgs like GDBus uses), and patch the CMs' and MC's tests to cope - would that be OK?

>  - _tp_g_dbus_object_dup_interface_names(): I would take a varargs of
> interfaces to ignore (or const gchar * const *ignore). Having just 2 is
> arbitrary and "skip_type" is fine for a channel but TpBaseConnection abuse
> of it for Requests.

Yeah, that did occur to me. I'll do that.

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

OK.

>  - "tp_base_connection_set_self_handle: consolidate GObject setters": I
> don't think it is needed, the gdbus-codegen emits PropertiesChanged in an
> idle callback and aggregates changes. It's a bit complex with threading,
> though... do you confirm the codegen is fine for our usage?

No idea. I'll look.

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.

>  * To support user-defined contact groups too, additionally implement
>  * %TP_TYPE_CONTACT_GROUP_LIST in the #TpBaseContactList subclass, add the
>  * %TP_IFACE_CONNECTION_INTERFACE_CONTACT_GROUPS1 interface to the output of
>- * #TpBaseConnectionClass.get interfaces_always_present, and implement the
>- * %TP_TYPE_SVC_CONNECTION_INTERFACE_CONTACT_GROUPS1 in the #TpBaseConnection
>- * subclass using tp_base_contact_list_mixin_groups_iface_init().
>+ * #TpBaseConnectionClass.get interfaces_always_present.

That function no longer exists. I think you can just end the sentence after "subclass"?

> +           _tp_gdbus_connection_interface_contact_groups1_emit_groups_changed (
> +                self->priv->contact_groups_skeleton,
> +                g_variant_new_fixed_array (G_VARIANT_TYPE_UINT32,
> +                    members_arr->data, members_arr->len, sizeof (TpHandle)),

A static assertion is desirable here (again); or we could add tp_handle_set_to_variant().

> -          g_hash_table_insert (map, GUINT_TO_POINTER (handle),
> -              (gchar *) tp_handle_inspect (self->repo, handle));
> +          /* We don't bother dupping the strings: they remain valid as long as
> +           * the connection's alive and hence the repo exists. */
> +          g_variant_builder_add (&builder, "{u&s}", handle,
> +              tp_handle_inspect (self->repo, handle));

I would be suspicious of that reasoning (although it matches current behaviour, which might well have been written by my younger and less defensive self), but it isn't actually true, because C-to-GVariant always copies strings anyway and the & is meaningless.

+  gchar *object_path;
...
+  g_object_get (chan,
+      "object-path", &object_path,
+      "channel-properties", &properties,
+      NULL);
...
+  g_variant_builder_add (builder, "(&o@a{sv})", object_path, properties);
+  g_variant_unref (properties);

The "&" doesn't do anything, and the object path is leaked.

*** Bug 77287 has been marked as a duplicate of this bug. ***

> TpExportableChannel, TpChannelIface: remove

This is Bug #77287, which I closed as a duplicate of this one.

Definitely remove TpChannelIface, it's crap.

I'm still a little unsure about removing TpExportableChannel - it makes telepathy-glib more tightly-coupled. OTOH, ever since TpBaseChannel was added, we've never had any reason to implement a channel that is not a TpBaseChannel. So, yeah, let's go for it. We can always add parallel APIs (or relax assertions/casts) in the unlikely event that this turns out to have been a mistake.

> + * This base class makes it easier to write channels
>   * implementations by implementing some of its properties, and defining other

"channel implementations"

> - * If the #TpExportableChannel:object-path property is not set at construct
> + * If the #TpBaseChannel:object-path property is not set at construct

When we turn it into a GDBusObjectSkeleton, I wonder whether we should switch to using GDBusObjectSkeleton:g-object-path. It's read/write/construct but not construct-only, FWIW.

> +   * If the channel implementation uses
> +   * <link linkend="telepathy-glib-dbus-properties-mixin">TpDBusPropertiesMixin</link>,
> +   * this property can implemented using
> +   * tp_dbus_properties_mixin_make_properties_hash() as follows:

Hmm, this is interesting. That won't necessarily work any more once you have switched the channel to be a GDBusObjectSkeleton, because not all the properties will necessarily be TpDBusPropertiesMixin-mediated...

I think we'll need a vfunc that fills a GVariantDict, and a note that overriding the vfunc is a better way to do things than overriding the property.

We can cross that bridge when we come to it, though.

> +  if (TP_IS_PRESENCE_MIXIN (self))
> +    {
> +      _tp_presence_mixin_init (self);
> +    }

I'm still unsure about coupling the Presence1 mixin to TpBaseConnection. OTOH presence is a fairly fundamental thing in IM... I do appreciate the simplification, though.

I think I'd still prefer it if the API was more like (Python-like pseudocode)

    def constructed(self):
        super.constructed()

        self.__presence_mixin = Tp.PresenceMixin.new()

    def fill_contact_attributes(self, *args):
        if self.__presence_mixin.fill_contact_attributes(self, *args):
            return
        ...

which I think would be enough to avoid having any glue in TpBaseConnection at all?

Your TpPresenceMixin(Interface) would have to be renamed to TpPresenceMixinConnection(Interface) or TpPresenceMixable(Interface) or something though.

- * @obj: An object with this mixin.
+ * @self: A #TpBaseConnection implemeting #TpPresenceMixinInterface

> -   * g_bus_get_sync() just after it is constructed; if this fails, this
> +   * g_bus_get_sync() just after it is ; if this fails, this

Typo, or deliberate?

 /**
  * TpPresenceMixinGetContactStatusFunc:
- * @obj: An object with this mixin.
+ * @self: A #TpBaseConnection implemeting #TpPresenceMixinInterface

Is this introspectable, or should it take a TpPresenceMixin *?

(If it isn't introspectable, maybe we should change TpBaseContactList while we still can.)

> + * @statuses: An array of #TpPresenceStatusSpec structures representing all
> + *  presence statuses supported by the protocol, terminated by a NULL name.

I don't think this is introspectable. A vfunc returning the same thing would be better.

+/* For some reason g-i wants that name */
+typedef struct _TpPresenceMixinInterface TpPresenceMixin;

It should be "typedef struct _TpPresenceMixin TpPresenceMixin" (an incomplete struct type that is declared but never defined).

(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

(In reply to comment #14)
> 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 is already not guaranteed to be 32-bit. It's a guint, not a guint32, because that was dbus-glib's API.

If you want to *change* TpHandle to a guint32 in Tp1, we could do that... then if the static assertion fails on some platform (which is unlikely, tbh), we have to be very careful at the telepathy-glib <-> dbus-glib boundary, which is non-type-safe.

That's probably the best approach despite the lack of type-safety - we're trying to eradicate GArray<guint> from our API in any case, and the static assertion will at least give us some early-warning.

(In reply to comment #15)
> (In reply to comment #14)
> > 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 is already not guaranteed to be 32-bit. It's a guint, not a
> guint32, because that was dbus-glib's API.
> 
> If you want to *change* TpHandle to a guint32 in Tp1, we could do that...
> then if the static assertion fails on some platform (which is unlikely,
> tbh), we have to be very careful at the telepathy-glib <-> dbus-glib
> boundary, which is non-type-safe.
> 
> That's probably the best approach despite the lack of type-safety - we're
> trying to eradicate GArray<guint> from our API in any case, and the static
> assertion will at least give us some early-warning.

dbus_g_value_build_g_variant() does:
  else if (type == G_TYPE_UINT)
    return g_variant_new_uint32 (g_value_get_uint (value));

So we already lost in platforms where guint != guint32. No? I would add that static assert that guint is 32bits and say "sorry, no telepathy for you" to any other platforms. I would be surprised glib itself works on them tbh.

(In reply to comment #16)
> dbus_g_value_build_g_variant() does:
>   else if (type == G_TYPE_UINT)
>     return g_variant_new_uint32 (g_value_get_uint (value));
> 
> So we already lost in platforms where guint != guint32. No?

No, that's fine, as long as the *value* fits in 32 bits. (If it doesn't, it'll be reduced modulo 2**32.)

> I would add that
> static assert that guint is 32bits and say "sorry, no telepathy for you" to
> any other platforms. I would be surprised glib itself works on them tbh.

Platforms where int < 32 bits are clearly obsolete; as a matter of policy, "the GNU system" (i.e. mostly GNU libc, in practice) doesn't work on them either. I think ignoring those is fine.

There are good practical reasons why platforms where int > 32 bits haven't happened (if int is > 32 bits and char is 8 bits then there is either no C primitive type of length 16, or none of length 32, because short can't be both) so I'm happy with having a static assertion rather than actually fixing it, but I don't really want our API to rely on "int will be exactly 32 bits long, forever".

oh right.

(In reply to comment #12)
> > +   * If the channel implementation uses
> > +   * <link linkend="telepathy-glib-dbus-properties-mixin">TpDBusPropertiesMixin</link>,
> > +   * this property can implemented using
> > +   * tp_dbus_properties_mixin_make_properties_hash() as follows:
> 
> Hmm, this is interesting. That won't necessarily work any more once you have
> switched the channel to be a GDBusObjectSkeleton, because not all the
> properties will necessarily be TpDBusPropertiesMixin-mediated...
> 
> I think we'll need a vfunc that fills a GVariantDict, and a note that
> overriding the vfunc is a better way to do things than overriding the
> property.
> 
> We can cross that bridge when we come to it, though.

I have WIP code here that makes TpDBusPropertiesMixin's getters return GVariant and tp_dbus_properties_mixin_make_properties_hash() takes a GVariantDict, as does TpBaseChannel::fill_immutable_properties(). Did not get to the point where it all builds though. Stay tuned.

http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=wip-gdbus-all&try=again

Known issues:

CMs, MC not ported yet.

"TpPresenceMixin: redo the API to be introspectable" I'm not sure about. It's a bit of a megapatch and leads to some annoying boilerplate in the CMs. I'm not sure we can do much better and still be introspectable, though. It does *nearly* (but not quite!) fix Bug #12896...

The self-handle test is still intermittently failing - there must be a race condition.

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

(In reply to comment #10)
> That function no longer exists. I think you can just end the sentence after
> "subclass"?

Appears to be OK at the end of the branch.

> "channel implementations"

Fixed

(In reply to comment #13)
> I think I'd still prefer it if the API was more like [...]
> which I think would be enough to avoid having any glue in TpBaseConnection
> at all?

Now that the functions take a TpPresenceMixin *, it's:

    def constructed(self):
        Tp.PresenceMixin.init(self)
        # or possibly even self.init() if nothing else collides

    def fill_contact_attributes(self, *args):
        if Tp.PresenceMixin.fill_contact_attributes(self, *args):
            return
        ...

which I think is as minimal as we're going to get without coupling TpBaseConnection to TpPresenceMixin. I think that's good enough.

> > -   * g_bus_get_sync() just after it is constructed; if this fails, this
> > +   * g_bus_get_sync() just after it is ; if this fails, this

Reverted

>  /**
>   * TpPresenceMixinGetContactStatusFunc:
> - * @obj: An object with this mixin.
> + * @self: A #TpBaseConnection implemeting #TpPresenceMixinInterface
> 
> Is this introspectable, or should it take a TpPresenceMixin *?

Changed it to take a TpPresenceMixin, and fixed s/implemeting/implementing/

> (If it isn't introspectable, maybe we should change TpBaseContactList while
> we still can.)

*** not yet done *** but it's off the critical path for this branch

> > + * @statuses: An array of #TpPresenceStatusSpec structures representing all
> > + *  presence statuses supported by the protocol, terminated by a NULL name.
> 
> I don't think this is introspectable. A vfunc returning the same thing would
> be better.

Done on a separate branch but it leads to some annoying changes to CMs. I'm going to defer this to Bug #71508.

> It should be "typedef struct _TpPresenceMixin TpPresenceMixin" (an
> incomplete struct type that is declared but never defined).

Fixed.

(In reply to comment #21)
> >  - "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 ***

Also done now. I think this is everything needed for the critical-path for this bug.

tp_handle_set_new_from_variant(), and making TpBaseContactList take a different type in its vfuncs, would be good to have before 1.0 but should not block merging this particular branch.

Implementation experience from Gabble (port not finished yet, so there could be more):

* tp_base_connection_dup_contact_attributes() needs to become public API
  again, because Gabble uses it for Addressing

* The TpBaseContactList should not export the Blocking1 interface until
  the connection goes CONNECTED, at which point it should call the
  can_block() vfunc to decide whether to export it or not

(In reply to comment #24)
> * The TpBaseContactList should not export the Blocking1 interface until
>   the connection goes CONNECTED, at which point it should call the
>   can_block() vfunc to decide whether to export it or not

Actually this is harder than I thought, because telepathy-glib wants to forbid adding interfaces after CONNECTED, but priv->status is changed before emitting status-changed (and CMs could conceivably be relying on that).

Wow, big branch.

http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=gdbus-object
(all commits since next - there's more than one page, but some of them are yours)

http://cgit.freedesktop.org/~smcv/telepathy-gabble/log?h=gdbus-object
http://cgit.freedesktop.org/~smcv/telepathy-salut/log?h=gdbus-object
http://cgit.freedesktop.org/~smcv/telepathy-idle/log?h=gdbus-object
http://cgit.freedesktop.org/~smcv/telepathy-haze/log?h=gdbus-object&try=again
http://cgit.freedesktop.org/~smcv/telepathy-rakia/log?h=gdbus-object
http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log?h=gdbus-object

I don't expect that Folks or Empathy will need changes outside regression tests. TPAW shouldn't need any changes at all.

(In reply to comment #23)
> tp_handle_set_new_from_variant(), and making TpBaseContactList take a
> different type in its vfuncs, would be good to have before 1.0 but should
> not block merging this particular branch.

Bug #77772, Bug #77773

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

Bug #77774

(In reply to comment #26)
> http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=gdbus-object
> (all commits since next - there's more than one page, but some of them are
> yours)

 - 0b57d9e66ca25a5e6684751a23a88b91e719e3c6 can be removed since it's replaced by 22ed228270e7e29e7ede985a198d5c675464be51

 - I have to admit I'm totally lost with TpDBusPropertiesMixin. We never implement TpSvcDBusProperties anymore, right? So _tp_dbus_connection_gather_tp_svc_skeletons() could warn if it sees that iface? We could use the "invisible mixin" in all CMs to kill tp_dbus_properties_mixin_class_init() and all code paths where offset!=0? I'm happier now that it is in -dbus because it should really be considered deprecated and we should try to remove it completely from all CMs on the long term.

 - tbh, I don't really like TpPreseceMixin using qdata to find the skeleton, I was happy to remove that kind of hack from TpBaseContactList. But if you prefer that way, I've no strong argument to stop you from doing it. Note that the TpBaseConnectionPrivate struct can move back to the .c instead of -internal.h now if you want.

This is a huge step forward, thanks !

(In reply to comment #29)
>  - 0b57d9e66ca25a5e6684751a23a88b91e719e3c6 can be removed since it's
> replaced by 22ed228270e7e29e7ede985a198d5c675464be51

I don't think an extra static assertion hurts, tbh.

>  - I have to admit I'm totally lost with TpDBusPropertiesMixin.

It implements the properties of all TpSvc* interfaces. It does not implement the properties of non-TpSvc* interfaces.

Here is how it normally works:

* method call comes in
* dispatched to a GDBusInterfaceSkeleton
* is it a TpSvcInterfaceSkeleton?
  - yes:
    * is it a property Get/Set/GetAll?
      - yes:
        * does it have a TpDBusPropertiesMixin?
          - yes: call into it
          - no: method call fails
      - no: call into TpSvcWhateverClass vfuncs
  - no: call into GDBusInterfaceSkeleton vfuncs

If the object implements TpSvcDBusProperties - for which the only remaining use is McdDBusProp - then that whole logic is bypassed and we use the TpSvcDBusProperties vtable instead. In the case of McdDBusProp, the object doesn't have a TpDBusPropertiesMixin either.

>  - I have to admit I'm totally lost with TpDBusPropertiesMixin. We never
> implement TpSvcDBusProperties anymore, right?

Wrong. Mission Control has its own implementation of TpSvcDBusProperties, because it wants to be able to set properties that are read-only at the D-Bus level, at (or in fact just after) construct time, as if they were read/write/construct-only, by using their D-Bus names (and without duplicating their setter logic).

Eventually, I would like MC to do something better than McdDBusProp, but now is not the time.

I think TpSvcDBusProperties should be a normal interface (for symmetry with its client-side equivalent, if nothing else) but nothing in telepathy-glib should implement it, and eventually, nothing in *Telepathy* should implement it.

> We could use the "invisible mixin" in all CMs to kill
> tp_dbus_properties_mixin_class_init() and all code paths where offset!=0?
> I'm happier now that it is in -dbus because it should really be considered
> deprecated and we should try to remove it completely from all CMs on the
> long term.

Feel free to clone a bug. I think I might actually have eradicated offset != 0 while updating the CMs (it seemed cleaner to use the invisible mixin than to include telepathy-glib-dbus.h in their headers) but this is way off the critical path for a clean and future-proof libtelepathy-glib-1.so API.

>  - tbh, I don't really like TpPreseceMixin using qdata to find the skeleton,
> I was happy to remove that kind of hack from TpBaseContactList. But if you
> prefer that way, I've no strong argument to stop you from doing it. Note
> that the TpBaseConnectionPrivate struct can move back to the .c instead of
> -internal.h now if you want.

I do prefer that: I think it's better than having individual interfaces' mixins tightly-coupled to the base class. In particular, if we release 1.0 with an unintrospectable TpPresenceMixin, we can deprecate it and add TpPresenceMixin2 without leaving detritus in TpBaseConnection.

Yes, I would like to move the private struct back to the .c at some point, but it isn't critical-path.

ok good with me, please merge this branch :)

Fixed in git for 0.99.11, which I should release before we merge anything else!