Bug 27676

16:47 < wjt> andrunko: hey, i just spoke to barisione irl
16:47 < wjt> andrunko: he thinks what he'd like is:
16:48 < wjt> • a feature which calls GetContactInfo (or uses the corresponding 
             contact attribute, if one is added), and listens for change 
             notification
16:48 < wjt> • a method to explicitly request the latest info
16:49 < wjt> the point being that then the application can safely turn on the 
             ContactInfo feature and get vCards opportunistically on XMPP, 
             without risking causing lots of network traffic
16:49 < wjt> and then once every few months, they can refresh
...
16:50 < smcv> for the method to explicitly request the latest info, presumably 
              we want both PendingContactInfo 
              *Tp::Contact::requestContactInfo(), and void 
              Tp::Connection::refreshContactInfo(SET<Tp::Contact>) for some 
              suitable type SET?

Here is a branch that adds the needed API on TpContact, it is not yet implemented. Please verify that's what we want so I can start implementing.

http://git.collabora.co.uk/?p=user/xclaesse/telepathy-glib.git;a=shortlog;h=refs/heads/contact-info

ContactInfo iface is now fully covered by high-level API, and my branch implements everything. Didn't test the code yet, though.

Turns out that RequestContactInfo has the same issues than what we had with RequestAvatar: DBus timeout for facebook contacts because server takes ages to reply.

ContactInfo is exactly the same case as avatars, some XMPP servers takes a lot of time to reply so we get dbus timeout. There are 2 solutions to this: 1) Let UI define the timeout for the call, but that's arbitrary value, and we can always find crap servers that takes more time. 2) Request+signal pattern.

ContactInfo spec actually have both: RefreshContactInfo+ContactInfoChanged and RequestContactInfo. Avatar iface was in the same situation and we decided to deprecate RequestAvatar because of that (or was there other reasons that does not apply here?). So what about also deprecating RequestContactInfo, or at least not wrap it in the high-level API?

(In reply to comment #4)
> Turns out that RequestContactInfo has the same issues than what we had with
> RequestAvatar: DBus timeout for facebook contacts because server takes ages to
> reply.
> 
> ContactInfo is exactly the same case as avatars, some XMPP servers takes a lot
> of time to reply so we get dbus timeout. There are 2 solutions to this: 1) Let
> UI define the timeout for the call, but that's arbitrary value, and we can
> always find crap servers that takes more time. 2) Request+signal pattern.
> 
> ContactInfo spec actually have both: RefreshContactInfo+ContactInfoChanged and
> RequestContactInfo. Avatar iface was in the same situation and we decided to
> deprecate RequestAvatar because of that (or was there other reasons that does
> not apply here?). So what about also deprecating RequestContactInfo, or at
> least not wrap it in the high-level API?

The point of it was for protocols where you don't get change notification for vCards (which includes XMPP), so you can explicitly update just one contact's info when the user mashes refresh in the window for them.

Here is empathy branch using the new API: http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=shortlog;h=refs/heads/contact-info

I cleaned my telepathy-glib branch and rebased on master. It is ready for review.

Note: I do not wrap RequestContactInfo, but only RefreshContactInfo for the reason explained in comment #4. It is trivial to add that later if we really want it.

I'd like a test, of course. Please update tests/lib/contacts-conn.[ch] from telepathy-qt4's copy (which lives in tests/lib/glib in tp-qt4 git) and use that? 

"Add high-level API on TpConnection to access ContactInfo properties" is missing from telepathy-qt4; I'll file a bug.

You don't have a binding for RequestContactInfo, but tp-qt4 does. If you add one, it should be an async method on TpContact which doesn't require the Feature. If you don't plan to add one, please open a separate enhancement/low bug for it. (Empathy might benefit from it if we add right-click -> "Contact Info..." or something, but I agree that it's not essential.)

I think connection.c is getting a bit unwieldy: could you move most of the extra Connection code (and in particular, the boxed types) to a new connection-contact-info.c?

It might also be worth factoring out a couple of internal functions into that file, with prototypes in connection-internal.h:

GList *_tp_contact_info_list_new_from_dbus_glib (const GPtrArray *);
GPtrArray *_tp_contact_info_list_to_dbus_glib (GList *);

> +struct _TpContactInfoFieldSpec
> +{
> +  gchar *name;
> +  GStrv parameters;
> +  TpContactInfoFieldFlags flags;
> +  guint max;
> +};

I'd like a 'priv' pointer for potential future expansion. In future, a good rule of thumb is to do this on every struct (at least ones that aren't stack-allocated). TpContactInfoField should have one, too.

> + * @name: A vCard field name, such as 'tel'.

This should mention that it's normalized to lower case.

> +tp_connection_get_contact_info_supported_fields (TpConnection *self)
> +{
> +  g_return_val_if_fail (TP_IS_CONNECTION (self), 0);

I'd prefer this spelled NULL rather than 0, when it represents a pointer.

This could be documented better; at the moment there's extra info, but no brief statement like "Returns a list of supported contact info fields for this connection".

My rule of thumb is that if the body of the method documentation is completely empty, then documenting via "Returns:" is enough, but if there's anything else to say about it, then the body of the method documentation should start with a brief statement of what the method does. This frequently ends up re-stating "Returns:", in which case move the main text to the body of the method documentation, and make the "Returns:" line very brief.

Things that are called "get_foo" and return a GList are usually (transfer container), i.e. they copy the GList but not the contents. I think it'd be worth doing that here.

There should be a boxed type for GList<TpContactInfoFieldSpec>, or TpContactInfoFieldSpec, or both.

I'm not sure whether there should be a property that mirrors this; we can always add one later, I suppose.

> TP_TYPE_CONTACT_INFO

I think this should be called TP_TYPE_CONTACT_INFO_LIST, with the corresponding change to the names of the copy and destroy functions.

> + * tp_connection_get_contact_info_flags:

This could be documented better; at the moment there's extra info, but no brief statement like "Returns the flags describing how contact info (vCards) behaves on this connection".

I'd like it to be mirrored as a G_TYPE_UINT property, that references the flags type in its documentation.

> + *  If this list is empty and the #TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT
> + *  flag is not set, any vCard type parameters may be used.

It might be worth having some methods on the TpContactInfoFieldSpec: perhaps simple accessors would be a waste of time, but these might be more useful:

tp_contact_info_field_spec_supports_all_parameters (self)
    (parameters == NULL || parameters[0] == NULL) &&
    (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) == 0

tp_contact_info_field_spec_has_exact_parameters (self)
    (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) != 0

tp_contact_info_field_spec_supports_parameter (self, parameter)
    (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) != 0 &&
    tp_strv_contains (parameters, parameter)

> + * Returns: (transfer none): a #TpContactInfoFlags

There's no need to annotate fundamental types (int, enum, float etc.) with (transfer); it's only useful on pointer types.

> + * tp_connection_set_contact_info_async:

This should have a cross-reference, something like:

"This method should not be expected to succeed if the result of tp_connection_get_contact_info_flags() does not include %TP_CONTACT_INFO_FLAG_CAN_SET."

> Add the TP_CONTACT_FEATURE_INFO feature on TpContact

This should be called TP_CONTACT_FEATURE_CONTACT_INFO, the "contact-info" property, etc. There's a double use of "contact" here: the information belongs to a contact (noun), but it's information on how to contact them (verb) outside the IM system (phone, postal address, etc.).

> +contacts_got_infos (TpConnection *connection,

Info is short for information, which doesn't pluralize (it's an aggregate noun, like air or sand), so this should be "contacts_got_[contact_]info".

> +tp_connection_refresh_contact_info_async (TpConnection *self,

Does it ever make sense to wait for the result of RefreshContactInfo? If it fails, what are you going to do about it? I think this should be a fire-and-forget method that calls tp_cli_connection_interface_contact_info_call_refresh_contact_info with a NULL callback (which means no reply message will ever appear on D-Bus).

> +      /* FIXME: Use TP_TOKEN_CONNECTION_INTERFACE_CONTACT_INFO_INFO when
> +       * available*/

It's now available.

For future reference
====================

> Replace NULL-terminated array of struct by GList* of struct.

It'd be easier to review this branch if this had been split up and squashed into the relevant earlier patches: you're under no obligation to keep earlier mis-designs in a branch's history.

> +          G_TYPE_STRING, (*p)->field_name,

You removed this later anyway, but it's just an awkward way to spell "p.field_name". Do the latter in future :-)

> -  GStrv parameters;
> -  GStrv field_value;
> +  gchar **parameters;
> +  gchar **field_value;

This should either have been squashed into the commit that introduced the struct, or committed separately with a reference to the g-i bug that requires it - it's not logically part of the commit "Replace NULL-terminated array of struct by GList* of struct" where it appears.

(In reply to comment #8)
> "Add high-level API on TpConnection to access ContactInfo properties" is
> missing from telepathy-qt4

Bug #28299, FYI.

(In reply to comment #8)
> I'd like a test, of course. Please update tests/lib/contacts-conn.[ch] from
> telepathy-qt4's copy (which lives in tests/lib/glib in tp-qt4 git) and use
> that? 

done, also opened bug #28302 to add AvatarData tests from tp-glib to tp-qt4.

(In reply to comment #8)
> You don't have a binding for RequestContactInfo, but tp-qt4 does. If you add
> one, it should be an async method on TpContact which doesn't require the
> Feature. If you don't plan to add one, please open a separate enhancement/low
> bug for it. (Empathy might benefit from it if we add right-click -> "Contact
> Info..." or something, but I agree that it's not essential.)

explained in comment #4 why I didn't wrap that method. In empathy I use Refresh+signal pattern and it works well.

> I think connection.c is getting a bit unwieldy: could you move most of the
> extra Connection code (and in particular, the boxed types) to a new
> connection-contact-info.c?

Good idea, done. Should I also make connection-contact-info.h?

> It might also be worth factoring out a couple of internal functions into that
> file, with prototypes in connection-internal.h:
> 
> GList *_tp_contact_info_list_new_from_dbus_glib (const GPtrArray *);
> GPtrArray *_tp_contact_info_list_to_dbus_glib (GList *);

We can do that when/if needed. 

> > +struct _TpContactInfoFieldSpec
> > +{
> > +  gchar *name;
> > +  GStrv parameters;
> > +  TpContactInfoFieldFlags flags;
> > +  guint max;
> > +};
> 
> I'd like a 'priv' pointer for potential future expansion. In future, a good
> rule of thumb is to do this on every struct (at least ones that aren't
> stack-allocated). TpContactInfoField should have one, too.

done

> > + * @name: A vCard field name, such as 'tel'.
> 
> This should mention that it's normalized to lower case.

That's a copy/paste from the spec, it does not tell about lower case there (should I open spec bug?). I fixed this by copy/pasting the description of "field_name" of TpContactInfoField.

> > +tp_connection_get_contact_info_supported_fields (TpConnection *self)
> > +{
> > +  g_return_val_if_fail (TP_IS_CONNECTION (self), 0);
> 
> I'd prefer this spelled NULL rather than 0, when it represents a pointer.

done, that was just a copy/past from the above function.

> This could be documented better; at the moment there's extra info, but no brief
> statement like "Returns a list of supported contact info fields for this
> connection".

done

> Things that are called "get_foo" and return a GList are usually (transfer
> container), i.e. they copy the GList but not the contents. I think it'd be
> worth doing that here.

That's usual? Can you provide some examples? I find this really confusing, either dup the return value or return the internal data, but half-duped is dangerous IMO.

I have to admit in my empathy code I have to g_list_copy() the return value (and not dup its elements) because I want to sort the list... but that depends on the usage you are doing, and it's really easy to copy the list in client code if needed.

> There should be a boxed type for GList<TpContactInfoFieldSpec>, or
> TpContactInfoFieldSpec, or both.

Made 4 boxed types, done.

> I'm not sure whether there should be a property that mirrors this; we can
> always add one later, I suppose.

I didn't make a property because that value can't change and can't be set, so that's a really limited property, a getter function is enough... Did the same for TpAvatarRequirements btw.

I think we can add this later is it turns out useful. That's easy with the boxed types.

> > TP_TYPE_CONTACT_INFO
> 
> I think this should be called TP_TYPE_CONTACT_INFO_LIST, with the corresponding
> change to the names of the copy and destroy functions.

done

> > + * tp_connection_get_contact_info_flags:
> 
> This could be documented better; at the moment there's extra info, but no brief
> statement like "Returns the flags describing how contact info (vCards) behaves
> on this connection".

done

> I'd like it to be mirrored as a G_TYPE_UINT property, that references the flags
> type in its documentation.

I didn't make a property for this for the same reason as avatar requirements and supported fields. Do you think we really should add a property? It can easily be added later though.

> > + *  If this list is empty and the #TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT
> > + *  flag is not set, any vCard type parameters may be used.
> 
> It might be worth having some methods on the TpContactInfoFieldSpec: perhaps
> simple accessors would be a waste of time, but these might be more useful:
> 
> tp_contact_info_field_spec_supports_all_parameters (self)
>     (parameters == NULL || parameters[0] == NULL) &&
>     (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) == 0
> 
> tp_contact_info_field_spec_has_exact_parameters (self)
>     (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) != 0
> 
> tp_contact_info_field_spec_supports_parameter (self, parameter)
>     (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) != 0 &&
>     tp_strv_contains (parameters, parameter)

This can easily be done later, I could open a bug for that?

> > + * Returns: (transfer none): a #TpContactInfoFlags
> 
> There's no need to annotate fundamental types (int, enum, float etc.) with
> (transfer); it's only useful on pointer types.

Wrong copy/paste, fixed.

> > + * tp_connection_set_contact_info_async:
> 
> This should have a cross-reference, something like:
> 
> "This method should not be expected to succeed if the result of
> tp_connection_get_contact_info_flags() does not include
> %TP_CONTACT_INFO_FLAG_CAN_SET."

done

> > Add the TP_CONTACT_FEATURE_INFO feature on TpContact
> 
> This should be called TP_CONTACT_FEATURE_CONTACT_INFO, the "contact-info"
> property, etc. There's a double use of "contact" here: the information belongs
> to a contact (noun), but it's information on how to contact them (verb) outside
> the IM system (phone, postal address, etc.).

done

> > +contacts_got_infos (TpConnection *connection,
> 
> Info is short for information, which doesn't pluralize (it's an aggregate noun,
> like air or sand), so this should be "contacts_got_[contact_]info".

Fixed eveywhere "infos" was used.

> > +tp_connection_refresh_contact_info_async (TpConnection *self,
> 
> Does it ever make sense to wait for the result of RefreshContactInfo? If it
> fails, what are you going to do about it? I think this should be a
> fire-and-forget method that calls
> tp_cli_connection_interface_contact_info_call_refresh_contact_info with a NULL
> callback (which means no reply message will ever appear on D-Bus).

As explained above, I think RequestContactInfo is not that useful. So in empathy I'm calling RefreshContactInfo and wait for the update signal. In the case refresh fails (Connection don' t have interface, or method not implemented in CM) I hide the contact details part of the contact info dialog. In the case that method succeed, I'm pretty sure to get "notify::contact-info" signal at some point, so I display a "please wait"-like label.

I think it's easy to give NULL for the callback anyway...

> > +      /* FIXME: Use TP_TOKEN_CONNECTION_INTERFACE_CONTACT_INFO_INFO when
> > +       * available*/
> 
> It's now available.

done

Still review- for lack of RequestContactInfo. I'll have a look at the updated branch and see if anything else needs fixing.

(In reply to comment #11)
> > > +tp_connection_refresh_contact_info_async (TpConnection *self,
> > 
> > Does it ever make sense to wait for the result of RefreshContactInfo? If it
> > fails, what are you going to do about it? I think this should be a
> > fire-and-forget method that calls
> > tp_cli_connection_interface_contact_info_call_refresh_contact_info with a NULL
> > callback (which means no reply message will ever appear on D-Bus).
> 
> As explained above, I think RequestContactInfo is not that useful. So in
> empathy I'm calling RefreshContactInfo and wait for the update signal. In the
> case refresh fails (Connection don' t have interface, or method not implemented
> in CM) I hide the contact details part of the contact info dialog. In the case
> that method succeed, I'm pretty sure to get "notify::contact-info" signal at
> some point, so I display a "please wait"-like label.

This is a bug, and you do need RequestContactInfo with a long (1 hour?) timeout. If the dialog is closed, you can cancel the TpProxyPendingCall to free up some memory; this doesn't waste the result, since ContactInfoChanged will still be emitted if an update comes back later. I suppose this means that the GAsyncResult binding should take a GCancellable, whose cancelled signal cancels the TpProxyPendingCall?

On a protocol (like Skype, I believe?) with the Push flag, i.e. where the CM gets contact info pushed to it (as with geolocation in XMPP), RefreshContactInfo() can be implemented correctly as a stub that just returns success, because the protocol already guarantees that up-to-date contact info will be pushed to you reasonably soon; so you won't get notify::contact-info until/unless the contact really changes it.

It would also be pedantically correct for Gabble to poll, then not emit the ContactInfoChanged signal if the contact info hasn't actually changed since last time it polled. As an implementation detail, Gabble sometimes emits ContactInfoChanged when it shouldn't, because Andre and I considered it to be unnecessary work to implement a generic compare-for-equality function for ContactInfo; however, if we'd done that, your Empathy branch would sometimes fail for XMPP too.

Rather than optimistically calling the method and seeing if it fails, you should check with tp_proxy_has_interface[_by_id], so you can avoid showing "please wait..." at all when there's nothing to wait for.

The crucial difference in Avatars is that you can know in advance whether a contact's avatar has changed or not, because the interface assumes that at least the tokens are pushed to the CM, and the token is enough to see changes: so whenever you call RequestAvatars, you already know whether to expect an avatar to turn up. In ContactInfo (on protocols without the Push flag, like XMPP), the only way is to poll.

> > I think connection.c is getting a bit unwieldy: could you move most of the
> > extra Connection code (and in particular, the boxed types) to a new
> > connection-contact-info.c?
> 
> Good idea, done. Should I also make connection-contact-info.h?

No need, just use connection.h for public and connection-internal.h for private as you did, I think. This also removes the risk of circular dependencies between the main and contact-info headers.

> > > + * @name: A vCard field name, such as 'tel'.
> > 
> > This should mention that it's normalized to lower case.
> 
> That's a copy/paste from the spec, it does not tell about lower case there
> (should I open spec bug?). I fixed this by copy/pasting the description of
> "field_name" of TpContactInfoField.

The spec hyperlinks to a Simple_Type that does say it's lower case, so there's no need for a spec bug. Copying only the text loses that link, making it necessary to say it explicitly.

> > Things that are called "get_foo" and return a GList are usually (transfer
> > container), i.e. they copy the GList but not the contents. I think it'd be
> > worth doing that here.
> 
> That's usual? Can you provide some examples? I find this really confusing,
> either dup the return value or return the internal data, but half-duped is
> dangerous IMO.

In telepathy-glib, tp_account_manager_get_valid_accounts() does this. It surprised me too, when reviewing that code, but Sjoerd and Jonny assure me that this is conventional; from a quick skim through devhelp, gdk_pixbuf_get_formats() also behaves like this.

> > > + *  If this list is empty and the #TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT
> > > + *  flag is not set, any vCard type parameters may be used.
> > 
> > It might be worth having some methods on the TpContactInfoFieldSpec: perhaps
> > simple accessors would be a waste of time, but these might be more useful:
> > 
> > tp_contact_info_field_spec_supports_all_parameters (self)
> >     (parameters == NULL || parameters[0] == NULL) &&
> >     (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) == 0
> > 
> > tp_contact_info_field_spec_has_exact_parameters (self)
> >     (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) != 0
> > 
> > tp_contact_info_field_spec_supports_parameter (self, parameter)
> >     (flags & TP_CONTACT_INFO_FIELD_FLAG_PARAMETERS_EXACT) != 0 &&
> >     tp_strv_contains (parameters, parameter)
> 
> This can easily be done later, I could open a bug for that?

Let's wait and see whether we need them; no need for a bug, I don't think. If they'd be useful, it'll become obvious.

> I think it's easy to give NULL for the callback anyway...

AIUI, it's unusual for _async functions to support being called for their side-effects with a NULL callback. If you do allow this (like tp_proxy_prepare_async() does), you should explicitly document it, and you should call the underlying tp_cli_*_call_* method with a NULL callback, rather than a callback that does nothing: this enables some optimizations in dbus-glib, libdbus and dbus-daemon (the reply message won't be sent at all, and dbus-daemon and the libraries won't waste memory on tracking it).

In this particular case, though, I think RefreshContacts should just be fire-and-forget: that's what's right for its use-case (which isn't Empathy).

More minor things:

> + * @parameters: The set of vCard type parameters which may be set on this field.

It might be worth reminding API users what we mean, since the terminology "type-parameters" is pretty obscure (but correct!): 'This might typically include "type=home" or "type=work"'.

(Confusingly, the only type-parameter generally seen in practice is called TYPE, so it would be correct to say "WORK is a typical value of the TYPE type-parameter"...)

> +TpContactInfoFieldSpec *tp_contact_info_field_spec_new (const gchar *name,
> +    GStrv parameters, TpContactInfoFieldFlags flags, guint max);
...
> +TpContactInfoField *tp_contact_info_field_new (const gchar *field_name,
> +    GStrv parameters, GStrv field_value);

Do these need to be public? If not, they could be _-prefixed in -internal.h. If they remain public, I think the replacement of NULL with { NULL } in tp_connection_get_contact_info_cb should move into tp_contact_info_field_spec_new, and similar for the other one.

I think tp_contact_info_field_new does need to be public (for the setter), but tp_contact_info_field_spec_new doesn't?

tp_contact_info_field_new_single_value (const gchar *, GStrv, const gchar *) would probably be a worthwhile addition - it'll be quite a common case in practice, I think. Or perhaps _new should just have a single value, and the GStrv-based one used internally should be tp_contact_info_field_new_multi_valued?

The single-valued one should say in its doc-comment "For instance, this is appropriate for the fn, label, tel, email and nickname fields", and the multi-valued one should say "For instance, this is appropriate for the org field (the first value is the organization name, and any subsequent values are organizational units in decreasing size order)".

We should probably also have "reminder" API for the two common multi-valued cases other than org:

tp_contact_info_field_new_adr (GStrv, const gchar *pobox, const gchar *extended, const gchar *street, const gchar *locality, const gchar *region, const gchar *postal_code, const gchar *country);

tp_contact_info_field_new_n (GStrv, const gchar *family, const gchar *given, const gchar *additional_names, const gchar *prefixes, const gchar *suffixes);

> +TpContactInfoFieldSpec *tp_contact_info_field_spec_copy (
> +    TpContactInfoFieldSpec *self);
...
> +TpContactInfoField *tp_contact_info_field_copy (TpContactInfoField *self);

@self should be const in both cases.

> + * Returns: a #TpContactInfoFlags

"a set of #TpContactInfoFlags" reads better

> struct _TpContactPrivate {
>+    /* info */
>+    GList *info;

Should still be renamed to contact_info. The comment is useless, but there should instead be a comment saying what type it is really: "list of TpContactInfoField".

> +contact_maybe_set_info (TpContact *self,

You prepend to a GList here. I'd prefer the prepend-and-reverse pattern, to keep the fields in the obvious order (in case there's some sort of "first address is most important" convention going on).

(In reply to comment #13)
> > +contact_maybe_set_info (TpContact *self,
> 
> You prepend to a GList here. I'd prefer the prepend-and-reverse pattern, to
> keep the fields in the obvious order (in case there's some sort of "first
> address is most important" convention going on).

GQueue! GQueue! O(1) appends and a more sensible API than bare GList!

(In reply to comment #14)
> GQueue! GQueue! O(1) appends and a more sensible API than bare GList!

... but also much less common usage in APIs, and no special-cased support in g-i. Perhaps using GQueue internally, and returning g_list_copy (g_queue_peek_head_link (q)), would be worthwhile though.

(In reply to comment #15)
> (In reply to comment #14)
> > GQueue! GQueue! O(1) appends and a more sensible API than bare GList!
> 
> ... but also much less common usage in APIs, and no special-cased support in
> g-i. Perhaps using GQueue internally, and returning g_list_copy
> (g_queue_peek_head_link (q)), would be worthwhile though.

(what's wrong with g_list_copy (q->head) ?)

(In reply to comment #12)
> > > Things that are called "get_foo" and return a GList are usually (transfer
> > > container), i.e. they copy the GList but not the contents. I think it'd be
> > > worth doing that here.
> > 
> > That's usual? Can you provide some examples? I find this really confusing,
> > either dup the return value or return the internal data, but half-duped is
> > dangerous IMO.
> 
> In telepathy-glib, tp_account_manager_get_valid_accounts() does this. It
> surprised me too, when reviewing that code, but Sjoerd and Jonny assure me that
> this is conventional; from a quick skim through devhelp,
> gdk_pixbuf_get_formats() also behaves like this.

Still thinking that's weird, but better be consistent with other APIs then. Done.

> > I think it's easy to give NULL for the callback anyway...
> 
> AIUI, it's unusual for _async functions to support being called for their
> side-effects with a NULL callback. If you do allow this (like
> tp_proxy_prepare_async() does), you should explicitly document it, and you
> should call the underlying tp_cli_*_call_* method with a NULL callback, rather
> than a callback that does nothing: this enables some optimizations in
> dbus-glib, libdbus and dbus-daemon (the reply message won't be sent at all, and
> dbus-daemon and the libraries won't waste memory on tracking it).
> 
> In this particular case, though, I think RefreshContacts should just be
> fire-and-forget: that's what's right for its use-case (which isn't Empathy).

Since we really want RequestContactInfo, I guess it's fair to make RefreshContactInfo be fire-and-forget. If we care about the return error/value I guess we can use RequestContactInfo.

done.

> > + * @parameters: The set of vCard type parameters which may be set on this field.
> 
> It might be worth reminding API users what we mean, since the terminology
> "type-parameters" is pretty obscure (but correct!): 'This might typically
> include "type=home" or "type=work"'.
> 
> (Confusingly, the only type-parameter generally seen in practice is called
> TYPE, so it would be correct to say "WORK is a typical value of the TYPE
> type-parameter"...)

done.

> > +TpContactInfoFieldSpec *tp_contact_info_field_spec_new (const gchar *name,
> > +    GStrv parameters, TpContactInfoFieldFlags flags, guint max);
> ...
> > +TpContactInfoField *tp_contact_info_field_new (const gchar *field_name,
> > +    GStrv parameters, GStrv field_value);
> 
> Do these need to be public? If not, they could be _-prefixed in -internal.h. If
> they remain public, I think the replacement of NULL with { NULL } in
> tp_connection_get_contact_info_cb should move into
> tp_contact_info_field_spec_new, and similar for the other one.
> 
> I think tp_contact_info_field_new does need to be public (for the setter), but
> tp_contact_info_field_spec_new doesn't?

tp_contact_info_field_spec_new() is the only not needed.

We need all copy/free functions because getter functions returns new container but not new elements, so if one want to keep the list it needs to copy and free later.

We also need tp_contact_info_field_new() to use for SetContactInfo.

So really the only optional is tp_contact_info_field_spec_new() so I kept public for API completness... but can easily be moved to connection-internal.h if you like.

> tp_contact_info_field_new_single_value (const gchar *, GStrv, const gchar *)
> would probably be a worthwhile addition - it'll be quite a common case in
> practice, I think. Or perhaps _new should just have a single value, and the
> GStrv-based one used internally should be
> tp_contact_info_field_new_multi_valued?
> 
> The single-valued one should say in its doc-comment "For instance, this is
> appropriate for the fn, label, tel, email and nickname fields", and the
> multi-valued one should say "For instance, this is appropriate for the org
> field (the first value is the organization name, and any subsequent values are
> organizational units in decreasing size order)".
> 
> We should probably also have "reminder" API for the two common multi-valued
> cases other than org:
> 
> tp_contact_info_field_new_adr (GStrv, const gchar *pobox, const gchar
> *extended, const gchar *street, const gchar *locality, const gchar *region,
> const gchar *postal_code, const gchar *country);
> 
> tp_contact_info_field_new_n (GStrv, const gchar *family, const gchar *given,
> const gchar *additional_names, const gchar *prefixes, const gchar *suffixes);

Totally agree those are useful. But can we delay those API helpers and get the core merged first?

> > +TpContactInfoFieldSpec *tp_contact_info_field_spec_copy (
> > +    TpContactInfoFieldSpec *self);
> ...
> > +TpContactInfoField *tp_contact_info_field_copy (TpContactInfoField *self);
> 
> @self should be const in both cases.

done. Note that we forgot that for tp_avatar_requirements_copy(). Is it too late?

> > + * Returns: a #TpContactInfoFlags
> 
> "a set of #TpContactInfoFlags" reads better

done

> > struct _TpContactPrivate {
> >+    /* info */
> >+    GList *info;
> 
> Should still be renamed to contact_info. The comment is useless, but there
> should instead be a comment saying what type it is really: "list of
> TpContactInfoField".

done

> > +contact_maybe_set_info (TpContact *self,
> 
> You prepend to a GList here. I'd prefer the prepend-and-reverse pattern, to
> keep the fields in the obvious order (in case there's some sort of "first
> address is most important" convention going on).

done

Also added RequestContactInfo wrapper in my branch. So it's ready for a new review round.

(In reply to comment #12)
> you do need RequestContactInfo with a long (1 hour?)
> timeout.

but:

+  tp_cli_connection_interface_contact_info_call_request_contact_info (
+      self->priv->connection, -1, self->priv->handle, contact_info_request_cb,

-1 means the default timeout, usually 25 seconds. Please either take a timeout as an explicit parameter (re-interpreting -1 as an hour or something), or just set a long hard-coded timeout.

> If the dialog is closed, you can cancel the TpProxyPendingCall to free
> up some memory

Using the TpContact as the weak_object as you've done is not actually useful, because GSimpleAsyncResult refs the source object (to ensure that the callback runs precisely once). This means the only way to cancel the call is with the TpProxyPendingCall.

The intended cancellation semantics of GAsyncResult and TpProxyPendingCall are not the same, unfortunately; a GAsyncResult async call always runs its callback exactly once, but a TpProxy call does not run its callback if cancelled. I think you'll need to make a context struct that contains both the GSimpleAsyncResult and the TpProxyPendingCall (or NULL if it already completed), and use that as the user_data for the async call and the cancellation signal. Alternatively, you could attach the TpProxyPendingCall to the GSimpleAsyncResult as qdata, perhaps?

Either way, you need to keep track of whether it's already finished, since it's invalid (use-after-free!) to cancel a TpProxyPendingCall that has already been cancelled or called its callback.

> So really the only optional is tp_contact_info_field_spec_new() so I kept
> public for API completness... but can easily be moved to connection-internal.h
> if you like.

I'd be inclined to follow the general principle of "if it doesn't have to be public, don't make it public" for that one.

> Totally agree those are useful. But can we delay those API helpers and get the
> core merged first?

Sure, we can do that later.

> done. Note that we forgot that for tp_avatar_requirements_copy(). Is it too
> late?

No, we can generally make things "more const" without breaking API. Please do that on a trivia branch and we can fast-track it in.

(In reply to comment #19)
> (In reply to comment #12)
> > you do need RequestContactInfo with a long (1 hour?)
> > timeout.
> 
> but:
> 
> +  tp_cli_connection_interface_contact_info_call_request_contact_info (
> +      self->priv->connection, -1, self->priv->handle, contact_info_request_cb,
> 
> -1 means the default timeout, usually 25 seconds. Please either take a timeout
> as an explicit parameter (re-interpreting -1 as an hour or something), or just
> set a long hard-coded timeout.

Already did that, I amended the commit. You probably pulled too early :)

> > If the dialog is closed, you can cancel the TpProxyPendingCall to free
> > up some memory
> 
> Using the TpContact as the weak_object as you've done is not actually useful,
> because GSimpleAsyncResult refs the source object (to ensure that the callback
> runs precisely once). This means the only way to cancel the call is with the
> TpProxyPendingCall.
> 
> The intended cancellation semantics of GAsyncResult and TpProxyPendingCall are
> not the same, unfortunately; a GAsyncResult async call always runs its callback
> exactly once, but a TpProxy call does not run its callback if cancelled. I
> think you'll need to make a context struct that contains both the
> GSimpleAsyncResult and the TpProxyPendingCall (or NULL if it already
> completed), and use that as the user_data for the async call and the
> cancellation signal. Alternatively, you could attach the TpProxyPendingCall to
> the GSimpleAsyncResult as qdata, perhaps?
> 
> Either way, you need to keep track of whether it's already finished, since it's
> invalid (use-after-free!) to cancel a TpProxyPendingCall that has already been
> cancelled or called its callback.

None of our wrappers for tp_cli_foo_call_bar() are cancellable. IIRC the reason is the semantic of GCancellable is to stop a running operation (like reading a file) but a DBus call can't be stopped, we can just ignore the callback... So I get told (sjoerd IIRC) that our solution is to use TpWeakRef as user_data, and in the callback we can check if our object (the GtkDialog in empathy case) is still alive, and if it is NULL we can return directly.

Do you object?

> > So really the only optional is tp_contact_info_field_spec_new() so I kept
> > public for API completness... but can easily be moved to connection-internal.h
> > if you like.
> 
> I'd be inclined to follow the general principle of "if it doesn't have to be
> public, don't make it public" for that one.

ok, removed it.

(In reply to comment #20)
> None of our wrappers for tp_cli_foo_call_bar() are cancellable. IIRC the reason
> is the semantic of GCancellable is to stop a running operation (like reading a
> file) but a DBus call can't be stopped, we can just ignore the callback... So I
> get told (sjoerd IIRC) that our solution is to use TpWeakRef as user_data, and
> in the callback we can check if our object (the GtkDialog in empathy case) is
> still alive, and if it is NULL we can return directly.

Normally I'd agree, but normally our D-Bus calls time out after 25 seconds (and the TpConnection and TpContact get unreffed at that point); if the timeout is an hour, then the TpConnection and TpContact stay around for up to an hour too. I wonder whether that's enough reason to offer a way to cancel.

However, thinking about it more, it's only the TpConnection, TpContact and TpWeakRef that are affected, and a typical UI will keep the connection and contact around anyway; if the connection disconnects, we hope the connection manager will correctly reply with an error to all pending requests. Perhaps you could add a test to Gabble to check that it does this?

(I agree that it would be misleading to offer a GCancellable on an operation with side-effects, like a "set" call.)

> +      G_OBJECT (self));

Passing the TpContact through the tp_cli call as the weak_object, when we know that it's reffed by the simple result and thus shouldn't be finalized during the call, at least deserves a comment.

When doing this in conjunction with a TpProxyPendingCall, I'd be inclined to say that it's risky to have a weak_object (cancelling manually after a weak_object might have died is a use-after-free); just put the TpContact in the context struct, which you have anyway.

> +  tp_proxy_pending_call_cancel (data->call);

I think this should be protected by "if (data->call != NULL)"...

>  contact_info_request_cb (TpConnection *connection,

... and this should set data->call to NULL on entry.

> +          g_cancellable_disconnect (data->cancellable, data->cancellable_id);

The docs say: "Additionally, in the event that a signal handler is currently running, this call will block until the handler has finished. Calling this function from a "cancelled" signal handler will therefore result in a deadlock."
tp_proxy_pending_call_cancel results in the pending call's data being freed, and its destructor calls g_cancellable_disconnect.

However, tp_proxy_pending_call_cancel frees the pending call in an idle because of Bug #14750, which saves us from this potential bug.

I think this is OK, but very very subtle: it deserves a comment in connection-contact-info.c saying that this code only works because of the fix for Bug #14750, and tp_proxy_pending_call_cancel() should gain a comment noting that this method relies on the idle.

+ * Note that requesting the vCard from network can take significant time, so
+ * a bigger timeout is set on the underlying DBus call. That's also the reason
+ * we provide @cancellable in case the UI is not interested in the vCard
+ * anymore.

en_GB nitpicking:

"from the network"

"underlying D-Bus call"

and I'd rephrase the second sentence as:

@cancellable can be cancelled to free resources used in the D-Bus call if the caller is no longer interested in the vCard.

(In reply to comment #22)
> > +      G_OBJECT (self));
> 
> Passing the TpContact through the tp_cli call as the weak_object, when we know
> that it's reffed by the simple result and thus shouldn't be finalized during
> the call, at least deserves a comment.
> 
> When doing this in conjunction with a TpProxyPendingCall, I'd be inclined to
> say that it's risky to have a weak_object (cancelling manually after a
> weak_object might have died is a use-after-free); just put the TpContact in the
> context struct, which you have anyway.

done

> > +  tp_proxy_pending_call_cancel (data->call);
> 
> I think this should be protected by "if (data->call != NULL)"...
> 
> >  contact_info_request_cb (TpConnection *connection,
> 
> ... and this should set data->call to NULL on entry.

I made it assert that data->call != NULL, with the new code it should never happen.

> > +          g_cancellable_disconnect (data->cancellable, data->cancellable_id);
> 
> The docs say: "Additionally, in the event that a signal handler is currently
> running, this call will block until the handler has finished. Calling this
> function from a "cancelled" signal handler will therefore result in a
> deadlock."
> tp_proxy_pending_call_cancel results in the pending call's data being freed,
> and its destructor calls g_cancellable_disconnect.
> 
> However, tp_proxy_pending_call_cancel frees the pending call in an idle because
> of Bug #14750, which saves us from this potential bug.
> 
> I think this is OK, but very very subtle: it deserves a comment in
> connection-contact-info.c saying that this code only works because of the fix
> for Bug #14750, and tp_proxy_pending_call_cancel() should gain a comment noting
> that this method relies on the idle.

I made the cancel in idle callback, it's safer IMO.

> + * Note that requesting the vCard from network can take significant time, so
> + * a bigger timeout is set on the underlying DBus call. That's also the reason
> + * we provide @cancellable in case the UI is not interested in the vCard
> + * anymore.
> 
> en_GB nitpicking:
> 
> "from the network"
> 
> "underlying D-Bus call"
> 
> and I'd rephrase the second sentence as:
> 
> @cancellable can be cancelled to free resources used in the D-Bus call if the
> caller is no longer interested in the vCard.

fixed

> -  g_object_unref (cancellable);
>  
> -  g_idle_add (contact_info_request_cancel, cancellable);
> +  g_idle_add_full (G_PRIORITY_HIGH, contact_info_request_cancel,
> +      cancellable, NULL);
>  
>    g_main_loop_run (result.loop);
>    g_assert_no_error (result.error);
>  
>    reset_result (&result);
>    tp_handle_unref (service_repo, handle);
> +  g_object_unref (cancellable);

Strictly speaking, the g_idle_add_full and the g_object_unref should be combined like so:

  g_idle_add_full (G_PRIORITY_HIGH, contact_info_request_cancel,
      cancellable, g_object_unref);

to guarantee that the cancellable lives long enough for the idle to go off. r+ with that change.

(In reply to comment #25)
> > -  g_object_unref (cancellable);
> >  
> > -  g_idle_add (contact_info_request_cancel, cancellable);
> > +  g_idle_add_full (G_PRIORITY_HIGH, contact_info_request_cancel,
> > +      cancellable, NULL);
> >  
> >    g_main_loop_run (result.loop);
> >    g_assert_no_error (result.error);
> >  
> >    reset_result (&result);
> >    tp_handle_unref (service_repo, handle);
> > +  g_object_unref (cancellable);
> 
> Strictly speaking, the g_idle_add_full and the g_object_unref should be
> combined like so:
> 
>   g_idle_add_full (G_PRIORITY_HIGH, contact_info_request_cancel,
>       cancellable, g_object_unref);
> 
> to guarantee that the cancellable lives long enough for the idle to go off. r+
> with that change.

asserts in contact_info_request_cancelled_cb() will fail if the g_main_loop_run() returns before the idle callback is called, so that shouldn't be a problem... But did the change anyway, it's nicer like that :)

Branch merged in master, finally!

Thanks for the detailled reviews :D