Bug 43828

Here goes a first review, but I would prefer if some gabble developer could check it also.

+ if (!gabble_roster_get_download_at_connection (base))
+ {
+ WockyStanza *stanza;
+ stanza = _gabble_roster_message_new (self, WOCKY_STANZA_SUB_TYPE_GET,
+ NULL);
+
+ conn_util_send_iq_async (self->priv->conn, stanza,
+ self->priv->cancel_on_disconnect,
+ roster_received_cb, tp_weak_ref_new (self, NULL, NULL));
+
+ g_object_unref (stanza);
+ }
+ else
+ {
+ g_set_error_literal (&error, TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
+ "roster already requested at connection");
+ }
The spec states that when DownloadAtConnection is true, Download should do nothing, so no need to raise an error if the roster was already requested.

+ gabble_simple_async_succeed_or_fail_in_idle (self, callback, user_data,
+ gabble_roster_request_subscription_async, error);

I guess this should be:
gabble_simple_async_succeed_or_fail_in_idle (self, callback, user_data,
    gabble_roster_download_async, error);

+ iface->download_async = gabble_roster_download_async;
/* we use the default _finish functions, which assume a GSimpleAsyncResult */
+
+ iface->get_download_at_connection =
+ gabble_roster_get_download_at_connection;

Put both assignments before the comment.

The rest looks good to me.

Thanks for the review.

(In reply to comment #1)
> Here goes a first review, but I would prefer if some gabble developer could
> check it also.
> 
> The spec states that when DownloadAtConnection is true, Download should do
> nothing, so no need to raise an error if the roster was already requested.

Fixed.

> + gabble_simple_async_succeed_or_fail_in_idle (self, callback, user_data,
> + gabble_roster_request_subscription_async, error);
> 
> I guess this should be:
> gabble_simple_async_succeed_or_fail_in_idle (self, callback, user_data,
>     gabble_roster_download_async, error);

Fixed. And also fixed in other functions.

> + iface->download_async = gabble_roster_download_async;
> /* we use the default _finish functions, which assume a GSimpleAsyncResult */
> +
> + iface->get_download_at_connection =
> + gabble_roster_get_download_at_connection;
> 
> Put both assignments before the comment.

But get_download_at_connection does not use GSimpleAsync.

+ { "download-at-connection",

This seems a confusing connection parameter name. download-contact-list-at-connection is better but is longer than the world. I guess we can use roster here though as it's gabble. "download-roster-on-connection" might be better?

Hey wait, isn't the D-Bus property meant to be a connection manager parameter like DecloakAutomatically?

+ DEBUG ("don't request the roster");

Why not? Say so.

Is there any way we can make gabble_roster_download_async and the changed code in connection_status_changed_cb share the same code perchance? Perhaps you could even just make connection_status_changed_cb call tp_base_contact_list_download_async directly?

I get a little upset seeing new code using conn_util_send_iq_async and not the WockyPorter methods, but oh well, up to you if you want to fix that.

(In reply to comment #3)
> + { "download-at-connection",
> 
> This seems a confusing connection parameter name.
> download-contact-list-at-connection is better but is longer than the world. I
> guess we can use roster here though as it's gabble.
> "download-roster-on-connection" might be better?

Yes. Fixed.

> Hey wait, isn't the D-Bus property meant to be a connection manager parameter
> like DecloakAutomatically?

Oops. Fixed.

> + DEBUG ("don't request the roster");
> 
> Why not? Say so.

Fixed.

> Is there any way we can make gabble_roster_download_async and the changed code
> in connection_status_changed_cb share the same code perchance? Perhaps you
> could even just make connection_status_changed_cb call
> tp_base_contact_list_download_async directly?

Calling tp_base_contact_list_download_async() wouldn't work because the subclass implementation gabble_roster_download_async() checks gabble_roster_get_download_at_connection() and the condition is the reverse than the one in connection_status_changed_cb().

I could create an ad-hoc function to factorize but only 3 instructions would be factorized (the condition in the "if" is not the same).

> I get a little upset seeing new code using conn_util_send_iq_async and not the
> WockyPorter methods, but oh well, up to you if you want to fix that.

+ 'org.freedesktop.Telepathy.Connection.Interface.ContactList.DownloadAtConnection': True,

Surely there's a constant for (at least some of) that? (Not having the literal string org.freedesktop.Telepathy scattered through the tests will make it easier to port to Telepathy 1.0.)

Fixed in branch roster3

Reviewing on Bug #43826 because the two branches are closely related.

(In reply to comment #7)
> Reviewing on Bug #43826 because the two branches are closely related.

Updated on branch roster5.

> + DEBUG ("don't request the roster because the property"
> + "ContactList.DownloadAtConnection is FALSE");

you're missing a space somewhere in this string (after 'property' or before 'ContactList').

Okay so I looked at the test and was like "whaaa? surely you can easily do this in a much better way without duplicating so much code?!" Turns out the exact answer (which I'm sure you already know) is no. I still don't like so much code duplication though so I rewrote the test.

My rewrite does *not* test for invalid connection parameters because it's really hard without changing servicetest. I reckon it's something we can live without though; every test will fail if we give a bad parameter due to RequestConnection dying anyway...

I also removed the simple test for connecting with normal connection parameters as connect/test-success.py does this for you already.

Created attachment 59115 [details]
smaller test

Here's the test. What do you think?

I haven't actually tried it with DownloadAtConnection code so if you want to use it could try it with your branch please? :-)

(In reply to comment #10)
> Created attachment 59115 [details]
> smaller test

Hm, I left in the 'priority' parameter test. Oh well.

Fixed in branch roster6:

(In reply to comment #9)
> > + DEBUG ("don't request the roster because the property"
> > + "ContactList.DownloadAtConnection is FALSE");
> 
> you're missing a space somewhere in this string (after 'property' or before
> 'ContactList').

Fixed

(In reply to comment #10)
> Here's the test. What do you think?
> 
> I haven't actually tried it with DownloadAtConnection code so if you want to
> use it could try it with your branch please? :-)

I took your test and fixed it. It is a lot clearer :)

(In reply to comment #11)
> Hm, I left in the 'priority' parameter test. Oh well.

I removed that.

Looks hot!

(In reply to comment #13)
> Looks hot!

352d2da..71dcf75 pushed to master:


commit 71dcf75998ea8a332903def7b6513df5e4e6d819
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Tue Dec 13 16:43:57 2011 +0000

    New test: connect/test-connection-params.py
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43828

commit 45d81af51417a79bd2fd7c98b0980ba7e725dd5c
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Tue Dec 13 12:16:07 2011 +0000

    ContactList: downloading the roster at connection depends on a connection parameter
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43828

commit deb67c35893b1770533eb48c4e7ec0840afc8926
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Thu Feb 23 16:29:49 2012 +0000

    Update dependency on telepathy-glib
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43826
    https://bugs.freedesktop.org/show_bug.cgi?id=43828

commit 687ec45023294d6003f987ff0dfc72986fdb7f46
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Wed Jan 4 14:22:02 2012 +0000

    gabble_roster_unpublish_async: correct the source tag

commit 5d7a8f948c504b842781e99f77bebcb94bf610a0
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Wed Jan 4 14:21:42 2012 +0000

    gabble_roster_authorize_publication_async: correct the source tag