Bug 30617

I started implementing this and a few questions regarding the proper implementation.

Do we want to check if TP_IFACE_QUARK_CHANNEL_INTERFACE_GROUP is implemented before calling RemoveMembers (as MC does)?
If yes, should this function assume that TP_CHANNEL_FEATURE_CORE is prepared on the object?

What happen if user passes a non empty message or a not none reason on a channel not implementing Group? Operation fails? We fall back to Close() and succeed ? We fallback and fail?

I don't know how this method should relate to Call channels, which distinguish between hanging up and closing the channel; if you Close() a Call channel, that's assumed to be "unclean shutdown", e.g. from Mission Control closing an undispatchable channel or cleaning up after a crashed Handler. Sjoerd, what do you think?

We should probably have two methods, close_async and leave_async, with the former wrapping the latter behind-the-scenes, and document leave_async as being intended for use with chatrooms etc.?

(In reply to comment #1)
> Do we want to check if TP_IFACE_QUARK_CHANNEL_INTERFACE_GROUP is implemented
> before calling RemoveMembers (as MC does)?
> If yes, should this function assume that TP_CHANNEL_FEATURE_CORE is prepared on
> the object?

I don't think waiting for Core is desirable. I think the logic should go something like this:

* if we know the Interfaces already, and Group is not among them, Close
* else if the reason and message are trivial, Close
* else RemoveMembers; if that fails, and the channel has still not been invalidated, Close instead

This means if the channel isn't prepared *and* we don't know its immutable properties, we might try RemoveMembers on a channel that later turns out not to have been a Group, but trying RemoveMembers is no slower than getting the interfaces, so, no problem.

> What happen if user passes a non empty message or a not none reason on a
> channel not implementing Group? Operation fails? We fall back to Close() and
> succeed ? We fallback and fail?

Fall back and succeed, I think. The important thing is that we left.

Also, if the channel is invalidated before we've finished and the method call fails, we should DEBUG() the failure reason, and succeed anyway - again, the important thing is that we left. The only way "leave" can fail should be if Close() failed and the channel remained open (e.g. non-empty Group).

Humm but we need to prepare TP_CHANNEL_FEATURE_GROUP in order to get
tp_channel_group_get_self_handle() working.

http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-close-30617

The API here looks right. I'm a bit reluctant to merge this without a clear picture of how it relates to Call, though...

(i.e. will it still make sense to use a Group_Change_Reason in the Call world?)

-------------------

Implementation:

> +  g_simple_async_result_complete (result);

This should be in an idle: TpProxy doesn't guarantee delayed callback calls, sadly.

> + * for any reason we can properly leave the channel, we close it.

"we can't properly"

I think it'd be useful if tp_channel_leave_async() supported a NULL callback?

> +  /* Create service-side tube channel object */

tube? :-)

> +  test->chan_room_service = g_object_new (

There's a test-specific wrapper for g_object_new which suppresses valgrind warnings about the "leaked" class struct.

> +  /* FIXME: This is crack a chan_room_service is actually not a
> +   * TpTestsTextChannelNull */
> +  props = tp_tests_text_channel_get_props (
> +      (TpTestsTextChannelNull *) test->chan_room_service);

Er, yes, that...

(In reply to comment #5)
> The API here looks right. I'm a bit reluctant to merge this without a clear
> picture of how it relates to Call, though...
> 
> (i.e. will it still make sense to use a Group_Change_Reason in the Call world?)

Is there a spec bug for that question?

> > +  g_simple_async_result_complete (result);
> 
> This should be in an idle: TpProxy doesn't guarantee delayed callback calls,
> sadly.

How so? Both _complete are in a D-Bus method callback so it should be fine,
no?

> > + * for any reason we can properly leave the channel, we close it.
> 
> "we can't properly"

fixed.

> I think it'd be useful if tp_channel_leave_async() supported a NULL callback?

Doesn't GSimpleAsyncResult already do that?

> > +  /* Create service-side tube channel object */
> 
> tube? :-)

Ooops; fixed.

> > +  test->chan_room_service = g_object_new (
> 
> There's a test-specific wrapper for g_object_new which suppresses valgrind
> warnings about the "leaked" class struct.

done.

> > +  /* FIXME: This is crack a chan_room_service is actually not a
> > +   * TpTestsTextChannelNull */
> > +  props = tp_tests_text_channel_get_props (
> > +      (TpTestsTextChannelNull *) test->chan_room_service);
> 
> Er, yes, that...

Oh I forgot that. From a quick look, tests doesn't seem to rely on it leaving
in the past so I'll port it to TpBaseChannel.

(In reply to comment #6)
> > > +  /* FIXME: This is crack a chan_room_service is actually not a
> > > +   * TpTestsTextChannelNull */
> > > +  props = tp_tests_text_channel_get_props (
> > > +      (TpTestsTextChannelNull *) test->chan_room_service);
> > 
> > Er, yes, that...
> 
> Oh I forgot that. From a quick look, tests doesn't seem to rely on it leaving
> in the past so I'll port it to TpBaseChannel.

done.

I'm starting to think the assumption made by mcp_dispatch_operation_leave_channels (that RemoveMembers([self], No_Reason) is exactly equivalent to Close()) might not be such a good idea. Sorry about that...

Perhaps it'd be better to have close_async() always be Close(), leave_group_async() always be removal of the self-handle, and leave it at that...

Arguments in favour of having the C methods directly mirror the D-Bus calls:

* close_async() always does the right thing for Call

* leave_group_async() is easier to document, since it always fails on a non-Group

* if we add proxy mode to Idle (Bug #24273) we need to distinguish between the two actions anyway

Arguments in favour of what MC does:

* some old CM versions can't RemoveMembers([self]) unless the flags allow kicking the other user (this made sense when I implemented it in MC, but now the answer is probably to fix the CM)

(In reply to comment #2)
> Also, if the channel is invalidated before we've finished and the method call
> fails, we should DEBUG() the failure reason, and succeed anyway - again, the
> important thing is that we left. The only way "leave" can fail should be if
> Close() failed and the channel remained open (e.g. non-empty Group).

I implemented this for the Close() case too.

(In reply to comment #6)
> > > +  g_simple_async_result_complete (result);
> > 
> > This should be in an idle: TpProxy doesn't guarantee delayed callback calls,
> > sadly.
> 
> How so? Both _complete are in a D-Bus method callback so it should be fine,
> no?

Unfortunately, tp_cli_*_call_* calls the callback immediately (before returning) if the proxy has already been invalidated, or if it doesn't have the interface. I'll fix that for 1.0, but I think it'd be a subtle API break to fix it now.

> > I think it'd be useful if tp_channel_leave_async() supported a NULL callback?
> 
> Doesn't GSimpleAsyncResult already do that?

I thought it did, but you're right, it doesn't. This doesn't give us the optimization of ignoring the reply at the D-Bus level, though.

I implemented that optimization for close_async, because it's trivial, but not for leave_async, because it's harder there.

> > > +  /* FIXME: This is crack a chan_room_service is actually not a
> > > +   * TpTestsTextChannelNull */
> > > +  props = tp_tests_text_channel_get_props (
> > > +      (TpTestsTextChannelNull *) test->chan_room_service);
> > 
> > Er, yes, that...
> 
> Oh I forgot that. From a quick look, tests doesn't seem to rely on it leaving
> in the past so I'll port it to TpBaseChannel.

You forgot to fix this bit, but I've done that now.

(In reply to comment #8)
> Perhaps it'd be better to have close_async() always be Close(),
> leave_group_async() always be removal of the self-handle, and leave it at
> that...

I'm still not sure exactly what the semantics should be. :-(

I've made close_async() always call Close: I'm pretty sure that's correct - when we implement proxy mode in Idle, we want it to close channels, but not leave them.

At the moment, leave_async() calls RemoveMembersWithReason with failover to Close. I think that's *probably* right - "leaving" a non-Group channel is fairly meaningless, but it seems harmless to implement that as Close (leaving is "more destructive than" closing, even in the proposed proxy mode).

Another possibility would be to make leave_async g_return_if_fail that it is (already known to be) a Group, rename it to group_leave_async, and remove the fallbacks to Close.

Sjoerd, any thoughts?

The branch looks good from a code pov.

I'm not sure of what's the best semantic either. Didn't we have plan to allow at some point to have a channel without being a member of it? In that case, closing the channel if we can't leave it could be annoying.

(In reply to comment #9)
> > > I think it'd be useful if tp_channel_leave_async() supported a NULL callback?
> > 
> > Doesn't GSimpleAsyncResult already do that?
> 
> I thought it did, but you're right, it doesn't.

Ahem. Please reverse that sentence: I thought GSimpleAsyncResult didn't support NULL as a callback, but it does allow it. :-)

Yeah, that's how I read it :)

Fixed in git for 0.13.10.