[Bug 29375] add something resembling GabbleBaseChannel

[bugzilla-daemon at freedesktop.org]

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

--- Comment #12 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-23 09:06:51 PDT ---
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.

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