While starting to use TpClientChannelFactoryInterface in Empathy I found 2 big issues with it. 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 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? TpClientChannelFactoryInterface *iface = TP_CLIENT_CHANNEL_FACTORY_GET_IFACE ( self); And I'm wondering if the TP_CLIENT_CHANNEL_FACTORY macro really makes sense as that's an iface and not an object?
(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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.