We should implement the Telepathy spec change from Bug #43035 with tpglib help from Bug #43826 in Gabble.
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
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.