Bug 27833

Please consult telepathy-qt4 for more details.

In particular, creation of a TpCDO should *not* automatically prepare this feature (as an exception to the usual behaviour), because when we get a CDO in ObserveChannels, we *don't* want it wasting D-Bus traffic on an object that about 5% of Observers will actually use.

AddDispatchOperation should ask for this feature explicitly, though.

For Bug #27874 we also need to be able to call Claim and HandleWith - you can do that with the tp_cli family of methods already, but I think this'd be best as a GAsyncResult-style API.

Ideally, it shouldn't be necessary to use any tp_cli_* on these objects.

I started working on this:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/dispatch-operation

There is no high-level API for D-Bus methods yet but I think this branch can already be reviewed/merged.

I added:
- GObject properties
- core feature
- a "channel-lost" Glib signal
- a simple service implementation to be able to test those

I added API for HandleWith() and Claim().

Humm, I'm wondering if we should have a _with_channels variant of tp_channel_dispatch_operation_new() as we receive the initial set of channels in AddDispatchOperation().

(In reply to comment #5)
> Humm, I'm wondering if we should have a _with_channels variant of
> tp_channel_dispatch_operation_new() as we receive the initial set of channels
> in AddDispatchOperation().

If you do, you'll need to re-fetch Channels *anyway*, and please fake a ChannelLost signal for any that turn out to have gone missing in the meantime!

I added more tests. I think the branch is now ready for review: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/dispatch-operation

> +maybe_set_connection (TpChannelDispatchOperation *self,
[...]
> +  g_object_notify ((GObject *) self, "connection");
> +
> +  if (g_hash_table_lookup (self->priv->immutable_properties,
> +        TP_PROP_CHANNEL_DISPATCH_OPERATION_CONNECTION) != NULL)
> +    return;

This doesn't look right. How about checking whether the path is the same one we already had before notifying? Same for account and possible-handlers.

> +tp_channel_dispatch_operation_set_property (GObject *object,
...
> +      self->priv->connection = g_value_dup_object (value);
(etc.)

This isn't right - the properties are marked as read-only. Also, unless they're construct-only, you can overwrite (and leak) an earlier non-NULL pointer.

> +get_dispatch_operation_prop_cb (TpProxy *proxy,

The CDO should be invalidated if its D-Bus properties aren't sane.

> +  if (self->priv->preparing_core)
> +    return;   /* already running */

You don't seem to ever set this to true?

> +   * TpChannelDispatchOperation::channel-lost: (skip)

Why do you (skip) this? It seems just as useful to Python/Javascript.

> + * tp_channel_dispatch_operation_handle_with_async:
...
> + * If successful, this method will cause the #TpProxy::invalidated signal
> + * to be emitted.

Specify which code it'll be? (TP_DBUS_ERROR_OBJECT_REMOVED, I think)

> +  g_assert (!tp_strdiff (tp_proxy_get_object_path (channel),
> +        tp_proxy_get_object_path (test->text_chan_2)));

Better as g_assert_cmpstr (X, !=, Y); for suitable X and Y.

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

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

>     test-base-client: test invalidating one channel while calling AddDispatchOperation

This looks as if it contains a secret bugfix? :-)

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.

(Oops, I reviewed Bug #27874 and Bug #27875 here too. The simple approver looks fine, and the example is good stuff!)

(In reply to comment #8)
> > +maybe_set_connection (TpChannelDispatchOperation *self,
> [...]
> > +  g_object_notify ((GObject *) self, "connection");
> > +
> > +  if (g_hash_table_lookup (self->priv->immutable_properties,
> > +        TP_PROP_CHANNEL_DISPATCH_OPERATION_CONNECTION) != NULL)
> > +    return;
> 
> This doesn't look right. How about checking whether the path is the same one we
> already had before notifying? Same for account and possible-handlers.

Hum, in that case should I recreate the priv->connetion as well? Didn't we already lost if the initial properties were actually wrong?

> > +tp_channel_dispatch_operation_set_property (GObject *object,
> ...
> > +      self->priv->connection = g_value_dup_object (value);
> (etc.)
> 
> This isn't right - the properties are marked as read-only. Also, unless they're
> construct-only, you can overwrite (and leak) an earlier non-NULL pointer.

Right. I removed the setter for those properties.

> > +get_dispatch_operation_prop_cb (TpProxy *proxy,
> 
> The CDO should be invalidated if its D-Bus properties aren't sane.

Done.
Does that include if the properties are not the same as the ones when received in tp_channel_dispatch_operation_new?

> > +  if (self->priv->preparing_core)
> > +    return;   /* already running */
> 
> You don't seem to ever set this to true?

Oooops. Fixed.

> > +   * TpChannelDispatchOperation::channel-lost: (skip)
> 
> Why do you (skip) this? It seems just as useful to Python/Javascript.

Copy/paste mistake. Fixed.

> > + * tp_channel_dispatch_operation_handle_with_async:
> ...
> > + * If successful, this method will cause the #TpProxy::invalidated signal
> > + * to be emitted.
> 
> Specify which code it'll be? (TP_DBUS_ERROR_OBJECT_REMOVED, I think)

Indeed. Done.

> > +  g_assert (!tp_strdiff (tp_proxy_get_object_path (channel),
> > +        tp_proxy_get_object_path (test->text_chan_2)));
> 
> Better as g_assert_cmpstr (X, !=, Y); for suitable X and Y.

done.

(In reply to comment #11)
> Hum, in that case should I recreate the priv->connetion as well? Didn't we
> already lost if the initial properties were actually wrong?

My complaint was that you emit notify::connection before checking whether to actually change the Connection object - just move the notify emission down a bit, I think. Yes, we should disregard the case of having incorrect initial properties.

> > The CDO should be invalidated if its D-Bus properties aren't sane.
> 
> Done.
> Does that include if the properties are not the same as the ones when received
> in tp_channel_dispatch_operation_new?

I think we should trust the properties from tp_channel_dispatch_operation_new, and assume they're right; if immutable properties were provided up-front, everything except Channels in the GetAll result can be ignored.

Other minor things, apart from the whole "I think the child objects should be pushable in at construct-time" issue:

> + * If a channel closes, the #TpChannelDispatchOperation::channel-lost signal
> + * is emitted. If all channels
>   * close, there is nothing more to dispatch, so the invalidated signal will be
>   * emitted with the domain %TP_DBUS_ERRORS and the error code
>   * %TP_DBUS_ERROR_OBJECT_REMOVED.

This would be better as "the #TpProxy::invalidated signal" (my fault, but it seems better to fix it in this branch than to have a trivia branch).

> +  g_object_notify ((GObject *) self, "channel-dispatch-operation-properties");

Perhaps "cdo-properties"?

> +  features[FEAT_CORE].name = TP_CHANNEL_DISPATCH_OPERATION_FEATURE_CORE;
> +  features[FEAT_CORE].core = TRUE;

This reminds me that I should rename ".core" to something less ambiguous, like "blocks_all_preparation". (This code is correct, because ".core" does not imply "prepare automatically".)

> + * tp_channel_dispatch_operation_borrow_immutable_properties:

These borrow() functions should probably all be (skip) - they're "C bindings" for a GObject property.

The result of tp_channel_dispatch_operation_borrow_immutable_properties should also be (element-type utf8 GObject.Value).

> The result of tp_channel_dispatch_operation_borrow_immutable_properties should
> also be (element-type utf8 GObject.Value).

... and/or documented as being a map from string to GValue in the text, with a reference to tp_asv_get_string() as an example of useful API to use with it.

(In reply to comment #12)
> (In reply to comment #11)
> > Hum, in that case should I recreate the priv->connetion as well? Didn't we
> > already lost if the initial properties were actually wrong?
> 
> My complaint was that you emit notify::connection before checking whether to
> actually change the Connection object - just move the notify emission down a
> bit, I think. 

But we check first if priv->connection wasn't already set. So at this stage we actually define the connection property.

> > > The CDO should be invalidated if its D-Bus properties aren't sane.
> > 
> > Done.
> > Does that include if the properties are not the same as the ones when received
> > in tp_channel_dispatch_operation_new?
> 
> I think we should trust the properties from tp_channel_dispatch_operation_new,
> and assume they're right; if immutable properties were provided up-front,
> everything except Channels in the GetAll result can be ignored.

Good, that's what I did.

(In reply to comment #13)
> Other minor things, apart from the whole "I think the child objects should be
> pushable in at construct-time" issue:

At this involves adding new API and we'll have to support the current one as well, would it be ok if I solve that in another branch based on the approver one? That would make testing easier and will avoid potential conflicts.

> > + * If a channel closes, the #TpChannelDispatchOperation::channel-lost signal
> > + * is emitted. If all channels
> >   * close, there is nothing more to dispatch, so the invalidated signal will be
> >   * emitted with the domain %TP_DBUS_ERRORS and the error code
> >   * %TP_DBUS_ERROR_OBJECT_REMOVED.
> 
> This would be better as "the #TpProxy::invalidated signal" (my fault, but it
> seems better to fix it in this branch than to have a trivia branch).

Fixed.

> > +  g_object_notify ((GObject *) self, "channel-dispatch-operation-properties");
> 
> Perhaps "cdo-properties"?

Renamed.

> > + * tp_channel_dispatch_operation_borrow_immutable_properties:
> 
> These borrow() functions should probably all be (skip) - they're "C bindings"
> for a GObject property.
> 
> The result of tp_channel_dispatch_operation_borrow_immutable_properties should
> also be (element-type utf8 GObject.Value).

done.

(In reply to comment #16)
> (In reply to comment #13)
> > Other minor things, apart from the whole "I think the child objects should be
> > pushable in at construct-time" issue:
> 
> At this involves adding new API and we'll have to support the current one as
> well, would it be ok if I solve that in another branch based on the approver
> one? That would make testing easier and will avoid potential conflicts.

That sounds fair. I'd rather not merge this branch until the other one's ready too, though.

Merged! Will be in 0.11.5