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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #19 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-04-27 02:09:43 PDT ---
(In reply to comment #18)
> The Observer needs documentation; Guillaume is working on it already.
> 
> > +test_basis (Test *test,
> 
> Do you mean test_basics? :-)

fixed.

> > +      "/org/freedesktop/Telepathy/Account/fake",
> > +      "/org/freedesktop/Telepathy/Connection/fake",
> 
> I'd prefer a syntactically valid object path (".../fake/fake/fake" would be
> sufficient).

Done.

> > +  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().

done.

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

done.

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

_tp_observe_channels_context_get_state is an internal API. Do you think I
should move the state attribute as a public one?

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

removed.

> > +      DEBUG ("ObserveChannels has not be implemented");
> 
> "not been implemented", and I'd prefer "class %s does not implement
> ObserveChannels" % G_OBJECT_TYPE_NAME (self)

fixed.

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

done.

> > +  [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?

done (I amend the commit).

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

renamed.

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

fixed.

> > +struct _TpObserveChannelsContext {
> ...
> > +  /* List of reffed TpChannelDispatchOperation */
> > +  TpChannelDispatchOperation *dispatch_operation;
> 
> I think this comment is a lie - it should be "reffed, or NULL".

fixed.

> > +  GPtrArray *requests;
> 
> Deserves a comment.

done.

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

done.

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

Already made it internal. :)

> > +      if (!tp_proxy_is_prepared (channel, TP_CHANNEL_FEATURE_CORE))
> > +        return FALSE;
> 
> I wonder whether we should also guarantee GROUP.

We could stick to CORE for now and add _set_channel_features () API later.
Or just add GROUP to be coherent with tp_channel_call_when_ready().
You call.

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

sorted.

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

Sure; but I prefer to do the deletion commit just before merging so I can
easily revert it right after and avoid lolo conflicts.

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

I think TpSimpleObserver is a good second step.
What about Recovering? How this API should deal with it?

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