[Bug 25236] TpBaseClient - an abstract class to implement clients

[bugzilla-daemon at freedesktop.org]

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

--- Comment #18 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-26 05:40:55 PDT ---
The Observer needs documentation; Guillaume is working on it already.

> +test_basis (Test *test,

Do you mean test_basics? :-)

> +      "/org/freedesktop/Telepathy/Account/fake",
> +      "/org/freedesktop/Telepathy/Connection/fake",

I'd prefer a syntactically valid object path (".../fake/fake/fake" would be
sufficient).

> +  param_spec = g_param_spec_pointer ("dbus-context", "D-Bus context",
> +      "The DBusGMethodInvocation associated with the ObserveChannels call",
> +      G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS);

I think this should be write-only - once the TpOCC has been constructed, it's
responsible for replying to the DBusGMI, and nobody else can do so safely. If
the client wants to find out the caller's unique name (which is the only other
thing you do with a DBusGMI, in practice), we can add tp_o_c_c_get_caller().

> +tp_observe_channels_context_accept (TpObserveChannelsContext *self)

I'd prefer it if this, and fail(), had a check for "already done that" -
probably a g_return_if_fail. They should NULL out priv->dbus_context when
called (because replying frees the context).

Now that you've added the concept of status later in the branch, you can use
that for the check.

> +    g_type_add_class_private (g_define_type_id, sizeof (
> +        TpBaseClientClassPrivate));)

Nitpicking: this introduces an empty statement, which causes some compilers to
warn. Get rid of the trailing semicolon to fix that.

> +      DEBUG ("ObserveChannels has not be implemented");

"not been implemented", and I'd prefer "class %s does not implement
ObserveChannels" % G_OBJECT_TYPE_NAME (self)

> +    g_warning ("Implementation of ObserveChannels didn't call "
> +        "tp_observe_channels_context_{accept,fail,delay}");

I'd like a G_OBJECT_TYPE_NAME here, and it should probably be a g_critical();
also, it should probably reply on D-Bus, perhaps with NotImplemented or
NotAvailable (we don't have a NotImplementedCorrectly error yet).

> +  [glib-2.0 >= 2.24, gobject-2.0 >= 2.22, gio-2.0 >= 2.22])

These three come from the same source tarball, so there's no point in not
bumping all three versions; I think it's more explicit / clearer if you keep
all three the same. Please also bump the versions in the pkg-config?

>    TpBaseClientClassPrivate *klass_pv = G_TYPE_CLASS_GET_PRIVATE (

I'd prefer class_priv, or possibly cls_priv if you renamed klass to cls. The
only reason to mis-spell "class" is to be nice to C compilers that are actually
a C++ compiler in disguise.

> +      tp_value_array_unpack (g_ptr_array_index (channels_arr, i), 2,
> +          &chan_path, &chan_props);
> +
> +      channel = tp_channel_new_from_properties (connection,
> +          chan_path, chan_props, &error);
> +      if (channel == NULL)

This leaks the path and properties. I'd free and unref them (respectively)
after creating the Channel, but before checking whether it's NULL.

> +struct _TpObserveChannelsContext {
...
> +  /* List of reffed TpChannelDispatchOperation */
> +  TpChannelDispatchOperation *dispatch_operation;

I think this comment is a lie - it should be "reffed, or NULL".

> +  GPtrArray *requests;

Deserves a comment.

> +  channels = g_ptr_array_new_with_free_func (g_object_unref);

Perhaps use g_ptr_array_sized_new (channels_arr->len) and call
g_ptr_array_set_free_func immediately after, to save reallocs? The same for the
ChannelRequests.

>  tp_observe_channels_context_prepare_async (TpObserveChannelsContext *self,
...
>    g_return_if_fail (self->priv->result == NULL);

I think prepare_async, prepare_finish could be internal, in which case this
assertion would be acceptable (library users could never hit it). Otherwise,
it's inappropriate.

> +      if (!tp_proxy_is_prepared (channel, TP_CHANNEL_FEATURE_CORE))
> +        return FALSE;

I wonder whether we should also guarantee GROUP.

> -#include <telepathy-glib/base-client-context-internal.h>
> +#include <telepathy-glib/observe-channels-context-internal.h>

I'd prefer it if you kept this block of headers (in base-client.c) in
alphabetical order (rationale: makes it more likely that conflicts have a
trivial resolution).

> Disable Approver and Handler support for now

It might be cleaner to do this as a deletion of code, and git revert it when
working on A and H code.

I don't know whether A or H is better to do first. Perhaps before doing either,
it would be worth doing a TpSimpleObserver that loosely resembles Danielle's
TpHandler (i.e. you instantiate it and give it a list of filters, a function
pointer for the callback, and some sort of user_data for the callback, rather
than having to subclass it: Danielle's rationale is that subclassing in
GObject/C is unnecessarily hard).

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