Bug 27833

Summary: TpChannelDispatchOperation high-level methods and CORE feature
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: tp-glibAssignee: Guillaume Desmottes <guillaume.desmottes>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/dispatch-operation
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 27874, 28015    

Description Guillaume Desmottes 2010-04-26 04:29:12 UTC
This feature will set GObject properties from D-Bus properties.
Comment 1 Simon McVittie 2010-04-26 04:32:24 UTC
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.
Comment 2 Simon McVittie 2010-04-29 02:29:55 UTC
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.
Comment 3 Guillaume Desmottes 2010-04-30 06:57:48 UTC
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
Comment 4 Guillaume Desmottes 2010-05-03 02:38:05 UTC
I added API for HandleWith() and Claim().
Comment 5 Guillaume Desmottes 2010-05-03 03:20:39 UTC
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().
Comment 6 Simon McVittie 2010-05-03 03:43:05 UTC
(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!
Comment 7 Guillaume Desmottes 2010-05-04 02:01:13 UTC
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
Comment 8 Simon McVittie 2010-05-06 10:14:49 UTC
> +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? :-)
Comment 9 Simon McVittie 2010-05-06 10:19:04 UTC
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.
Comment 10 Simon McVittie 2010-05-06 10:20:02 UTC
(Oops, I reviewed Bug #27874 and Bug #27875 here too. The simple approver looks fine, and the example is good stuff!)
Comment 11 Guillaume Desmottes 2010-05-07 02:16:19 UTC
(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.
Comment 12 Simon McVittie 2010-05-07 03:48:50 UTC
(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.
Comment 13 Simon McVittie 2010-05-07 04:19:38 UTC
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).
Comment 14 Simon McVittie 2010-05-07 04:20:41 UTC
> 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.
Comment 15 Guillaume Desmottes 2010-05-07 04:54:41 UTC
(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.
Comment 16 Guillaume Desmottes 2010-05-07 05:11:42 UTC
(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.
Comment 17 Simon McVittie 2010-05-07 05:42:48 UTC
(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.
Comment 18 Guillaume Desmottes 2010-05-10 05:13:55 UTC
Merged! Will be in 0.11.5

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.