Bug 33271

Summary: Implement ChannelDispatcher.DelegateChannels() and PresentChannel()
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: mission-controlAssignee: Guillaume Desmottes <guillaume.desmottes>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/cassidy/telepathy-mission-control;a=shortlog;h=refs/heads/re-dispatch-33271
Whiteboard: PLUS PLUS PLUS
i915 platform: i915 features:
Bug Depends on: 37109    
Bug Blocks:    

Description Guillaume Desmottes 2011-01-19 05:37:47 UTC
In bug #25293 I proposed a new API to redispatch channels; we should implement it in MC.
Comment 1 Guillaume Desmottes 2011-01-21 05:40:57 UTC
I have an start of implementation of this in http://git.collabora.co.uk/?p=user/cassidy/telepathy-mission-control;a=shortlog;h=refs/heads/re-handle-33271

I re-implemented all the logic rather than reusing the existing dispatching code as redispatching is much simpler than the general dispatching which is already pretty complex.
Comment 2 Vivek Dasmohapatra 2011-02-15 12:43:40 UTC
Looks reasonable to me.
Make it so.
Comment 3 Guillaume Desmottes 2011-02-16 02:07:38 UTC
Merged to master. I keep this bug opened while we don't implement the stable API.
Comment 4 Guillaume Desmottes 2011-02-23 05:28:52 UTC
I updated to latest spec and implemented CD.ReEnsureChannel().

http://git.collabora.co.uk/?p=user/cassidy/telepathy-mission-control;a=shortlog;h=refs/heads/re-dispatch-33271
Comment 5 Guillaume Desmottes 2011-05-10 02:16:51 UTC
Here is the cleaned branch implementing the stable API:
http://cgit.collabora.co.uk/git/user/cassidy/telepathy-mission-control/log/?id=refs/heads/re-dispatch-33271

We need a tp-glib 0.15.1 release (and a dep bump) before merging it but it can already be reviewed.
Comment 6 Jonny Lamb 2011-05-11 03:07:33 UTC
+ /* All the channels should belong to the same account, so we just need
+ * to get the connection for the first one. */

o rly? The spec says nothing about this?

+ g_object_get (self->priv->master, "account-manager", &am, NULL);
+ g_assert (am != NULL);
+
+ account = mcd_account_manager_lookup_account_by_path (am, chan_account);
+ g_assert (account != NULL);

I'm not sure about asserting on failures. With non-fatal assertions MC will continue to run but PresentChannels won't return? I realise it's only going to fail in a very odd case, but still.

Otherwise looks fine.
Comment 7 Guillaume Desmottes 2011-05-11 04:28:56 UTC
(In reply to comment #6)
> + /* All the channels should belong to the same account, so we just need
> + * to get the connection for the first one. */
> 
> o rly? The spec says nothing about this?

I think that make sense, don't you? I'll update the spec if you agree that's a
sane requierement.

> + g_object_get (self->priv->master, "account-manager", &am, NULL);
> + g_assert (am != NULL);

There are a loads of exactly the same assertion in the code.

> + account = mcd_account_manager_lookup_account_by_path (am, chan_account);
> + g_assert (account != NULL);
> 
> I'm not sure about asserting on failures. With non-fatal assertions MC will
> continue to run but PresentChannels won't return? I realise it's only going to
> fail in a very odd case, but still.

I changed to g_return_if_fail() for the other assertions.
Comment 8 Guillaume Desmottes 2011-05-12 06:57:35 UTC
http://cgit.collabora.co.uk/git/user/cassidy/telepathy-mission-control/log/?id=refs/heads/re-dispatch-33271
now implements new new version of the spec (bug #37109)
Comment 9 Jonny Lamb 2011-05-13 08:50:46 UTC
(In reply to comment #8)
> http://cgit.collabora.co.uk/git/user/cassidy/telepathy-mission-control/log/?id=refs/heads/re-dispatch-33271
> now implements new new version of the spec (bug #37109)

Did you mean to include the doc/reference/*/tmpl/*.sgml files?

+static ChannelToDelegate *
+channel_to_delegate_new (DelegateChannelsCtx *ctx,
+  McdAccount *account,
+  McdChannel *channel)

then

+static void
+free_not_delegated_error (gpointer data)
+{
+    g_boxed_free (TP_STRUCT_TYPE_NOT_DELEGATED_ERROR, data);
+}

What's up with the indentation?

Okay, everything else looks fine. In an ideal world I'd like another test or two for multiple channels which one fails and one succeeds in delegation and stuff like that, but if you cba... :-)

Merge away, once the spec branch is merged.

Oh, also, could you give me spec branch which adds a comment to DelegateChannels and PresentChannels saying which version of MC this was released in?
Comment 10 Guillaume Desmottes 2011-05-15 23:59:08 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > http://cgit.collabora.co.uk/git/user/cassidy/telepathy-mission-control/log/?id=refs/heads/re-dispatch-33271
> > now implements new new version of the spec (bug #37109)
> 
> Did you mean to include the doc/reference/*/tmpl/*.sgml files?

Rah stupid files; removed.

> +static ChannelToDelegate *
> +channel_to_delegate_new (DelegateChannelsCtx *ctx,
> +  McdAccount *account,
> +  McdChannel *channel)
> 
> then
> 
> +static void
> +free_not_delegated_error (gpointer data)
> +{
> +    g_boxed_free (TP_STRUCT_TYPE_NOT_DELEGATED_ERROR, data);
> +}
> 
> What's up with the indentation?

Yeah some part of the file had it wrong; I fixed it.

> Merge away, once the spec branch is merged.

We need a tp-glib release first.

> Oh, also, could you give me spec branch which adds a comment to
> DelegateChannels and PresentChannels saying which version of MC this was
> released in?

Ok, I'll do one once it's merged.
Comment 11 Guillaume Desmottes 2011-05-17 02:19:36 UTC
Merged to master; will be in MC 5.7.12
Comment 12 Simon McVittie 2011-05-17 02:58:17 UTC
Due to the addition of a telepathy-glib 0.15 dependency, this isn't suitable for 5.7.x/5.8.x; it can either be changed to use an internal copy of that interface, or be added in 5.9.0. On the assumption that the latter is what we want, I've branched 5.8.x from just before Guillaume's branch was merged.

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.