[Bug 27676] ContactInfo high-level API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #11 from Xavier Claessens <xclaesse at gmail.com> 2010-05-30 07:57:54 PDT ---
(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

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