[Bug 43826] avoid downloading the roster at every connection

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=43826

--- Comment #5 from Andre Moreira Magalhaes <andrunko at gmail.com> 2012-01-19 08:31:17 PST ---
(In reply to comment #4)
> (In reply to comment #3)
> > + * tp_base_contact_list_get_download_at_connection:
> > ...
> > + * The default implementation always returns %TRUE, which is correct for most
> > + * protocols;
> > 
> > Yep, seems good. However:
> > 
> > + g_return_val_if_fail (TP_IS_BASE_CONTACT_LIST (self), FALSE);
> > 
> > and
> > 
> > + if (!TP_IS_MUTABLE_CONTACT_LIST (self))
> > +   return FALSE;
> > 
> > and
> > 
> > + g_return_val_if_fail (iface != NULL, FALSE);
> > 
> > and
> > 
> > + g_return_val_if_fail (iface->get_download_at_connection != NULL, FALSE);
> > 
> > I realise these are corner cases but surely these should be returning TRUE in
> > the error case?
> 
> Ok.
> 
> > It might be nice time to check that all the other functions
> > that use tp_base_contact_list_true_func actually do the right thing?
> 
> Other tp_base_contact_list_true_func functions are:
> - tp_base_contact_list_can_change_contact_list
> - tp_base_contact_list_get_request_uses_message
> - tp_base_contact_list_can_block
> 
> But it does not make sense to return TRUE in case of error, because in case of
> programmable error, it will not go further (changing the contact list,
> block...). And if the object does not implement the mutable interface, we must
> return FALSE. It breaks the tests when I tried to return TRUE.
As discussed with Alban on IRC, I believe every g_return_if_fail here should
return FALSE.

Quote from IRC:
<andrunko> albanc, I think you should return FALSE on the g_return_val_if_fails
<andrunko> in get_download_at_connection
<andrunko> for example
<andrunko> + g_return_val_if_fail (TP_IS_BASE_CONTACT_LIST (self), TRUE);
<andrunko> if the object is not a basecontactlist, it will not
download_at_connection
<andrunko> this is a programming error and should break code relying on it
<albanc> andrunko, notice that it was FALSE before and Jonny asked me to change
it to TRUE :p
<andrunko> albanc, yeah, I know, but I don't agree with him
<andrunko> also everywhere else returns FALSE
<andrunko> I think it does not make sense to return true
<albanc> andrunko, for programmable errors (g_return_val_if_fail), I don't
think it matter a lot...
<andrunko> albanc, it actually does not matter, you could return whatever you
want, the code is already buggy, but I think you should still return FALSE
<andrunko> in every place a IS_SOMETHING_EXPECTED fails
<albanc> I agree that it means the code is already buggy
<albanc> yeah

What do you think Jonny?

> > +typedef void (*TpBaseContactListFunc) (
> > + TpBaseContactList *self,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data);
> > 
> > I don't like this name? It seems way too vague.
> 
> I changed it to TpBaseContactListAsyncFunc. The other names (such as
> TpBaseContactListActOnContactsFunc) are named after their signature only since
> they are used for several purposes.
Rename it to TpBaseContactListDownloadFunc similar to
TpBaseContactListRequestSubscriptionFunc?

> I still would like a reply on questions from Comment #2. 

> > I would like a review on:
> > - does the branch break the API / API? e.g. 
> > adding new fields in _TpMutableContactListInterface
> > - was it the correct place to add the changes in the Mutable interface?
No, it does not break API/ABI if they are added in the end of GInterfaces and
are optional.

> I also updated the
> documentation of TpMutableContactListInterface and it says "Since: 0.13.0". Is
> it correct? How does it work when some fields are added in different versions?
Afaik when making a release, all Since: UNRELEASED are updated to the version
being released. So just add a Since: UNRELEASED to the new methods in the
interface. The interface is still "Since: 0.13.0" as the interface was added in
this version.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.