[Bug 29375] add something resembling GabbleBaseChannel

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-03 03:40:16 PDT ---
Some thoughts on the code:

In general I'd like fewer direct accesses to the object struct if this is going
to be library code (conn, object_path, interfaces, target, initiator, closed
should probably all be priv members with accessors).

The class struct needs considerable ABI padding.

> * Subclasses must implement the Close method on #TpSvcChannel (setting
> * #GabbleBaseChannel:closed to %TRUE when it is called). The default

If there's only one Channel method needed now, I'd make this an ordinary
virtual method, to make porting to GDBus easier. If it needs to be async, it
should probably be GAsyncResult-based rather than mentioning
DBusGMethodInvocation.

> The default
> * implementation for #TpExportableChannel:channel-properties just includes the
> * immutable properties from the Channel interface; subclasses will almost
> * certainly want to override this to include other immutable properties.

I've added tp_dbus_properties_mixin_fill_properties_hash(), which would be
useful for this. The BaseChannel could steal implementation ideas from
TpBaseProtocol.

> The
> * default implementation for #TpExportableChannel:channel-destroyed is simply
> * the value of #GabbleBaseChannel:closed; this should be fine for channels
> * that don't respawn.

As a Gabble implementation detail, setting the 'closed' struct member directly
is fine, but as a base class in a library, I'd prefer to put the struct member
in priv, and provide a non-virtual method (tp_base_channel_destroyed()?) that
emits Closed and simultaneously sets priv->closed.

For respawnable channels, we could offer a second method,
tp_base_channel_reopened() or some such, that you call instead, to emit Closed
without setting channel-destroyed? (Also document that you must implement
TpSvcChannelInterfaceDestroyable if you'll use that method.)

Perhaps the BaseChannel should always implement
TpSvcChannelInterfaceDestroyable, and close(_async) and destroy(_async) should
have the same signature, to facilitate using the same implementation for both
in the non-respawning case?

> They may also choose to override
> * #GabbleBaseChannel:requested, whose default implementation is "initiator ==
> * self_handle?".

I'd prefer to make requested mandatory at construct time, tbh. It's one more
line of very easy boilerplate for library users, but explicit is better than
implicit.

gabble_base_channel_register() currently uses tp_get_bus(). BaseChannel should
use tp_base_connection_get_dbus_daemon() and tp_dbus_daemon_register_object()
instead; tp_base_channel_destroyed() should use
tp_dbus_daemon_unregister_object().

The BaseChannel shouldn't have a pointer to its TpBaseConnection unless it will
do the refcounting correctly (object-lifetime assumptions in CMs are becoming
one of my pet hates). This probably means keeping a strong ref for the object's
whole lifetime, releasing it in dispose() and relying on the parent channel
manager to close and unref the channel on disconnection -
TpBaseContactListChannel in my contact-list branch does this.

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