[Bug 27676] ContactInfo high-level API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #8 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-28 04:31:46 PDT ---
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.

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