Bug 43828

Summary: avoid downloading the roster at every connection
Product: Telepathy Reporter: Alban Crequy <alban.crequy>
Component: gabbleAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: andrunko
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/alban/telepathy-gabble.git/log/?h=roster6
Whiteboard: review+
i915 platform: i915 features:
Attachments: smaller test

Description Alban Crequy 2011-12-14 07:13:18 UTC
We should implement the Telepathy spec change from Bug #43035 with tpglib help from Bug #43826 in Gabble.
Comment 1 Andre Moreira Magalhaes 2011-12-20 09:31:05 UTC
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.
Comment 2 Alban Crequy 2012-01-04 08:21:42 UTC
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.
Comment 3 Jonny Lamb 2012-01-09 04:30:23 UTC
+ { "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.
Comment 4 Alban Crequy 2012-01-20 09:45:28 UTC
(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.
Comment 5 Simon McVittie 2012-02-23 08:14:12 UTC
+ '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.)
Comment 6 Alban Crequy 2012-03-02 09:18:04 UTC
Fixed in branch roster3
Comment 7 Simon McVittie 2012-03-08 06:48:19 UTC
Reviewing on Bug #43826 because the two branches are closely related.
Comment 8 Alban Crequy 2012-03-20 12:03:08 UTC
(In reply to comment #7)
> Reviewing on Bug #43826 because the two branches are closely related.

Updated on branch roster5.
Comment 9 Jonny Lamb 2012-03-27 09:31:16 UTC
> + 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.
Comment 10 Jonny Lamb 2012-03-27 09:32:58 UTC
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? :-)
Comment 11 Jonny Lamb 2012-03-27 09:33:51 UTC
(In reply to comment #10)
> Created attachment 59115 [details]
> smaller test

Hm, I left in the 'priority' parameter test. Oh well.
Comment 12 Alban Crequy 2012-03-27 11:55:52 UTC
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.
Comment 13 Jonny Lamb 2012-03-27 12:27:20 UTC
Looks hot!
Comment 14 Alban Crequy 2012-03-28 05:06:28 UTC
(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

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.