[Bug 27676] ContactInfo high-level API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #17 from Xavier Claessens <xclaesse at gmail.com> 2010-06-02 03:47:25 PDT ---
(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

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