[Bug 27676] ContactInfo high-level API

[bugzilla-daemon at freedesktop.org]

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

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

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