Bug 29375

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.

As a way to test it, the same branch should port the example CMs in examples/ to use the base class (except for contactlist, which has its channels deleted by my pending contact-lists branch anyway). This is hopefully an exercise in mass code-deletion :-)

I've started implementing this on my base-channel branch here: http://git.collabora.co.uk/?p=user/jonathon/telepathy-glib;a=shortlog;h=refs/heads/base-channel

So far I've ported a single example CM (the 'echo' example), which shows a nice trend:
examples/cm/echo/chan.c |  381 +++++------------------------------------------
 examples/cm/echo/chan.h |   10 +-
 2 files changed, 39 insertions(+), 352 deletions(-)

If you have some time, I would appreciate a quick 'pre-review' before I finish porting all of the other examples.

(In reply to comment #3)
> I've started implementing this on my base-channel branch here:
> http://git.collabora.co.uk/?p=user/jonathon/telepathy-glib;a=shortlog;h=refs/heads/base-channel
> 
> So far I've ported a single example CM (the 'echo' example), which shows a nice
> trend:
> examples/cm/echo/chan.c |  381 +++++------------------------------------------
>  examples/cm/echo/chan.h |   10 +-
>  2 files changed, 39 insertions(+), 352 deletions(-)
> 
> If you have some time, I would appreciate a quick 'pre-review' before I finish
> porting all of the other examples.

You're missing a way to set 'interfaces' on an instance, now that it's in a Priv structure. Possibly others, I haven't checked; and maybe I missed the adding of a setter.

(I actually would lean towards not making these private, because it saves so much typing in subclasses which frequently access, say, the target handle. But maybe this is anathema? I was just thinking “hey I'm glad these fields are public” while porting a Ring channel to use the version copied from Gabble earlier today... ☺)

Looks like the right idea to me otherwise :)

(In reply to comment #4)
> You're missing a way to set 'interfaces' on an instance, now that it's in a
> Priv structure. Possibly others, I haven't checked; and maybe I missed the
> adding of a setter.

Oh, you're right.  I didn't add any setters since I figured they were all available as gobject properties, but now that I look back at the code, several of the properties ("interfaces" for instance) are read-only.  I definitely need to fix that.

> (I actually would lean towards not making these private, because it saves so
> much typing in subclasses which frequently access, say, the target handle. But
> maybe this is anathema? I was just thinking “hey I'm glad these fields are
> public” while porting a Ring channel to use the version copied from Gabble
> earlier today... ☺)

hm.  is it really going to save that much typing?  I'm a bit hesitant to expose too much in the struct itself.

In discussing it with sjoerd a bit, we've come to think that it might be nicer if the list of properties for 'channel-properties' is provided by a vfunc so that subclasses don't need to override the 'channel-properties' property, since that seems potentially a bit fragile.  I need to ponder the design options there a bit.

(In reply to comment #4)
> You're missing a way to set 'interfaces' on an instance, now that it's in a
> Priv structure. Possibly others, I haven't checked; and maybe I missed the
> adding of a setter.
> 

after a quick discussion on IRC, we noticed that nobody actually uses this feature and it isn't all that useful, so I decided to only support setting the list of interfaces on a per-class basis rather than either per-class OR per-object.  

So I pushed this up to my base-channel branch along with some other changes, and I think it's now ready for a thorough review.  see http://git.collabora.co.uk/?p=user/jonathon/telepathy-glib;a=shortlog;h=refs/heads/base-channel

Looks good, functionally! A few nit-picks:

+  if (priv->conn != NULL)
+    {
+      g_object_unref (priv->conn);
+      priv->conn = NULL;
+    }
+

If you're feeling keen, this could use tp_clear_object(). If not, no worries.

+          /* create an empty properties hash for subclasses to fill */
+          GHashTable *properties =
+            tp_dbus_properties_mixin_make_properties_hash (G_OBJECT (chan), NULL, NULL, NULL);
+          if (klass->add_properties)
+            klass->add_properties (chan, properties);
+
+          g_value_take_boxed (value, properties);

Coding style: there should be an empty line before the 'if'.

I'm off to port some Ring code to use this branch and see how I feel about using it. :)

This branch doesn't pass `make check` with gtk-doc enabled. This could be to do with the new stuff I'm adding; I'll fix it up.

(In reply to comment #10)
> This branch doesn't pass `make check` with gtk-doc enabled. This could be to do
> with the new stuff I'm adding; I'll fix it up.

I didn't fix this up, because you did. :)

But I did fix up a few broken links, tweak some documentation, and add a bunch of accessors: <http://git.collabora.co.uk/?p=user/wjt/telepathy-glib.git;a=shortlog;h=refs/heads/base-channel>

I ported Ring's media channel to this, and it's pretty nice. The immutable property vfunc works really well.

ExampleEchoChannel constructor:
> +  g_object_get (object, "connection", &conn, NULL);

Should use tp_base_channel_get_connection().

(Also, the coding style is a bit off; please indent g_object_get like the next bit I quote, instead.)

example_echo_channel_closed:
> +  g_object_get (self,
> +          "channel-destroyed", &closed,
> +          NULL);

Could we have a tp_base_channel_is_destroyed()?

> +  g_object_get (self,
> +      "handle", &target,
> +      NULL);

Should use tp_base_channel_get_target()

I think I'd prefer target_type to be target_handle_type, and get_target() to be get_target_handle().

> + * Since: 0.11.12

This is untrue; 0.11.12 has been and gone. All new code should use Since: 0.11.UNRELEASED (with the capitalization), and during the release process, the maintainer (i.e. usually me) replaces those with the real version.

> +tp_base_channel_register (TpBaseChannel *chan)

Please add a boolean to check that the channel hasn't already been registered (see TpBaseClient for an example). Anything else that can only be called before registering (I'm not sure if there is actually any such function) should check the same boolean.

> + * Returns: (transfer none): the target handle of @chan

The transfer annotation isn't applicable to integers, so drop the (transfer none), and say it via the text of the doc-comment instead. Likewise for tp_base_channel_get_initiator().

> +tp_base_channel_is_requested (TpBaseChannel *chan)
> +{
> +  g_return_val_if_fail (TP_IS_BASE_CHANNEL (chan), 0);

I'd prefer FALSE.

> +static void
> +tp_base_channel_add_properties (TpBaseChannel *chan, GHashTable *properties)

I'd prefer this to have a name that doesn't suggest that it's the "call the right virtual function" wrapper for add_properties(); tp_base_channel_basic_add_properties(), perhaps?

I don't think requiring subclasses to chain up is language-binding-friendly? I'm not sure what we can do about it, though...

> +  TpBaseConnection *conn = (TpBaseConnection *) chan->priv->conn;

The cast is unnecessary.

> +    case PROP_OBJECT_PATH:
> +      g_free (chan->priv->object_path);
> +      chan->priv->object_path = g_value_dup_string (value);

Isn't this construct-only? If so, you can just assert that it's initially NULL.

> +  if (priv->conn != NULL)
> +    {
> +      g_object_unref (priv->conn);
> +      priv->conn = NULL;
> +    }

This could usefully be tp_clear_object (&priv->conn);.

> +  param_spec = g_param_spec_string ("initiator-id", "Initiator's bare JID",

"Initiator's identifier", this isn't Gabble :-)

15:32 < wjt> smcv: currently object-path is a construct-only property. I was 
             porting various gabble classes to it... and this means that the 
             channel can't decide its own object path, without overriding that 
             property (which breaks stuff in base-channel.c which assumes that 
             priv->object_path is meaningful)
15:33 < wjt> smcv: possible fixes include: don't do that; make everything in 
             base-channel.c use g_object_get() to get the property; add a vfunc 
             called from base_channel_class->constructed() which allows the 
             subclass to pick its object path
15:34 < wjt> smcv: </braindump of the thing jonner and i are on the fence about>
15:35 < smcv> wjt: my vote would be the vfunc, only called if the property is 
              still NULL after setting construct-time properties, but I'll look 
              at the implementation first
15:35 < smcv> I actually think having the channel manager assign the object 
              path is better in any case though
15:35 < barisione> wjt: g_object_get + overriding the property?
15:36 < jonner> well the other option is overriding constructor, I guess, but 
                that's not that nice either.
15:36 < barisione> vfunc seems saner to me (without knowing the code)
15:36 < wjt> smcv: counter-example: GabbleRoomListChannel. it currently smushes 
             its address in memory into the path; there's nothing else about 
             the channel that uniquely identifies it
15:36 < smcv> wjt: mm, true
15:36 < wjt> smcv: as we stand the channel manager could invent a counter
15:36 < smcv> no you're right, that would be rubbish
15:37 < wjt> well that's what we do for streamed media channels. :þ
15:37 < smcv> better to have the channel identify itself
15:37 < smcv> yeah, I always thought that was a bit of a code-smell

I still think the way forward here is to have a vfunc that's called from constructed() if the caller of g_object_new left object-path as NULL. It could either return the entire object path, or preferably, just the thing to append to (the Connection's object path + "/").

Ideally, the default implementation in TpBaseChannel would put ("%p" % self) through tp_escape_as_identifier, or use ("addr%" G_GSIZE_MODIFIER "x" % GPOINTER_TO_SIZE (self)), or something, so that it would always generate unique object paths without any further intervention.

(In reply to comment #12)
> example_echo_channel_closed:
> > +  g_object_get (self,
> > +          "channel-destroyed", &closed,
> > +          NULL);
> 
> Could we have a tp_base_channel_is_destroyed()?

ALF: http://git.collabora.co.uk/?p=user/wjt/telepathy-glib.git;a=commit;h=372ea20484d0f416e30af8f92accf120a329c0f4

> > + * Returns: (transfer none): the target handle of @chan
> 
> The transfer annotation isn't applicable to integers, so drop the (transfer
> none), and say it via the text of the doc-comment instead. Likewise for
> tp_base_channel_get_initiator().

I was sure I saw some other bits of code annotating TpHandle params thus. But I can't find them now.

> I still think the way forward here is to have a vfunc that's called from
> constructed() if the caller of g_object_new left object-path as NULL. It could
> either return the entire object path, or preferably, just the thing to append
> to (the Connection's object path + "/").

The latter is what I proposed on Friday. :)

(Those review comments were relative to wjt's version of the branch, so some of them have already been fixed.)

The echo CM has been moved to tests/lib in master because it's not actually a good example any more (it doesn't have Messages), so I'd prefer it if you could also port echo-message-parts, aka echo2.

Ideally, port all of them, of course (except for contact-list, which is going to have its channels deleted when my TpBaseContactList branch lands).

If this branch lands in master before TpBaseContactList, which seems likely, then I'll port the hidden internal channels in TpBaseContactList to use this, and there will be much rejoicing :-)

>   * TpBaseChannel:
> + *
>   * @parent: fields shared by the superclass

That doesn't look like the right syntax to me: member docs are meant to be immediately after the title line. What failure are you "fixing" with that?

I think what you actually want to do is to put /*<private>*/ at the beginning of the struct (making everything private), then remove the documentation of @parent.

FYI, I've pushed some new commits to my branch that should fix all of your review issues.  The only one outstanding yet is to port the echo-message-parts example CM, but I anticipate that will be finished and pushed up quite soon.

I've now pushed 3 new examples ported to TpBaseChannel (including echo-message-parts).  The last one (callable) introduces a test failure in 'make check' that I haven't been able to chase down yet, unfortunately.

>   * Subclasses should fill in #TpBaseChannelClass.channel_type,
> - * #TpBaseChannelClass.target_type and #TpBaseChannelClass.interfaces, and
> - * implement the #TpBaseChannelClass.close and
> + * #TpBaseChannelClass.target_handle_type and #TpBaseChannelClass.interfaces,
> + * and implement the #TpBaseChannelClass.close and
>   * #TpBaseChannelClass.fill_immutable_properties virtual functions.

Nitpicking: fill_immutable_properties is not mandatory (or if it's mandatory, it shouldn't be), so I'd prefer it to be mentioned in a subsequent paragraph instead, something like:

    To provide additional functionality, subclasses may also implement
    #TpBaseChannelClass.fill_immutable_properties.

> +static gchar *
> +tp_base_channel_get_object_path_suffix (TpBaseChannel *self)
> +{
> +  gchar * obj_path = g_strdup_printf ("channel%p", self);
> +  gchar * escaped = tp_escape_as_identifier (obj_path);
> +  g_free (obj_path);
> +  return escaped;
> +}

Please name this to indicate that it's the concrete base implementation rather than the "call the vfunc" wrapper (tp_base_channel_basic_get_object_path_suffix?).

Style: gchar *obj_path, etc., and a blank line between (initialized) declarations and other code please.

> +      gchar *base_path = klass->get_object_path_suffix (chan);
> +      g_assert (base_path != NULL && *base_path != '\0');

Blank line between these, please. Please also split up the assertion: "assert a; assert b" gives better assertion-failure messages than "assert a && b" (where you don't know which one failed).

> +typedef gchar* (*TpBaseChannelGetPathFunc) (TpBaseChannel *chan);

Nitpicking: gchar *(*TBCGPF)...

In echo-message-parts:

> +  received = tp_message_new (tp_base_channel_get_connection (
> +        TP_BASE_CHANNEL (self)), 1, len);

Perhaps put base = TP_BASE_CHANNEL (self) at the top of the function, then use @base thereafter? Likewise in the constructor. The other examples could probably benefit from this too.

I approve greatly of the massive code-deletion :-)

In callable (which obviously isn't appropriate to merge until tests pass, but unfortunately I can't see any obvious reason why they fail):

> +      dtmf_iface_init);)

The last G_IMPLEMENT_INTERFACE call in G_DEFINE_TYPE_WITH_CODE doesn't need a trailing semicolon, and some compilers will complain about the resulting empty statement.

>-      self->priv->initiator /* actor */, TP_CHANNEL_GROUP_CHANGE_REASON_NONE);
>+      tp_base_channel_get_initiator (TP_BASE_CHANNEL (self)) /* actor */,
>+      TP_CHANNEL_GROUP_CHANGE_REASON_NONE);

You could use base_chan here?

+static gchar *
+tp_base_channel_get_object_path_suffix (TpBaseChannel *self)
+{
+  gchar * obj_path = g_strdup_printf ("channel%p", self);
+  gchar * escaped = tp_escape_as_identifier (obj_path);
+  g_free (obj_path);
+  return escaped;
+}

I feel like the escape_as_identifier() is unnecessary... but then, I took a look around and can't find any specification for what %p should produce.

Lots of other code assumes that it'll produce something that's valid in an object path, though. :)

(In reply to comment #18)
> I feel like the escape_as_identifier() is unnecessary... but then, I took a
> look around and can't find any specification for what %p should produce.
> 
> Lots of other code assumes that it'll produce something that's valid in an
> object path, though. :)

Yeah, the output of %p is implementation-specific; a 64-bit platform would be entirely within its rights to output "0123:4567:8901:2345" rather than the conventional 0123456789012345.

You could use ("channel%" G_GSIZE_FORMAT "x", GPOINTER_TO_SIZE (self)) perhaps?

I've merged master to my copy of this branch, fixing the conflicts.

(In reply to comment #16)
> I've now pushed 3 new examples ported to TpBaseChannel (including
> echo-message-parts).  The last one (callable) introduces a test failure in
> 'make check' that I haven't been able to chase down yet, unfortunately.

I've chased this down. I've also chased down a lifecycle bug that broke my port of GabbleSearchChannel to TpBaseChannel. (How ironic: the most problematic channel implementation to port was the one for which I originally wrote GabbleBaseChannel. :))

I believe this branch is now ready to merge. I fixed the stuff mentioned in comment 17 and everything!

And we're done! Huge success. Will be in 0.11.14.

commit c4768dedea8ba56bcd36d03ab76c785036b0d24e
Merge: 3d1080f 1077016
Author: Will Thompson <will.thompson@collabora.co.uk>
Date:   Tue Aug 24 15:20:32 2010 +0100

    Merge branch 'base-channel'
    
    Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
    Fixes: <https://bugs.freedesktop.org/show_bug.cgi?id=29375>