Bug 28015

Summary: TpChannelDispatchOperation: convenience API to Claim and reject CDOs
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: low CC: guillaume.desmottes
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=cdo-28015
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 27833    
Bug Blocks:    
Attachments: factor out prepare_core_and_claim()
Add tp_channel_dispatch_operation_close_channels_async() (#28015)
add tp_channel_dispatch_operation_leave_channels_async (#28015)
add tp_channel_destroy_async()
add tp_channel_dispatch_operation_destroy_channels_async() (fdo #28015)

Description Simon McVittie 2010-05-07 04:25:01 UTC
+++ This bug was initially created as a clone of Bug #27833 +++

We should add API to TpChannelDispatchOperation to do some common "reject" operations.

mission-control-plugins/dispatch-operation.h in MC master (5.5.x) has three versions of rejecting channels:

* close_channels
  - Claim()
  - Close()

* destroy_channels
  - Claim()
  - Destroy() if Destroyable is implemented
  - Close() if Destroyable is not implemented

* leave_channels, with an optional TpChannelGroupChangeReason and message
  - Claim()
  - RemoveMembers([self-handle], $reason, $message) if available
  - Close() if RemoveMembers wasn't available or failed

When Call becomes fully-supported, a way to terminate Call channels gracefully would also be good.
Comment 1 Guillaume Desmottes 2011-04-25 06:25:15 UTC
I started implementing this but I'm wondering then the close_channels() operation should be completed:

a) When Claim() returns
b) When all the Close() calls returned

If we go for b), should we succeed only if *all* the Close calls succeeded?


I'm tempted to go for a) as we can't do much if Close() fails anyway.
Comment 2 Will Thompson 2011-05-24 02:59:43 UTC
(In reply to comment #1)
> I started implementing this but I'm wondering then the close_channels()
> operation should be completed:
> 
> a) When Claim() returns
> b) When all the Close() calls returned
> 
> If we go for b), should we succeed only if *all* the Close calls succeeded?
> 
> 
> I'm tempted to go for a) as we can't do much if Close() fails anyway.

Yeah, I agree. Maybe debug some stuff if Close() fails but… I don't see much benefit in propagating that up to the app, what's the app going to do?
Comment 3 Guillaume Desmottes 2011-05-25 00:40:11 UTC
Created attachment 47117 [details] [review]
factor out prepare_core_and_claim()
Comment 4 Guillaume Desmottes 2011-05-25 00:40:14 UTC
Created attachment 47118 [details] [review]
Add tp_channel_dispatch_operation_close_channels_async() (#28015)
Comment 5 Guillaume Desmottes 2011-05-25 00:41:31 UTC
I only implemented the close_channels version here but this can already be reviewed.

What should be the semantic of the 2 others calls? Should they return as soon as Destroy/RemoveMembers has been called on all channels?
Comment 6 Will Thompson 2011-05-25 08:59:35 UTC
Review of attachment 47117 [details] [review]:

Looks functionally fine but I have some refcounting and other nitpicks.

::: telepathy-glib/channel-dispatch-operation.c
@@ +1446,3 @@
   if (!tp_channel_dispatch_operation_claim_finish (self, result, &error))
     {
+      DEBUG ("Failed to Calim %s: %s",

Calim is my best friend's name.

@@ +1452,3 @@
+      g_simple_async_result_complete (ctx->result);
+      /* Remove the ref we got from the caller */
+      g_object_unref (ctx->result);

This is not very clear to me, really: you're having to do “more” freeing of the ctx on some but not all code paths, just so that the callback has to do its own unreffing of the result object?

Also this failure path is duplicated. I don't mind that much though. I do mind 'goto out' when there's exactly one line of code between the goto and the label and you could easily add an else block.
Comment 7 Will Thompson 2011-05-25 10:02:57 UTC
Review of attachment 47118 [details] [review]:

::: telepathy-glib/channel-dispatch-operation.c
@@ +1606,3 @@
+      TpChannel *channel = g_ptr_array_index (self->priv->channels, i);
+
+      g_error_free (error);

If the callback were to use its user_data, it would be unsafe because 'self' might have died before the callback got called.

But it doesn't, so you should pass NULL.
Comment 8 Will Thompson 2011-05-25 10:03:00 UTC
Review of attachment 47118 [details] [review]:

::: telepathy-glib/channel-dispatch-operation.c
@@ +1606,3 @@
+      TpChannel *channel = g_ptr_array_index (self->priv->channels, i);
+
+      g_error_free (error);

If the callback were to use its user_data, it would be unsafe because 'self' might have died before the callback got called.

But it doesn't, so you should pass NULL.
Comment 9 Guillaume Desmottes 2011-05-26 03:27:07 UTC
(In reply to comment #6)
> Review of attachment 47117 [details] [review]:
> 
> Looks functionally fine but I have some refcounting and other nitpicks.
> 
> ::: telepathy-glib/channel-dispatch-operation.c
> @@ +1446,3 @@
>    if (!tp_channel_dispatch_operation_claim_finish (self, result, &error))
>      {
> +      DEBUG ("Failed to Calim %s: %s",
> 
> Calim is my best friend's name.

fixed (squashed).

> @@ +1452,3 @@
> +      g_simple_async_result_complete (ctx->result);
> +      /* Remove the ref we got from the caller */
> +      g_object_unref (ctx->result);
> 
> This is not very clear to me, really: you're having to do “more” freeing of the
> ctx on some but not all code paths, just so that the callback has to do its own
> unreffing of the result object?

As said in prepare_core_and_claim's documentation, it takes the ref on @result
and pass it back to @callback is everything is fine. But if an error occurs,
@callback won't be called, the operation will be completed (with an error) and
so we have to drop this ref.

I made that clearer in the code.

> Also this failure path is duplicated. I don't mind that much though. I do mind
> 'goto out' when there's exactly one line of code between the goto and the label
> and you could easily add an else block.

I factored out prepare_core_and_claim_ctx_failed() removing the code
duplication.
Comment 10 Guillaume Desmottes 2011-05-26 03:31:46 UTC
(In reply to comment #7)
> Review of attachment 47118 [details] [review]:
> 
> ::: telepathy-glib/channel-dispatch-operation.c
> @@ +1606,3 @@
> +      TpChannel *channel = g_ptr_array_index (self->priv->channels, i);
> +
> +      g_error_free (error);
> 
> If the callback were to use its user_data, it would be unsafe because 'self'
> might have died before the callback got called.
> 
> But it doesn't, so you should pass NULL.

Sorry I don't understand what you mean.
Comment 11 Guillaume Desmottes 2011-05-27 04:05:36 UTC
I merged tp_channel_dispatch_operation_close_channels_async; will be in 0.15.1.

I keep the bug opened as we still miss the destroy and remove members case.
Comment 12 Will Thompson 2011-05-27 04:08:15 UTC
(In reply to comment #5)
> What should be the semantic of the 2 others calls? Should they return as soon
> as Destroy/RemoveMembers has been called on all channels?

Probably. In both cases I think they should try to Close() the channel if the method we really wanted to use doesn't work. But I don't see a great deal of point notifying the application if the channels can't be closed for some reason.
Comment 13 Guillaume Desmottes 2011-05-30 05:13:43 UTC
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=cdo-28015 updated with the 2 other versions.
Comment 14 Guillaume Desmottes 2011-05-30 05:13:57 UTC
Created attachment 47310 [details] [review]
add tp_channel_dispatch_operation_leave_channels_async (#28015)
Comment 15 Guillaume Desmottes 2011-05-30 05:14:02 UTC
Created attachment 47311 [details] [review]
add tp_channel_destroy_async()
Comment 16 Guillaume Desmottes 2011-05-30 05:14:06 UTC
Created attachment 47312 [details] [review]
add tp_channel_dispatch_operation_destroy_channels_async() (fdo #28015)
Comment 17 Will Thompson 2011-05-30 05:34:01 UTC
Review of attachment 47311 [details] [review]:

::: telepathy-glib/channel.c
@@ +2402,3 @@
+        }
+
+  if (error != NULL)

I think there's way too much debugging output in this function. And a goto! I'd just DEBUG() if Destroy() fails, move the “Close(); return;” into get_invalidated() == NULL {}, and then you can lose the goto.
Comment 18 Will Thompson 2011-05-30 05:35:12 UTC
Review of attachment 47312 [details] [review]:

Looks fine.
Comment 19 Will Thompson 2011-05-30 05:35:50 UTC
Review of attachment 47310 [details] [review]:

looks fine.
Comment 20 Guillaume Desmottes 2011-05-30 05:57:00 UTC
Merged to master; will be in 0.15.2

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.