[Bug 29375] add something resembling GabbleBaseChannel

[bugzilla-daemon at freedesktop.org]

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

--- Comment #17 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-24 02:55:51 PDT ---
>   * 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?

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