[Bug 27676] ContactInfo high-level API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #12 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-31 03:30:14 PDT ---
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).

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