Bug 32609

Branch ready for review :)

"states" sounds like a poor name to me but I can't think of a better one.

Bouh, no test :(

I shamelessly copied naming from tp_base_contact_list_dup_states(). But now I'm wondering if I should rather have 3 getters instead of one. Also wondering if I should rename property "subscription-state" to "subscribe" and "publish-state" to "publish". Do you have an opinion on this?

For tests, I have patched contacts-conn.c to use a TpBaseContactList for my contactsd work on Harmattan, it is in my todo list to backport this to tp-glib.

I modified a bit the API to be more convenient, and I added unit tests

> + * tp_contact_get_publish_request:

Please document whether/under what circumstances this returns %NULL or "". One nice set of semantics would be to guarantee that it's non-NULL if and only if publish-state is REQUESTED; an alternative nice set of semantics would be to guarantee that it's never NULL (in which case you'd either need a special case in the getter, or initialization to g_strdup (NULL)).

The doc-comments for the getters should cross-reference to the properties. I like the wording "Returns: the value of #TpContact:publish-state", personally.

> +      TP_SUBSCRIPTION_STATE_UNKNOWN,
> +      TP_SUBSCRIPTION_STATE_YES,
> +      TP_SUBSCRIPTION_STATE_UNKNOWN,

Can this go above YES if you're given a stupid value from D-Bus? I think it can - so the limit needs to be G_MAXUINT32. Or, clip out-of-range values to UNKNOWN whenever you receive information from D-Bus.

I don't like using an enum member as the upper limit: if anything it should be (NUM_TP_SUBSCRIPTION_STATES - 1) (which is numerically equal, at the moment).

> Simplify contacts-conn by using TpConnectionPresenceType

I'm not keen on this; I know it's only test code, but it's a misleading thing to do. The point of not using TpConnectionPresenceType is that basically no connection managers will have exactly one status per TpConnectionPresenceType - in the general case, a CM will lack support for some of the "important" (i.e. directly mapped to presence type) statuses, and will have some statuses that share a TpConnectionPresenceType (e.g. in Gabble, "available" and "chat" (free for chat) are both AVAILABLE; in MSN, AIUI, "on-the-phone" and "having-lunch" could both be BUSY, but there is no "busy" as such).

(In reply to comment #5)
> > + * tp_contact_get_publish_request:
> 
> Please document whether/under what circumstances this returns %NULL or "". One
> nice set of semantics would be to guarantee that it's non-NULL if and only if
> publish-state is REQUESTED; an alternative nice set of semantics would be to
> guarantee that it's never NULL (in which case you'd either need a special case
> in the getter, or initialization to g_strdup (NULL)).

With current implementation, it is guaranteed to be non-NULL once the feature is prepared. I think that's a reasonable behavior. I added documentation for it.

> The doc-comments for the getters should cross-reference to the properties. I
> like the wording "Returns: the value of #TpContact:publish-state", personally.

Fixed

> > +      TP_SUBSCRIPTION_STATE_UNKNOWN,
> > +      TP_SUBSCRIPTION_STATE_YES,
> > +      TP_SUBSCRIPTION_STATE_UNKNOWN,
> 
> Can this go above YES if you're given a stupid value from D-Bus? I think it can
> - so the limit needs to be G_MAXUINT32. Or, clip out-of-range values to UNKNOWN
> whenever you receive information from D-Bus.
> 
> I don't like using an enum member as the upper limit: if anything it should be
> (NUM_TP_SUBSCRIPTION_STATES - 1) (which is numerically equal, at the moment).

Ok, I changed to be in [0, G_MAXUINT]. If/When we want to restrict the values, we could use mkenum anyway.

> > Simplify contacts-conn by using TpConnectionPresenceType
> 
> I'm not keen on this; I know it's only test code, but it's a misleading thing
> to do. The point of not using TpConnectionPresenceType is that basically no
> connection managers will have exactly one status per TpConnectionPresenceType -
> in the general case, a CM will lack support for some of the "important" (i.e.
> directly mapped to presence type) statuses, and will have some statuses that
> share a TpConnectionPresenceType (e.g. in Gabble, "available" and "chat" (free
> for chat) are both AVAILABLE; in MSN, AIUI, "on-the-phone" and "having-lunch"
> could both be BUSY, but there is no "busy" as such).

Ok dropped that commit

Implementation looks good to me, just some documentation details to fix.
I'd prefer to get an API++ from smcv before merging this though.

+ *  (available since UNRELEASED)

We use 0.13.UNRELEASED

+   * TpContact:publish-request:
Documentation is unclear about what's returned if TpContact:publish-state is
not TP_SUBSCRIPTION_STATE_ASK. I think it's "", we should document that.

+   * @contact: A #TpContact
should be "a #TpContact". Same for other arguments.

The API looks fine. Drive-by documentation nits:

> + *  #TpContact:publish-state and #TpContact:publish-request. Require a
> + *  Connection implementing ContactList interface.

... Requires a Connection implementing the %TP_IFACE_CONNECTION_INTERFACE_CONTACT_LIST interface.

The use of @self as the argument name in tp_contact_get_subscribe_state(), tp_contact_get_publish_state() and tp_contact_get_publish_request() meant I had to read the documentation at least twice, due to confusion with the other meaning of "self" (SelfHandle, etc.).

Replacing "@self" with "this contact" or even "this remote contact" in the text of the doc-comment would improve it, I think - an exception to the usual rule that more cross-references make documentation better :-)

Perhaps saying "the local user" instead of "the user" in these docstrings would also be a good way to disambiguate how our jargon works.

> +   * A #TpSubscriptionState indicating the state of the user's subscription
> +   * to contact's presence.

English grammar: "subscription to this contact's presence" (i.e. add "this")

> +   * A #TpSubscriptionState indicating the state of contact's subscription to
> +   * the user's presence.

English grammar: "the state of this contact's subscription" (i.e. add "this")

Fixed all those comments and merged. Thanks.