I'm not sure about the best API for this.
The signature of my current callback for the AddDispatchOperation implementation is:

typedef void (*TpBaseClientClassAddDispatchOperationImpl) (
    TpBaseClient *client,
    GList *channels,
    TpChannelDispatchOperation *dispatch_operation,
    TpAddDispatchOperationContext *context);

All the channels have TP_CHANNEL_FEATURE_CORE prepared and the CDO has TP_CHANNEL_DISPATCH_OPERATION_FEATURE_CORE prepared.

The channels list is a bit redundant as we can get them using tp_channel_dispatch_operation_borrow_channels().
On the other hand, TP_CHANNEL_DISPATCH_OPERATION_FEATURE_CORE doesn't prepare the channels objects so the channels returned by tp_channel_dispatch_operation_borrow_channels() are not guaranteed to be prepared.

Should we remove the GList from this callback? In that case should we add a TP_CHANNEL_DISPATCH_OPERATION_FEATURE_CHANNELS_CORE_PREPARED feature and ensure that the CDO has this feature prepared?
If we do that, the list of channels passed to AddDispatchOperation() would be ignored as CDO will fetch them from D-Bus.

Let's say we let the channels arg in the callback. How should we handle channels who failed to prepare (because they've been invalidated in most/all the cases)?
a) Pass them to the callback? Then the list of channels could be different from the one returned by tp_channel_dispatch_operation_borrow_channels()
b) Ignore them and don't pass them to the callback.

Should we also pass an invalidated TpChannelDispatchOperation object to the callback or ignore it and return from the D-Bus call right away?

(Personnal note: these 2 commits don't have their equivalent in the approver branch)
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=de8447ea9cf1fd36f9a8781522a2ef3524366d9a
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=12c77955be313d4f285b488313f2291075ee1505

I think the distinction is: the Channels passed to AddDispatchOperation are an *initial* set of channels, while the CDO's Channels property is the *current* set of channels.

I'd be inclined to pass them to the callback; also, the Account and Connection.

Another thing to consider, which I missed during TpBaseClient code review (sorry): uniqueness guarantees. At the moment, you always make a new TpAccount and TpConnection in _tp_base_client_observe_channels, and presumably in the Approver. I think it should be more like this:

* each TpBaseClient has a reference to a TpAccountManager
* if the TpBaseClient's dbus_daemon is the same as returned by tp_dbus_daemon_dup(), then the account manager is guaranteed to be the one from tp_account_manager_dup(); otherwise, it's given by tp_account_manager_new()
* the TpAccount in ObserveChannels is made using tp_account_manager_ensure_account() on the AM
* the TpConnection is made using tp_account_ensure_connection() on the Account
* the channels are attached to that TpConnection

This means that any TpContacts will all come from the same TpConnection, which is a considerable win.

API is now:

/**
 * TpBaseClientClassAddDispatchOperationImpl:
 * @client: a #TpBaseClient instance
 * @account: a #TpAccount having %TP_ACCOUNT_FEATURE_CORE prepared if possible
 * @connection: a #TpConnection having %TP_CONNECTION_FEATURE_CORE prepared
 * if possible
 * @channels: (element-type TelepathyGLib.Channel): a #GList of #TpChannel,
 *  all having %TP_CHANNEL_FEATURE_CORE prepared if possible
 * @dispatch_operation: a #TpChannelDispatchOperation having
 * %TP_CHANNEL_DISPATCH_OPERATION_FEATURE_CORE prepared if possible
 * @context: a #TpObserveChannelsContext representing the context of this
 *  D-Bus call
 *
 * Signature of the implementation of the AddDispatchOperation method.
 *
 * This function must call either tp_add_dispatch_operation_context_accept(),
 * tp_add_dispatch_operation_context_delay() or
 * tp_add_dispatch_operation_context_fail() on @context before it returns.
 *
 * Since: 0.11.UNRELEASED
 */

(In reply to comment #1)
> On the other hand, TP_CHANNEL_DISPATCH_OPERATION_FEATURE_CORE doesn't prepare
> the channels objects so the channels returned by
> tp_channel_dispatch_operation_borrow_channels() are not guaranteed to be
> prepared.

I thinks that's annoying as, in practice that's the channels list that will be used by the approver.
It's silly that those TpChannel are not the same ones as the one passed in the callback.
Suggestion?

(I'm reviewing this as part of Bug #27833.)

Replying here as that's the right bug.

> > + * This function must call either tp_add_dispatch_operation_context_accept(),
> > + * tp_add_dispatch_operation_context_delay() or
> > + * tp_add_dispatch_operation_context_fail() on @context before it returns.
> 
> I think it's also worth putting in a reference to the HandleWith and Claim
> wrappers.

Done.

> > +      if (g_error_matches (error, TP_DBUS_ERRORS, TP_DBUS_ERROR_OBJECT_REMOVED))
> > +        {
> > +          /* There is no point calling the AddDispatchOperation
> > +           * implementation; just terminate the D-Bus call right away */
> > +          tp_add_dispatch_operation_context_accept (ctx);
> > +        }
> 
> This isn't meant to happen - ChannelLost and Finished are meant to be delayed
> until all approvers have returned from AddDispatchOperation (per the spec), and
> we're an approver that hasn't returned. So, I don't think it's worth a special
> case.

Cool. Removed.

> >     test-base-client: test invalidating one channel while calling AddDispatchOperation
> 
> This looks as if it contains a secret bugfix? :-)

Yeah, but I probably squash the fix while rebasing. :)

(In reply to comment #9)
> As with the Observer context, it would be good to use the shared
> TpAccountManager, TpAccount, TpConnection chain if possible.
> 
> It might be good to have the CDO and the Approver context receive an account
> and a connection as optional construct-time properties, which would change the
> processing to something like:
> 
> set_property:
>     remember the immutable properties
>     *don't* make Account etc. proxies
> 
> constructed:
>     if no Account, Connection, etc. were supplied at construct time,
>     look in the immutable properties and try to set them
> 
> Alternatively, the CDO and context could take the TpAccountManager as a
> construct-time property, and start the chain there.

Why not just do the same trick in the CDO using _tp_dbus_daemon_is_the_shared_one and create the AM accordingly? That way we wouldn't have to add new API.

Note that this doesn't solve the "issue" of having different TpChannel objects in the context and in CDO.

(In reply to comment #9)
> Why not just do the same trick in the CDO using
> _tp_dbus_daemon_is_the_shared_one and create the AM accordingly? That way we
> wouldn't have to add new API.

You unavoidably end up with a different TpAccount and TpConnection in all non-shared-bus cases, which seems less than ideal.

> Note that this doesn't solve the "issue" of having different TpChannel objects
> in the context and in CDO.

Yes, that's a fair point; perhaps the approver context should be able to stuff TpChannel objects into the CDO, too (maybe with library-internal API).

For Observers that are acting as non-interactive approvers, it would also be useful for the observer context to be able to pre-populate the CDO with the right TpAccount, TpConnection and initial TpChannel objects; the CDO still wouldn't be pre-prepared, but when asked to prepare, it would use its existing objects.

Trivia seen when reading through the combined diff (you can consider the fixes for these to have been pre-reviewed):

> +++ b/telepathy-glib/add-dispatch-operation-context-internal.h

Copyright date should be 2010 like the others, or 2009-2010 if it's derived from code from 2009

> +  void (*chain_up) (GObject *) =
> +    ((GObjectClass *) \
> +      tp_add_dispatch_operation_context_parent_class)->constructed;

You don't need that backslash, line-joining is generally only relevant in macro definitions

> + * Implementation can then use tp_channel_dispatch_operation_handle_with_async()
> + * or tp_channel_dispatch_operation_claim_async() to approve or disapprove the
> + * channels.

"The implementation can then use tp_channel_dispatch_operation_handle_with_async() to approve handling of the channels, or tp_channel_dispatch_operation_claim_async() to take responsibility for handling or closing them", perhaps?

> and may only be called on objects that implement
> + * @add_dispatch_operation.

"and may only be called on objects whose class has called tp_base_client_implement_add_dispatch_operation()" perhaps?

> +  if (cls->priv->add_dispatch_operation_impl == NULL)
> +    {
> +      DEBUG ("class %s does not implement AddDispatchOperation",
> +          G_OBJECT_TYPE_NAME (self));

CRITICAL()? This isn't meant to be reachable, AFAICS.

(In reply to comment #11)
> Trivia seen when reading through the combined diff (you can consider the fixes
> for these to have been pre-reviewed):
> 
> > +++ b/telepathy-glib/add-dispatch-operation-context-internal.h
> 
> Copyright date should be 2010 like the others, or 2009-2010 if it's derived
> from code from 2009

Fixed.

> > +  void (*chain_up) (GObject *) =
> > +    ((GObjectClass *) \
> > +      tp_add_dispatch_operation_context_parent_class)->constructed;
> 
> You don't need that backslash, line-joining is generally only relevant in macro
> definitions

removed.

> > + * Implementation can then use tp_channel_dispatch_operation_handle_with_async()
> > + * or tp_channel_dispatch_operation_claim_async() to approve or disapprove the
> > + * channels.
> 
> "The implementation can then use
> tp_channel_dispatch_operation_handle_with_async() to approve handling of the
> channels, or tp_channel_dispatch_operation_claim_async() to take responsibility
> for handling or closing them", perhaps?

changed.

> > and may only be called on objects that implement
> > + * @add_dispatch_operation.
> 
> "and may only be called on objects whose class has called
> tp_base_client_implement_add_dispatch_operation()" perhaps?

done. I changed the Observer API as well to stay coherent.

> > +  if (cls->priv->add_dispatch_operation_impl == NULL)
> > +    {
> > +      DEBUG ("class %s does not implement AddDispatchOperation",
> > +          G_OBJECT_TYPE_NAME (self));
> 
> CRITICAL()? This isn't meant to be reachable, AFAICS.

Wouldn't that make the client remotely crashable by calling a D-Bus method it doesn't implement?

(In reply to comment #12)
> > > +  if (cls->priv->add_dispatch_operation_impl == NULL)
> > > +    {
> > > +      DEBUG ("class %s does not implement AddDispatchOperation",
> > > +          G_OBJECT_TYPE_NAME (self));
> > 
> > CRITICAL()? This isn't meant to be reachable, AFAICS.
> 
> Wouldn't that make the client remotely crashable by calling a D-Bus method it
> doesn't implement?

AFAICS, you already raised NotImplemented if the "is an approver" flag wasn't set, and trying to set that flag without having an AddDispatchOperation implementation would already have caused a critical?

(In reply to comment #13)
> (In reply to comment #12)
> > > > +  if (cls->priv->add_dispatch_operation_impl == NULL)
> > > > +    {
> > > > +      DEBUG ("class %s does not implement AddDispatchOperation",
> > > > +          G_OBJECT_TYPE_NAME (self));
> > > 
> > > CRITICAL()? This isn't meant to be reachable, AFAICS.
> > 
> > Wouldn't that make the client remotely crashable by calling a D-Bus method it
> > doesn't implement?
> 
> AFAICS, you already raised NotImplemented if the "is an approver" flag wasn't
> set, and trying to set that flag without having an AddDispatchOperation
> implementation would already have caused a critical?

You're right. Done for the Observer and Approver.

http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/approver
contains the previous branches and fix the "recycling proxies" issue.

> +        "dbus-connection", ((TpProxy *) bus_daemon)->dbus_connection,

This is redundant - just pass in the bus daemon and TpProxy will DTRT. Add a check to the regression test that the dbus-connection is the same as the bus daemon's dbus-connection, if you're worried.

Strictly speaking, _tp_channel_dispatch_operation_new_with_objects doesn't need the bus_daemon argument - it could get it from the TpAccount - but that's probably unnecessarily subtle.

Other than that, this looks good.

done

Looks good, ship it

Merged! Will be in 0.11.5