[Bug 31631] Serious issues with TpClientChannelFactoryInterface

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-11-16 06:32:31 PST ---
(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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.