Bug 43826

Here goes a review, all in all looks good to me, just some minor details.

+ * Download the contact list when it is not done automatically at
+ * connection
Missing dot "connection."

+ * If the #TpBaseContactList subclass does not implement
+ * %TP_TYPE_MUTABLE_CONTACT_LIST, it is an error to call this method.

Maybe
"This method should only be used if the #TpBaseContactList subclass implements
%TP_TYPE_MUTABLE_CONTACT_LIST."

But I see that other methods use "it is an error to call this method", so change it if you feel the need.

Otherwise seems good to me.

I updated the branch with:
- refresh Connection_Interface_Contact_List.xml with the changes from Bug #43035
- add missing dot

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?

+ * 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? 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?

+typedef void (*TpBaseContactListFunc) (
+ TpBaseContactList *self,
+ GAsyncReadyCallback callback,
+ gpointer user_data);

I don't like this name? It seems way too vague.

(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.

> +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.

I still would like a reply on questions from Comment #2. 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?

(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.

(In reply to comment #5)
> <andrunko> this is a programming error and should break code relying on it

The g_critical() will do that, if made fatal (we often do that).

> <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

I think you should return whichever value won't cause callers to do something dangerous in response. In the case of "will it download the roster or not?", either way is equally harmless, so my vote is "leave it as-is and get on with your life".

> > > +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.

I started this convention with the initial implementation of TpBaseContactList, and I'm inclined to agree with Alban:

> Rename it to TpBaseContactListDownloadFunc similar to
> TpBaseContactListRequestSubscriptionFunc?

Unlike the function for requesting subscriptions, nothing about that signature is specific to downloading contact lists; a function of that signature could do any action that doesn't have additional arguments.

What I was trying to avoid with ActOnContactsFunc is that we have one typedef per virtual method, even though a lot of them are remarkably similar (like ActOnContactsFunc, which does some verb (e.g. unblock) to a set of contacts). It's the same reasoning as having GDestroyNotify instead of GHashTableValueFreeFunc, GPtrArrayItemFreeFunc and GAsyncResultUserDataFreeFunc :-)

(In reply to comment #2)
> - was it the correct place to add the changes in the Mutable interface?

I don't really see why this is in the Mutable interface: whether we can delay downloading the contact list has nothing to do with whether we can modify it.
Mutable is about whether the contact list can be modified from the Telepathy side - for instance, in principle it should be missing from Facebook contact lists, because Facebook's XMPP server doesn't allow any edits.

Delayed-download works equally well on an immutable contact list, so I think these new things should replace an appropriate amount of ABI padding in TpBaseContactList itself.

(In reply to comment #6)
> (In reply to comment #5)
> > <andrunko> this is a programming error and should break code relying on it
> 
> The g_critical() will do that, if made fatal (we often do that).
> 
> > <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
> 
> I think you should return whichever value won't cause callers to do something
> dangerous in response. In the case of "will it download the roster or not?",
> either way is equally harmless, so my vote is "leave it as-is and get on with
> your life".
Agreed, just leave it as-is.

> > > > +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.
> 
> I started this convention with the initial implementation of TpBaseContactList,
> and I'm inclined to agree with Alban:
> 
> > Rename it to TpBaseContactListDownloadFunc similar to
> > TpBaseContactListRequestSubscriptionFunc?
> 
> Unlike the function for requesting subscriptions, nothing about that signature
> is specific to downloading contact lists; a function of that signature could do
> any action that doesn't have additional arguments.
> 
> What I was trying to avoid with ActOnContactsFunc is that we have one typedef
> per virtual method, even though a lot of them are remarkably similar (like
> ActOnContactsFunc, which does some verb (e.g. unblock) to a set of contacts).
> It's the same reasoning as having GDestroyNotify instead of
> GHashTableValueFreeFunc, GPtrArrayItemFreeFunc and GAsyncResultUserDataFreeFunc
> :-)
Makes sense, I have no preference here tbh, either way I am fine.

> (In reply to comment #2)
> > - was it the correct place to add the changes in the Mutable interface?
> 
> I don't really see why this is in the Mutable interface: whether we can delay
> downloading the contact list has nothing to do with whether we can modify it.
> Mutable is about whether the contact list can be modified from the Telepathy
> side - for instance, in principle it should be missing from Facebook contact
> lists, because Facebook's XMPP server doesn't allow any edits.
> 
> Delayed-download works equally well on an immutable contact list, so I think
> these new things should replace an appropriate amount of ABI padding in
> TpBaseContactList itself.
Hmm now the "Mutable" name makes sense to me, how did I miss that? After reading this rationale I have to agree that it should be in TpBaseContactList itself.

(In reply to comment #6)
> I think
> these new things should replace an appropriate amount of ABI padding in
> TpBaseContactList itself.

Done in the branch roster2 (see URL).

Branch roster3 rebased on master (which has the spec in).

> + * If the #TpBaseContactList subclass does not override
> + * download_async, it is an error to call this method.

Telepathy design principle number 47: don't be remotely crashable. If the Telepathy client calls Download() in a CM that is older than this telepathy-glib version, the CM shouldn't critical and crash.

I think the default implementation should be to raise TP_ERROR_NOT_IMPLEMENTED, asynchronously. You might find that g_simple_async_report_error_in_idle() is useful.

Other than that, this looks good.

> ContactList: add property DownloadAtConnection in TpBaseContactList

In this implementation, each CM's subclass basically has to implement storing a boolean property, and a getter method for it, which seems like a bit of a waste of time.

You could perhaps make life easier for CM authors by adding a boolean GObject property download-at-connection (default: TRUE) to the base class, using that in tp_base_contact_list_get_list_dbus_property, and removing get_download_at_connection; then the CM author would pass "download-at-connection" to g_object_new(), and that'd be all they needed to do. In older CMs, the default of TRUE would be used.

Fixed in branch roster4 and rebased on master

Nearly there, but some minor stuff that's probably worth appending to the branch rather than squashing in:

It's worth saying in the doc-comment that download-at-connection doesn't change anything in TpBaseContactsList's behaviour, and that implementations should check it when they become connected and in their Download method, and behave accordingly.

> + g_return_val_if_fail (cls->download_async != NULL, FALSE);
> +
> + return cls->download_finish (self, result, error);

Surely this should check download_finish, not download_async?

Life would be easier for CM authors if you provided tp_base_contact_list_get_download_at_connection() with the obvious implementation (looking in self->priv). Then:

> +gabble_roster_get_download_at_connection (TpBaseContactList *base)

... you could just call tp_base_contact_list_get_download_at_connection() instead.

Updated in branch roster5.

(In reply to comment #13)
> Nearly there, but some minor stuff that's probably worth appending to the
> branch rather than squashing in:
> 
> It's worth saying in the doc-comment that download-at-connection doesn't change
> anything in TpBaseContactsList's behaviour, and that implementations should
> check it when they become connected and in their Download method, and behave
> accordingly.

Done.

> > + g_return_val_if_fail (cls->download_async != NULL, FALSE);
> > +
> > + return cls->download_finish (self, result, error);
> 
> Surely this should check download_finish, not download_async?

Yes, fixed.

> Life would be easier for CM authors if you provided
> tp_base_contact_list_get_download_at_connection() with the obvious
> implementation (looking in self->priv). Then:
> 
> > +gabble_roster_get_download_at_connection (TpBaseContactList *base)
> 
> ... you could just call tp_base_contact_list_get_download_at_connection()
> instead.

Done.

For the record: TpBaseConnection->create_channel_managers() is called before the connection's parameters being filled by TpBaseProtocolClass->new_connection(). So GabbleRoster does not have the correct value of "download-at-connection" at creation. It is updated later by gabble_connection_set_property(PROP_DOWNLOAD_AT_CONNECTION). I added a comment there, as we said on IRC.

> + * @download_async: the implementation of
> + * tp_base_contact_list_download_async(); if a subclass does not implement
> + * this itself, the default implementation raise TP_ERROR_NOT_IMPLEMENTED,
> + * asynchronously. Since: 0.UNRELEASED

implementation *will* raise TP_…

> + * If the #TpBaseContactList subclass does not override
> + * download_async, the default implementation raise TP_ERROR_NOT_IMPLEMENTED,
> + * asynchronously.

same thing here.

> + * This is a virtual method which may be implemented using
> + * #TpContactList.download_finish. If the @result
> + * will be a #GSimpleAsyncResult, the default implementation may be used.

You mean #TpContactListClass.download_finish

Also, the docs make out that not implementing download_async() isn't /so/ bad as it'll just return NotImplemented but then we have this in constructed:

>   g_return_if_fail (cls->get_contact_list_persists != NULL);
> + g_return_if_fail (cls->download_async != NULL);

and this will prevent the rest of the base class being set up properly, no? I'm not sure what the best thing to do is here... perhaps g_warn_if_fail?

> +gboolean
> +tp_base_contact_list_get_download_at_connection (TpBaseContactList *self)

This isn't added to -sections.txt

(In reply to comment #15)
> > + * @download_async: the implementation of
> > + * tp_base_contact_list_download_async(); if a subclass does not implement
> > + * this itself, the default implementation raise TP_ERROR_NOT_IMPLEMENTED,
> > + * asynchronously. Since: 0.UNRELEASED
> 
> implementation *will* raise TP_…

Fixed

> > + * If the #TpBaseContactList subclass does not override
> > + * download_async, the default implementation raise TP_ERROR_NOT_IMPLEMENTED,
> > + * asynchronously.
> 
> same thing here.

Fixed

> > + * This is a virtual method which may be implemented using
> > + * #TpContactList.download_finish. If the @result
> > + * will be a #GSimpleAsyncResult, the default implementation may be used.
> 
> You mean #TpContactListClass.download_finish

Fixed

> Also, the docs make out that not implementing download_async() isn't /so/ bad
> as it'll just return NotImplemented but then we have this in constructed:
> 
> >   g_return_if_fail (cls->get_contact_list_persists != NULL);
> > + g_return_if_fail (cls->download_async != NULL);
> 
> and this will prevent the rest of the base class being set up properly, no? I'm
> not sure what the best thing to do is here... perhaps g_warn_if_fail?

tp_base_contact_list_class_init() sets up a default function:
  cls->download_async = tp_base_contact_list_download_async_default;

And the default function just returns TP_ERROR_NOT_IMPLEMENTED. So it should not happen.

> > +gboolean
> > +tp_base_contact_list_get_download_at_connection (TpBaseContactList *self)
> 
> This isn't added to -sections.txt

Added. I also moved the declarations outside of the <SUBSECTION changes> because it is no longer implemented with TP_TYPE_MUTABLE_CONTACT_LIST.

(In reply to comment #16)
> tp_base_contact_list_class_init() sets up a default function:
>   cls->download_async = tp_base_contact_list_download_async_default;
> 
> And the default function just returns TP_ERROR_NOT_IMPLEMENTED. So it should
> not happen.

Good point.

> Added. I also moved the declarations outside of the <SUBSECTION changes>
> because it is no longer implemented with TP_TYPE_MUTABLE_CONTACT_LIST.

Cool.

This all looks fine. gogogogogogo.

Pushed on git master: 234bf10..a705c6f

commit a705c6fbd77befe839aedc9890f6529f3f861abd
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Thu Feb 23 18:36:26 2012 +0000

    test: contact-list download
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43826

commit 3e0b118737007410230ba5ada721747a8818eeea
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Tue Mar 20 11:58:50 2012 +0000

    ContactList: implement helper tp_base_contact_list_get_download_at_connection
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43826

commit 8bde24822f73aeb9ec66f719d3066673d41d1c08
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Sat Feb 25 19:31:05 2012 +0000

    ContactList: add method Download in TpBaseContactList
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43826

commit aa1d2196aa5291af961d4ceff36507773a5393c0
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Fri Jan 20 13:51:01 2012 +0000

    ContactList: add property DownloadAtConnection in TpBaseContactList
    
    https://bugs.freedesktop.org/show_bug.cgi?id=43826