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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #25 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-04-28 02:23:57 PDT ---
(In reply to comment #24)
> This looks as though it's on track. I think it's worth merging as soon as it
> can support observers cleanly.

Agreed.

> > tp_observe_channels_context_get_recovering
> 
> is_recovering(), perhaps?

renamed.

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

Fixed.

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

Humm weird.

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

Done in tp_base_client_take_observer_filter as _add_ uses this one.

> 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
>  */

I replaced doc by this.

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

fixed.

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

I added a FIXME.

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

Much better; I changed.
I also added a g_return_if_fail checking that observe_channels_impl has been
defined.

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

done.

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

changed.

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

replaced by a DEBUG().

> > +  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, ".", '/');

changed.

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

done.

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

done.

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

As said in an earlier comment, ,I'll remove the code once the branch is ready
to be merged (to be able to revert the commit without conflict). I #if 0 them
for now.


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

Sounds like a good idea. I'll add that.

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

Very good idea! Done.

> > +          "Implementation of ObserveChannels of %s didn't call "
> 
> Nitpicking: "Implementation of ObserveChannels in %s" would avoid the repeated
> "of" while remaining grammatically correct.

changed.

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

changed.

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

done.

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

changed.

> 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 */

added.

> 
> Still to review
> ===============
> 
> I haven't reviewed the test or the SimpleClient yet.

I guess you mean SimpleObserver :)
I think that's ok, we can merge the Observer bits of BaseClient first. At least
we know that it allows to easily write SimpleObserver.

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

Would you implement it using TpBaseClient or TpSimpleObserver?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.