I think the handling of incoming contact lists and groups should be handled via the same codepath.

Created attachment 157482 [details] [review]
http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/new-channels-614236

 libempathy/empathy-tp-contact-list.c |  163 +++++++++++++--------------------
 1 files changed, 64 insertions(+), 99 deletions(-)

Review of attachment 157482 [details] [review]:

So this seems mostly fine, but I think it could go a step further and replace the 3 calls to EnsureChannel() with the same infrastructure being used to discover HANDLE_TYPE_GROUPs, that way even more common code could be shared.

::: libempathy/empathy-tp-contact-list.c
@@ +735,3 @@
+								  properties, NULL);
+			got_list_channel (list, channel);
+			g_object_unref (channel);

Is it worth factoring this out into a separate function like tp_contact_list_group_add_channel() is?

I suppose in a way it is via got_list_channel. Move the channel creation there maybe?

@@ +771,3 @@
+				TP_IFACE_CHANNEL ".ChannelType"),
+		    TP_IFACE_CHANNEL_TYPE_CONTACT_LIST))
+			continue;

It seems to me that this request for Channels should also handle finding the contact lists, rather than explicitly calling EnsureChannels for the contact lists (unless that's against the Telepathy spec for some reason). After all, they're either already in Channels, or going to soon be announced by NewChannels.

You can then replace this entire for loop with common code shared with the NewChannels signal handler.

(In reply to comment #3)
> It seems to me that this request for Channels should also handle finding the
> contact lists, rather than explicitly calling EnsureChannels for the contact
> lists (unless that's against the Telepathy spec for some reason). After all,
> they're either already in Channels, or going to soon be announced by
> NewChannels.

Interesting. Actually, after discussion with Sjoerd and Smcv, I made another branch, based on this one, removing code in NewChannels catching list channels and call EnsureChannel with an 'infinite' timeout instead.
See http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/infinite-timeout

Not sure what's best. Thoughts?

I defer to smcv's opinion.

Groups and lists (by which I mean subscribe, publish etc.) are not the same.

For lists, you need to call EnsureChannel (probably directly on the CM) so you can distinguish between a list that hasn't appeared yet, and a list that will never appear. (For instance, if you defer processing until "stored" appears, then you'll wait forever on Salut connections.)

For groups, you can't know what the complete set of groups is, so you need a hybrid approach:

* when discovering what groups there are, you have to use Channels and NewChannels (or the deprecated versions thereof, or be an Observer, or some similar activity)

* when adding a contact to a group, you must EnsureChannel (optionally, you can skip this if you already know that the group exists, and what its object path is)

I think the "infinite timeout" approach is easier and make the code cleaner.
Danielle: could you review this branch directly please?

In got_channels_cb(), do you need to repeat the contents of this loop? Channels is in the same format as NewChannels, so can you share this code in common?

Otherwise looks good.

Created attachment 158098 [details] [review]
http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/infinite-timeout

 libempathy/empathy-tp-contact-list.c |  162 ++++++++++++----------------------
 1 files changed, 55 insertions(+), 107 deletions(-)

Review of attachment 158098 [details] [review]:

Looks good.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.