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

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Tue Apr 27 20:06:50 CEST 2010 

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

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|telepathy-bugs at lists.freede |guillaume.desmottes at collabo
                   |sktop.org                   |ra.co.uk

--- Comment #24 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-27 11:06:50 PDT ---
This looks as though it's on track. I think it's worth merging as soon as it
can support observers cleanly.

(In reply to comment #21) 
> Oh, I obviously wasn't reading thoroughly enough... I should change the example
> in the docs to use a const gchar *, then.

(Fixed in master, does not affect 0.10.)

Review
======

> tp_observe_channels_context_get_recovering

is_recovering(), perhaps?

> + * TpBaseClientClassObserveChannelsImpl:
...
> + * @dispatch_operation: a #TpChannelDispatchOperation or %NULL; the
> + * dispatch_operation is not garanteed to be prepared

"not guaranteed to" (same misspelling in @requests).

Annotations needed (where "..." means the docs you already have):

@channels: (element-type Tp.Channel): ...
@requests: (element-type Tp.ChannelRequest): ...
@dispatch_operation: (allow-none): ...

I'd be inclined to call the first parameter "client" or "observer" rather than
"self" here, but that's a matter of taste.

> + * tp_base_client_add_observer_filter:
...
> + * @filter: (transfer none) (element-type utf8 GObject.Value):

This seems to contain non-breaking spaces in the annotations, for some reason?
Is that what you intended?

(To see non-breaking spaces in vim, ":set list listchars=tab:»→,trail:…,nbsp:‽"
or some such.)

It would be nice to assert (with g_return_if_fail) that there is an
observe_channels implementation.

You should explicitly say that the client mustn't have been registered, and I
don't think this is really enough documentation (it should be possible to guess
more or less what's going on without referring to telepathy-spec). How about
this?

/**
 * tp_base_client_add_observer_filter:
 * @self: a #TpBaseClient
 * @filter: (transfer none) (element-type utf8 GObject.Value):
 * a %TP_HASH_TYPE_CHANNEL_CLASS
 *
 * Register a new channel class as Observer.ObserverChannelFilter.
 * The @observe_channels virtual method set up using
 * tp_base_client_implement_observe_channels() will be called whenever
 * a new channel's properties match the ones in @filter.
 *
 * This method may only be called before tp_base_client_register() is
 * called, and may only be called on objects that implement
 * @observe_channels.
 *
 * Since: 0.11.UNRELEASED
 */

> + * tp_base_client_take_observer_filter:
...
> + * @filter: (transfer container) (element-type utf8 GObject.Value):

This contains non-breaking spaces, and parameters that are stolen (like this
one) should be (transfer full).

Also, the entire function should really be (skip) (there's no point in it for
language bindings), but that breaks gtk-doc <= 1.14, so we can't do that until
1.15 comes out (or at least until we get a gtk-doc with patches from upstream
git into major distributions).

> + * Define the value of the Observer.Recover property.

That's not really enough. How about this?

 * Set whether the channel dispatcher should attempt to recover
 * this Observer if it crashes. (This is implemented by setting
 * the value of its Recover D-Bus property.)
 *
 * Normally, Observers are only notified when new channels
 * appear. If an Observer is set to recover, when it registers with
 * tp_base_client_register(), it will also be told about any channels
 * that already existed before it started.
 *
 * For Observers that are activatable as a D-Bus service, if the
 * Observer exits or crashes while there are any channels that match
 * its filter, it will automatically be restarted by service-activation.
 * 
 * This method may only be called before tp_base_client_register() is
 * called, and may only be called on objects that implement
 * @observe_channels.

> + * tp_base_client_register:
...
> + * Returns: %TRUE if the client has been registered.
> + * Methods that set the filters and other immutable state cannot be called
> + * after this one.

I'd prefer an example: "... state, such as
tp_base_client_add_observer_filter(), cannot ..."

> + * tp_base_client_register:
...
> + * Returns: %TRUE if the client has been registered.

I'd prefer this worded as "if the client was registered successfully" - your
wording makes it sound as if, if you call this method twice, the second call
will also return TRUE.

> +      g_warning ("Failed to register bus name %s\n", string->str);

No thanks. If registering the bus name failed, just raise the GError - it's up
to the caller what they do about it.

> +  path = g_strdup_printf ("/%s", g_strdelimit (string->str, ".", '/'));

g_strdelimit edits the string in-place. It's OK here because you don't re-use
it, but I'd prefer it to be obviously correct at a glance:

  path = g_strdup_printf ("/%s", string->str);
  g_strdelimit (path, ".", '/');

> +      case PROP_DBUS_DAEMON:
> +        g_value_set_object (value, self->priv->dbus);
> +        break;
> +      case PROP_NAME:

I vaguely prefer to have a blank line between cases in a switch, although it
doesn't really matter when the implementations are so small.

> +      case PROP_DBUS_DAEMON:
> +        self->priv->dbus = g_value_dup_object (value);

I'd prefer this case to start with:

  g_assert (self->priv->dbus == NULL);    /* construct-only */

to make it obvious why it isn't a potential leak. Similar for PROP_NAME.

> +#if 0
> +    case DP_APPROVER_CHANNEL_FILTER:

If you're going to #if these, you should do the same to their
TpDBusPropertiesMixinPropImpl and TpDBusPropertiesMixinIfaceImpl structs.
Otherwise, you'll be remotely-assertable (by retrieving the offending
properties).

I wonder whether there should be a property and/or accessor for the client's
bus name and object path? If so, they should be set during the constructor
based on name and uniquify, and then used as-is at register time (i.e. move
some code from register to constructor).

> +  TpBaseClientClassPrivate *cls_pv = G_TYPE_CLASS_GET_PRIVATE (
> +      TP_BASE_CLIENT_GET_CLASS (self), TP_TYPE_BASE_CLIENT,
> +      TpBaseClientClassPrivate);

Please avoid nonstandard abbreviations like "pv" where feasible (private things
are called private or priv).

Can we cache this in FooClass.priv in class_init, like the way we cache object
private structures in Foo.priv in the instance init? Then you could use
cls->priv->observe_channels_impl().

> +          "Implementation of ObserveChannels of %s didn't call "

Nitpicking: "Implementation of ObserveChannels in %s" would avoid the repeated
"of" while remaining grammatically correct.

> + * tp_observe_channels_context_delay:
> + * @self: a #TpObserveChannelsContext
> + *
> + * Called by #TpBaseClientClassObserveChannelsImpl to indicate that it
> + * implements the method in an async way. It has to ref the
> + * #TpObserveChannelsContext before calling this function and is responsible
> + * to call tp_observe_channels_context_accept once the call is done.

I'd prefer:

... an async way. The caller must take a reference to the
#TpObserveChannelsContext before calling this function, and is responsible for
calling either tp_observe_channels_context_accept() or
tp_observe_channels_context_fail() later.

If disposed in state NONE or DELAYED, the context should probably g_critical(),
and raise NotImplemented on D-Bus.

> + * If the "recovering" key in present in the Observer_Info hash table
> + * associated with this context, its value; %FALSE if the key is not present.
> + *
> + * Returns: value

That describes the mechanism, but not the purpose. I'd prefer:

 * If this call to ObserveChannels is for channels that already
 * existed before this observer started (because the observer used
 * tp_base_client_set_observer_recover()), return %TRUE.
 *
 * In most cases, the result is %FALSE.
 *
 * Returns: %TRUE for pre-existing channels, %FALSE for new channels

Perhaps ..._is_recovering() would be a better name? I don't know.

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

This deserves a comment, something like:

  /* This is only used once, by TpBaseClient, so for simplicity, we only
   * allow one asynchronous preparation */

Still to review
===============

I haven't reviewed the test or the SimpleClient yet.

Future work
===========

I think we should get Observers working nicely (including an example and
perhaps TpSimpleObserver) before we move on to the other classes. After that,
I'd recommend Handler next - Approver is harder, because it needs more API on
ChannelDispatchOperation.

> +/**
> + * SECTION:base-client
> + * @title: TpBaseClient
> + * @short_description: base class for Telepathy clients on D-Bus
> + *
> + * This base class makes it easier to write #TpSvcClient
> + * implementations. Subclasses should usually pass the filters they
> + * want and override the D-Bus methods they implement.
> + */

Eventually, I'd like a code example or a reference to TpSimpleObserver (etc.)
here.

It'd be nice to have an observer in examples/client/ that just does a
g_message() whenever a StreamedMedia call starts or finishes, or something
similarly easy.

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

More information about the telepathy-bugs mailing list