Bug 31631

(In reply to comment #0)
> A) tp_base_client_set_channel_factory() is broken. This is fixed in
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=9deb3dcad3a60f43d6367296ae32f2b26c4103a6

r+

> B) The 'self' pointer passed to method implementation is wrong.
> -    return iface->dup_channel_features (iface, channel);
> +    return iface->dup_channel_features (self, channel);
> solves it but it seems that something is wrong.
> 
> This cast is suspicious, self is supposed to already be a
> TpClientChannelFactoryInterface; so why do we need to cast it?

Ugh, I should have spotted this at review. The type of the method is wrong; we've screwed up the declarations.

TpClientChannelFactory* is a pointer to an object that implements the ClientChannelFactory interface, which would be useful for potentially-stateful instance methods.

TpClientChannelFactoryInterface* is a pointer to the interface's vtable (class-like struct), which isn't actually useful.

This means that you can't usefully implement any client channel factory whose methods look at their first argument.

Proposed solution: add obj_create_channel and obj_dup_channel_features methods that do what we intended; have their default implementations chain to the current (wrong) ones; in telepathy-glib 1.0, remove the current methods and remove the obj_ prefix from the corrected versions.

> And I'm wondering if the TP_CLIENT_CHANNEL_FACTORY macro really makes sense as
> that's an iface and not an object?

14:10 < cassidy> [...] According 
                 to the doc, G_TYPE_CHECK_INSTANCE_CAST should only be used in 
                 type implementation, so I think we shouldn't have used it here

I think a GInterface counts as a type: GAsyncResult has this usage, for instance. Our macros are analogous to GAsyncResult's, except that TP_CLIENT_CHANNEL_FACTORY is implemented wrong.

The semantics of TP_CLIENT_CHANNEL_FACTORY(obj) are (meant to be): if obj is non-NULL and its GType implements ("is-a") ClientChannelFactory, return it, cast to TpClientChannelFactory*; else critical and return NULL.

However, TP_CLIENT_CHANNEL_FACTORY actually casts the instance pointer to TpClientChannelFactoryInterface, which is invalid - the instance doesn't start with a struct _TpClientChannelFactoryInterface, so the fields in the returned struct will be meaningless (they'll just alias whatever's at the beginning of struct _GObject).

At some point I should enhance tools/gobject-foo.py to produce correct GInterfaces with the same names that G_DEFINE_INTERFACE would use.

16:05 < smcv> we could probably fix it without API breaks, but only with a 
              liberal scattering of gpointer
16:06 < smcv> or we could just go "clearly nobody's using it yet, and 
              typesafety is more important" and break API while keeping ABI
16:07 < cassidy> I'd vote for that

Here's a branch to do that.

Looks good, sorry for this mess.

Maybe open a bug regarding the deprecated API so we won't forget about it for 1.0?

Fixed in git for 0.13.6; does not affect 0.12.