Bug 21787

This interface could also contain the names we have given to other people ("pet names") as per http://www.xmpp.org/extensions/xep-0165.html "Best Practices to Discourage JID
Mimicking", solving part of Bug #14540.

It could either be an interface on the Connection, or a sidecar (Bug #24661).

*** Bug 21837 has been marked as a duplicate of this bug. ***

As per Bug #21837 we should make sure that the new API supports requesting subscription for someone again, even if you already did so.

Increasing priority because ContactList channels are a major API wart, and replacing them like this would make it much easier to solve Bug #26205. Rob doesn't think it's worth implementing high-level API for contact lists in telepathy-glib without doing this first.

Here's a first draft for ContactList, ContactGroups and (for Bug #14540) Nicknames.

I've deferred contact blocking for now (smcv/wip-blocking branch if anyone wants to pick it up).

Source:

http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/contact-list

HTML:

http://people.freedesktop.org/~smcv/telepathy-spec-contact_list/spec/

Here's a sketch of how a GLib API could look:

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/sketch-contact-lists

(I haven't compared this with telepathy-qt4 yet.)

Looking at the API again, I think I was wrong to make RemoveContacts() take a mode parameter. It should be three methods:

* RemoveContacts()
* RemoveSubscriptions() (advanced use only)
* RemovePublications() (likewise)

Bugs:

* We need a way to signal publication requests *with the associated message* (or just signal them and have a function-based accessor for the message, or something). telepathy-qt4 doesn't yet do this either, although it does have a convenient presencePublicationRequested() signal.

Extra features in telepathy-qt4:

* Tp::Contact has addedToGroup(), removedFromGroup() signals, whereas in my API sketch, TpContact only has notify::groups - this would be easy to add though

* telepathy-qt4 has canRequestPresenceSubscription(), canRemovePresenceSubscription(), etc., whereas this new API simplifies it to CanChangeSubscriptions - hopefully we don't need the extra complexity and can just ignore it

* telepathy-qt4 allows messages in more places - hopefully requesting subscription is the only one that actually matters

* telepathy-qt4 can block and unblock contacts, which I've deferred here

* telepathy-qt4 also emits a signal when groups' members change - do we need this, or can it just be inferred from the contacts' signals?

Omissions in telepathy-qt4:

* telepathy-qt4 doesn't yet have API for replacing a contact's groups (in the older D-Bus API it uses, this would be unnecessarily hard)

* telepathy-qt4 doesn't have API for "completely remove a contact" yet (in the older D-Bus API it uses, this is removal from "stored")

Misc differences:

* telepathy-qt4 puts the contact stuff on a helper object, Tp::ContactManager

Notable similarities:

* PresenceState is a 1:1 copy of what's already in telepathy-qt4

* Groups.Groups is Tp::ContactManager::allKnownGroups() in telepathy-qt4

* ContactList.Contacts is Tp::ContactManager::allKnownContacts()

From a spec meeting:

Rename Nicknames to Names; move local-alias there; add a pretty-id or similar.

When do we start emitting ContactsChanged? After GetContactListAttributes() has returned or the roster has been retrieved, whichever is the later. (On Salut, you don't know when you have got the whole roster.)

Should ContactChanged, ContactRemoved be plural? On Salut you can get a flood of contacts after the original download (because you join two switches together).

Should have three separate removal methods:

• Unsubscribe()
• Unpublish()
• Remove()

Most UIs will just use the last one. Remove() also removes the alias.

GroupsChanged should be au as as.

Make GroupCreated plural.

(In reply to comment #8)
> * We need a way to signal publication requests *with the associated message*
> (or just signal them and have a function-based accessor for the message, or
> something).

I've added a "publish-request" attribute to the D-Bus API. We can bind this appropriately.

(Or, a mad idea: perhaps publication requests should be channels?)

(In reply to comment #9)
> Rename Nicknames to Names; move local-alias there; add a pretty-id or similar.

Done.

> When do we start emitting ContactsChanged? After GetContactListAttributes() has
> returned or the roster has been retrieved, whichever is the later. (On Salut,
> you don't know when you have got the whole roster.)

Done. I also changed ContactsChanged so it's emitted when the roster is retrieved, now that it's plural anyway.

> Should ContactChanged, ContactRemoved be plural? On Salut you can get a flood
> of contacts after the original download (because you join two switches
> together).

Done. They're now a combined ContactsChanged signal with Changed and Removed arguments.

> Should have three separate removal methods:
> 
> • Unsubscribe()
> • Unpublish()
> • Remove()

I left Remove called RemoveContacts.

Note that there is a naming collision: MailNotification also has an Unsubscribe method. Do we care about deficient bindings where this would be hard to call (are there any?), or about bindings where this is hard to implement (dbus-python services)? If we do, we should fix this before MailNotification is undrafted.

> GroupsChanged should be au as as.

Done. I also changed the group rename/removal signals to have a strictly-speaking-redundant GroupsChanged emission, now that GroupsChanged is cheaper, to make it easier for clients to track a subset of the group state.

> Make GroupCreated plural.

Done.

Mikhail Z confirms that, as I'd assumed in the current proposal, we can safely treat groups as if they were identified by human-readable name, and we can require CMs for protocols where this is not the case to append a number to disambiguate or something. (See Bug 13419.)

Looks pretty good to me.
Some questions/comments:

"The GetContactListAttributes method will wait until the contact list is available before returning. Using a method also allows extra attributes to be retrieved at the same time."
In practice that means that any client (include tp-glib's client side API) will have to pass an "infinite" timeout when calling this function as fetching the roster can take a while with slow servers.

How UI is supposed to display the DefaultGroup? It won't be localized so we can't really display it as it.

Can we have Names without ContactList?

Does Name replae Aliasing?

SetLocalAlias (s: Alias) → nothing
Should be: SetLocalAlias (u: handle, s: Alias) → nothing

Names should describe the relation between the nickname and Account.Nickname.
Are they different btw? If not, why not just remove SetNickname() and use Account.Nickname instead?

details:
reference to "GetContactAttributes" should be links.

(In reply to comment #12)
> Names should describe the relation between the nickname and Account.Nickname.
> Are they different btw? If not, why not just remove SetNickname() and use
> Account.Nickname instead?

Oh right, the AM needs a way to tell the CM about the nickname. Anyway, we should document the API that clients should use.

(In reply to comment #12)
> "The GetContactListAttributes method will wait until the contact list is
> available before returning. Using a method also allows extra attributes to be
> retrieved at the same time."
> In practice that means that any client (include tp-glib's client side API) will
> have to pass an "infinite" timeout when calling this function as fetching the
> roster can take a while with slow servers.

Yes, a fair point; the spec should document this.

> How UI is supposed to display the DefaultGroup? It won't be localized so we
> can't really display it as it.

It's to support Haze/libpurple, in which every contact must be in at least one group; contacts not in any group get put in _("Buddies") (so in fact it *is* localized, via libpurple). UIs could conceivably treat contacts whose groups include the DefaultGroup as if they weren't in that group? Perhaps?

At the moment DefaultGroup can be written; I need to check with wjt/Maiku whether this makes sense.

Perhaps I should just discard it, and Haze contacts will magically join _("Buddies") if necessary.

> Can we have Names without ContactList?

Yes. With hindsight, I should have used a separate branch for it - the fact that it's on this branch is a remnant of having local-alias as an attribute on the ContactList interface.

> Does Name replae Aliasing?

Yes, eventually.

> SetLocalAlias (s: Alias) → nothing
> Should be: SetLocalAlias (u: handle, s: Alias) → nothing

Oops. Yes :-)

> reference to "GetContactAttributes" should be links.

I'll fix that.

(In reply to comment #14)
> (In reply to comment #12)
> > How UI is supposed to display the DefaultGroup? It won't be localized so we
> > can't really display it as it.
> 
> It's to support Haze/libpurple, in which every contact must be in at least one
> group; contacts not in any group get put in _("Buddies") (so in fact it *is*
> localized, via libpurple). UIs could conceivably treat contacts whose groups
> include the DefaultGroup as if they weren't in that group? Perhaps?
> 
> At the moment DefaultGroup can be written; I need to check with wjt/Maiku
> whether this makes sense.

Each prpl has its own name for this group. There's no API in libpurple for the default group; prpls just shove ungrouped contacts into some group of their choosing. The UI cannot change the name of this fallback group.

I'd drop it. It's a bug in libpurple.

I've removed the Names part and deferred that to Bug #14540, leaving only ContactList and ContactGroups on this branch.

http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/contact-list

http://people.freedesktop.org/~smcv/telepathy-spec-contact_list/spec/

I think this could be merged as a DRAFT.

The only controversial point is the Hold parameter in GetContactListAttributes(). We should decide once what we want to do with that.

Drafts are in 0.19.6.

Errata since 0.19.6, based on a partial implementation in telepathy-glib (signal emissions and unfinished implementation of methods):

GetContactListAttributes should be called RequestContactListAttributes, or just RequestContactList, because it takes time (potentially quite a while on XMPP).

The actor for contact list changes is not given by this API. Do we care? The non-obvious edges are:

* subscription request rejected by remote user or cancelled by us
* publication request rejected by us or cancelled by remote user

It's unclear to what extent those would be state-recoverable anyway, though.

It's slightly unspecified what the signals are when you rename a group, say from Old to New with members Alice and Bob. The possibilities are:

1) GroupRenamed("Old", "New"); GroupsChanged([alice, bob], ["New"], ["Old"])
2) GroupRenamed("Old", "New"); GroupsCreated(["New"]); GroupsRemoved(["Old"]); GroupsChanged([alice, bob], ["New"], ["Old"])

There's no merged draft for ContactBlocking.

(In reply to comment #19)
> * subscription request rejected by remote user or cancelled by us
> * publication request rejected by us or cancelled by remote user
> 
> It's unclear to what extent those would be state-recoverable anyway, though.

Is there a point in recovering the actor? It's interesting when it happens, but after the pending contact is not there, who cares why did it go away while we were not looking?

(In reply to comment #19)
> Errata since 0.19.6

Further errata:

* The interaction of the D-Bus properties with receiving the initial contact list should be specified. Before that point, I propose that ContactGroups.Groups should always be empty, but all the other ContactList and ContactGroups properties are meaningful.

* The interaction of the contact attributes with receiving the initial contact list should be specified. I propose that they're omitted from the dict before the initial contact list is received, so UIs can detect this if they really care; in practice bindings will interpret this as Presence_State_No, which is also fine.

* I wonder whether we should have a boolean attribute for "is on the contact list", which would be false for everyone until the initial contact list is received?

(In reply to comment #19)
> It's slightly unspecified what the signals are when you rename a group, say
> from Old to New with members Alice and Bob. The possibilities are:
> 
> 1) GroupRenamed("Old", "New"); GroupsChanged([alice, bob], ["New"], ["Old"])
> 2) GroupRenamed("Old", "New"); GroupsCreated(["New"]); GroupsRemoved(["Old"]);
> GroupsChanged([alice, bob], ["New"], ["Old"])

I implemented (2).

(In reply to comment #21)
> * The interaction of the D-Bus properties with receiving the initial contact
> list should be specified. Before that point, I propose that
> ContactGroups.Groups should always be empty, but all the other ContactList and
> ContactGroups properties are meaningful.

I implemented this.

> * The interaction of the contact attributes with receiving the initial contact
> list should be specified. I propose that they're omitted from the dict before
> the initial contact list is received, so UIs can detect this if they really
> care; in practice bindings will interpret this as Presence_State_No, which is
> also fine.

I implemented this.

(In reply to comment #19)
> 2) GroupRenamed("Old", "New"); GroupsCreated(["New"]); GroupsRemoved(["Old"]);
> GroupsChanged([alice, bob], ["New"], ["Old"])

Would it be saner to emit GroupsRemoved(["Old"]) after all members have left the group?

(In reply to comment #21)
> * I wonder whether we should have a boolean attribute for "is on the contact
> list", which would be false for everyone until the initial contact list is
> received?

As currently spec'd, we can't solve Bug #19903 in the way Olivier requests, unless the Maemo UI (or its descendant) specifically excludes contacts we aren't subscribed to. Further discussion on Bug #19903.

(In reply to comment #23)
> (In reply to comment #19)
> > 2) GroupRenamed("Old", "New"); GroupsCreated(["New"]); GroupsRemoved(["Old"]);
> > GroupsChanged([alice, bob], ["New"], ["Old"])
> 
> Would it be saner to emit GroupsRemoved(["Old"]) after all members have left
> the group?

IMO, no; this way, you can distinguish between "remove all members then delete the group" and "remove all members because the group was deleted" (and similar for renaming).

(In reply to comment #19)
> The actor for contact list changes is not given by this API. Do we care? The
> non-obvious edges are:
> 
> * subscription request rejected by remote user or cancelled by us
> * publication request rejected by us or cancelled by remote user

These could be achieved by changing the tristate into a 4-state enum (although we'd have to rename it from Presence_State to something else, to avoid conflicts with the existing type of the same name in telepathy-qt4):

* Yes
* Ask (or Pending)
* Rejected
* No (also used for cancelled requests)

If we're not recycling telepathy-qt4's tristate, we might as well add an Unknown member with numeric value 0, too.

> It's unclear to what extent those would be state-recoverable anyway, though.

Rejected would probably morph into No across a disconnect/reconnect on most protocols.

> There's no merged draft for ContactBlocking.

There's now an unmerged draft, Bug #28423.

Review requested for smcv/contact-list-errata.

HTML: http://people.freedesktop.org/~smcv/telepathy-spec-contact_list_rejection/spec/
gitweb: http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/contact-list-errata

(In reply to comment #19)
> GetContactListAttributes should be called RequestContactListAttributes, or just
> RequestContactList, because it takes time (potentially quite a while on XMPP).

Fixed in smcv/contact-list-errata.

(In reply to comment #22)
> (In reply to comment #19)
> > It's slightly unspecified what the signals are when you rename a group, say
> > from Old to New with members Alice and Bob. The possibilities are:
> > 
> > 1) GroupRenamed("Old", "New"); GroupsChanged([alice, bob], ["New"], ["Old"])
> > 2) GroupRenamed("Old", "New"); GroupsCreated(["New"]); GroupsRemoved(["Old"]);
> > GroupsChanged([alice, bob], ["New"], ["Old"])
> 
> I implemented (2).

Specified in smcv/contact-list-errata.

> (In reply to comment #21)
> > * The interaction of the D-Bus properties with receiving the initial contact
> > list should be specified. Before that point, I propose that
> > ContactGroups.Groups should always be empty, but all the other ContactList and
> > ContactGroups properties are meaningful.
> 
> I implemented this.

Specified in smcv/contact-list-errata.

> > * The interaction of the contact attributes with receiving the initial contact
> > list should be specified. I propose that they're omitted from the dict before
> > the initial contact list is received, so UIs can detect this if they really
> > care; in practice bindings will interpret this as Presence_State_No, which is
> > also fine.
> 
> I implemented this.

Specified in smcv/contact-list-errata.

(In reply to comment #26)
> (In reply to comment #19)
> > The actor for contact list changes is not given by this API. Do we care? The
> > non-obvious edges are:
> > 
> > * subscription request rejected by remote user or cancelled by us
> > * publication request rejected by us or cancelled by remote user
> 
> These could be achieved by changing the tristate into a 4-state enum (although
> we'd have to rename it from Presence_State to something else, to avoid
> conflicts with the existing type of the same name in telepathy-qt4):
> 
> * Yes
> * Ask (or Pending)
> * Rejected
> * No (also used for cancelled requests)
> 
> If we're not recycling telepathy-qt4's tristate, we might as well add an
> Unknown member with numeric value 0, too.

Implemented in:

HTML: http://people.freedesktop.org/~smcv/telepathy-spec-contact_list_rejection/spec/
gitweb: http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/contact-list-rejection

(In reply to comment #27)
> Review requested for smcv/contact-list-errata.

Sorry, that should have been:

HTML: http://people.freedesktop.org/~smcv/telepathy-spec-contact_list_errata/spec/
gitweb: http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/contact-list-errata

errata looks good to me.

I've merged contact-list-errata, leaving contact-list-rejection as the outstanding branch here (Comment #28).

Re GetContactListAttributes: I find it sad that special timeout had to be specified for this method; is it a generally accepted practice across Telepathy? This will require special work for bindings and clients, unless you provide a way to annotate the longer timeout and generate code for it. Also, a good value for such a timeout would be the maximum of practical sync time limits on an unknown set of protocols. This is unmanageable.

Maybe a property/signal pair could be added so that clients could wait on the signal, and know that GCLA will return empty if called while contact list synchronization is pending?

(In reply to comment #32)
> Maybe a property/signal pair could be added

Also useful to expose the state when contact list retrieval fails.

(In reply to comment #32)
> This will require special work for bindings

Our generated bindings in telepathy-glib have the timeout as a parameter, so they're OK already. However, telepathy-qt4 doesn't; I consider this to be a telepathy-qt4 bug (I've filed Bug #28797) whose fix is blocked by a QtDBus bug (<http://bugreports.qt.nokia.com/browse/QTBUG-11775>).

> Also, a
> good value for such a timeout would be the maximum of practical sync time
> limits on an unknown set of protocols. This is unmanageable.

I think the timeout is a UI decision. For how long is it acceptable for your UI to display a "please wait" spinner before giving up in disgust - a minute, an hour, 24 hours? However long that is, that's the timeout you should have.

> Maybe a property/signal pair could be added so that clients could wait on the
> signal, and know that GCLA will return empty if called while contact list
> synchronization is pending?

I think a boolean property with change notification could be a good idea too, though. The change notification could be implicit (any ContactsChanged, GroupsChanged, etc. signal implicitly indicates the transition to ContactListRetrieved=TRUE), or explicit.

(In reply to comment #33)
> (In reply to comment #32)
> > Maybe a property/signal pair could be added
> 
> Also useful to expose the state when contact list retrieval fails.

Is your idea that this is state-recovery, something like this?

property ContactListFailure: s
    The D-Bus error name with which retrieval of the contact list failed,
    or the empty string if retrieval succeeded or is still in progress

I find it more natural for the data model to be that if you care about the contact list, you call RequestContactList. If it takes a while, you can display a "please wait" indication or something, knowing that it's still in progress. When you eventually get your reply, it's either an error or success; if you call RequestContactList when the attempt to download the contact list has already finished, you get the same cached error or success.

(Having said that, TpBaseContactList doesn't yet allow subclasses to signal that retrieval of the contact list failed, except by disconnecting the Connection; I'll put that on my to-do list for telepathy-glib, but it's not directly relevant to the spec.)

Another timeout-related issue is what happens if you want to alter the contact list (RequestSubscription, SetGroupMembers, etc.), which I expect will require that you've already retrieved the contact list in most protocols.

Options:

1) Until the contact list has been retrieved, all alterations fail.

This is essentially equivalent to ContactList channels, which you must request (with CreateChannel or EnsureChannel, and the potential for a timeout!) before you can call their methods.

wjt and I think this model is harder than it needs to be; in practice it means that every non-buggy client that wants to manipulate the contact list will need to invent a queue of "things I want to do when the contact list is ready".

2) The contact list is guaranteed to be retrieved before CONNECTED. Alterations fail with Disconnected until the status changes to CONNECTED. After CONNECTED, alterations immediately succeed.

This avoids the timeout issue completely (connecting is already more than a single method call), but it means that CONNECTED is delayed even further, which I don't think is desirable; a lot of things only work after CONNECTED, and on some protocols you have to do quite a lot of work to get the contact list (as I understand it, on MSN it's kept on a different server).

3) Until the contact list has been retrieved, alterations are queued. When the contact list has been retrieved, we start acting on alterations.

wjt and I think this is the way forward, and it's what I've implemented in Gabble. It does have the disadvantage that if you make an alteration before the contact list has come in, you should set a larger-than-default timeout for it.

If it takes longer than the timeout, the UI will indicate a failure, but the side-effect will eventually happen anyway - indistinguishable from the same change being made by a different XMPP resource or something - so I don't think that's necessarily such a big problem. If you don't want to have non-default timeouts on calls other than RequestContactList, you can also queue calls in the client, as with (1).

4) Alterations are possible right from the start (even before CONNECTED). When the contact list has been retrieved, we start acting on alterations.

In practice, this is the same as (3), because most alterations require you to have some contact handles, and those don't work until you're CONNECTED. I don't think it's useful to be able to delete and empty groups in advance, and those are the only alterations I can think of that don't need a contact handle :-)

(In reply to comment #32)
> Re GetContactListAttributes: I find it sad that special timeout had to be
> specified for this method

I now realise that this is essentially equivalent to calling EnsureChannel for the subscribe, publish and stored lists, which is best-practice in the current D-Bus API. As far as I know, nobody uses a longer-than-default timeout for those EnsureChannel calls, and we generally cope; it'll only take longer than 25 seconds on pathological servers.

In both the old and new API, if the initial method call times out, you can recover by trying again later if you want, or even recover by watching for signals (NewChannels in the old API, ContactsChanged in the new) and catching up on what you missed.

(In reply to comment #34)
> I think the timeout is a UI decision. For how long is it acceptable for your UI
> to display a "please wait" spinner before giving up in disgust - a minute, an
> hour, 24 hours? However long that is, that's the timeout you should have.

This is orthogonal to the way of signalling. But I realize that as a non-modifying method with natural semantics of failure (timed out => no contacts available yet), GCLA timing out is not such a big problem.

> I think a boolean property with change notification could be a good idea too,
> though. The change notification could be implicit (any ContactsChanged,
> GroupsChanged, etc. signal implicitly indicates the transition to
> ContactListRetrieved=TRUE), or explicit.
> 
> (In reply to comment #33)
> > (In reply to comment #32)
> > > Maybe a property/signal pair could be added
> > 
> > Also useful to expose the state when contact list retrieval fails.
> 
> Is your idea that this is state-recovery, something like this?
> 
> property ContactListFailure: s
>     The D-Bus error name with which retrieval of the contact list failed,
>     or the empty string if retrieval succeeded or is still in progress

I was thinking more about an enum ContactListState = (Initial|Retrieving|Retrieved|Failed)

The error could go to the method return, if you want to keep GCLA as a potentially slow roundtrip.

Transcript of some discussion on IRC:

14-07-2010 14:39:59 < sjoerd: smcv: why does the REquestSubscription spec entry talk about aliases
14-07-2010 14:40:44 > smcv: sjoerd: because they used to be on that interface, and because in any protocol with user-set aliases, you should probably set one
14-07-2010 14:41:18 > smcv: sjoerd: (see <tp:rationale>)
14-07-2010 14:41:36 < sjoerd: that's great and all, but i don't think it belongs there and it's just confusing
14-07-2010 14:42:25 > smcv: the other reason is that something (either Aliasing/Names or RequestSubscription) ought to recommend an order in which to set them
14-07-2010 14:43:16 < sjoerd: That's should probably be in the description then
14-07-2010 14:43:31 < sjoerd: It's more of a guideline then documentation of a specific method
14-07-2010 14:43:41 > smcv: and indeed to try to avoid the failure mode that Empathy has / previously had, where it sets the user's alias to their identifier, then their nickname turns up, but you'll never see it because the local alias takes precedence
14-07-2010 14:44:19 > smcv: (Gabble does that too, as a protocol workaround, but IMO it's worse that Empathy does, because that also hurts Haze, particularly for protocols like ICQ where the identifier is opaque)
14-07-2010 14:44:33 < sjoerd: That's not the specs problem
14-07-2010 14:45:39 > smcv: I agree the description is too short, it should summarize what you do
14-07-2010 14:50:39 < sjoerd: smcv: hmm, does (SubscriptionPersist = True, CanChangeSubscriptions = False) ever occur?
14-07-2010 14:50:49 > smcv: sjoerd: facebook
14-07-2010 14:51:13 < sjoerd: smcv: not on a protocol level though, gabble won't know that
14-07-2010 14:51:28 > smcv: sjoerd: it'd be better UI if it did
14-07-2010 14:51:48 > smcv: sjoerd: likewise editing out some interfaces/actions/flags if it notices it's on facebook chat
14-07-2010 14:51:50 < sjoerd: but i guess you could imagine a faceobok like situation with a none-xmpp chat protocol
14-07-2010 14:51:54 > smcv: yeah
14-07-2010 14:52:15 < wjt: is this something profiles should cover?
14-07-2010 14:52:21 > smcv: if I get round to writing telepathy-1970 it'd be like that
14-07-2010 14:52:29 > smcv: (the contact list would be /etc/passwd :-P )
14-07-2010 14:52:35 < sjoerd: heh
14-07-2010 14:52:39 < sjoerd: wjt: probably
14-07-2010 14:53:00 < sjoerd: smcv: can we call them ContactListPersis and CanChangeContactList btw ?
14-07-2010 14:53:30 < * sjoerd finds things like: CanChangeSubscriptions: presence subscription and publication can be changed confusing
14-07-2010 14:53:58 > smcv: wjt: the advantage of getting it from the CM is that if we can talk the facebook people into adding some stream anticapabilities, and their server becomes more capable in future, we don't have capabilities arbitrarily swithced off by an out of date profile
14-07-2010 14:54:13 < sjoerd: well
14-07-2010 14:54:21 < sjoerd: profiles are essentially an overlay aren't they
14-07-2010 14:54:29 > smcv: yeah
14-07-2010 14:54:44 > smcv: "I happen to know that this feature doesn't work", in this case
14-07-2010 14:54:49 < sjoerd: So we should have them in the CM as well, but in the facebook case the profile can indicate extra restrictions on top of the protocol, which would be current
14-07-2010 14:54:57 < sjoerd: *correct
14-07-2010 14:55:39 < wjt: so in this case it would be "override this connection property to have this hard-coded value"
14-07-2010 14:55:43 > smcv: by "have them in the CM" do you mean ship a .profile with gabble, or what?
14-07-2010 14:56:27 > smcv: sjoerd: vague rationale for it being Subscriptions: subscription and publication are different ways to look at the same thing really
14-07-2010 14:56:42 > smcv: but yeah those could change to be ContactListFoo
14-07-2010 14:57:40 < sjoerd: Yeah, i know it's all different views, which is what it makes it confusing :)
14-07-2010 14:57:56 > smcv: also, CanChangeSubscriptions is really what we mean
14-07-2010 14:58:13 > smcv: well, I suppose "can change subscriptions, and publication negatively"
14-07-2010 14:58:16 > smcv: or some such
14-07-2010 14:58:29 > smcv: aagh
14-07-2010 14:58:51 > smcv: I don't want to get into the group flags situation again, let's call it CanChangeContactList :-)
14-07-2010 14:58:59 < sjoerd: hehe
14-07-2010 14:59:38 > smcv: I think it's reasonable to say that if you can request subscriptions, but you can't (say) revoke publication, then your CM or your server or both suck
14-07-2010 15:01:02 < sjoerd: the REquestContactList thing mikhailz raises is interesting
14-07-2010 15:01:50 < sjoerd: makes me wonder if we should have a ContactListRetrieved with a change signal
14-07-2010 15:02:17 < sjoerd: time to make more tea and ponder on the failure cases :p
14-07-2010 15:02:48 > smcv: I think the quad-state that mikhailz suggests might be a good model
14-07-2010 15:03:10 > smcv: if you want to know where we've got to, just look at it
14-07-2010 15:03:22 > smcv: if you want to wait for it to resolve, call the method
14-07-2010 15:03:32 > smcv: if you want to know why it failed, call the method
14-07-2010 15:04:02 > smcv: and you can either try the method again on timeout, or set a long timeout, or whatever you like
14-07-2010 15:12:25 < sjoerd: everyone will get that error path wrong though
14-07-2010 15:16:06 > smcv: we also can't usefully make contact list edits without waiting for the initial contact list, so...
14-07-2010 15:17:07 > smcv: I should probably write client API on TpConnection to make sure it's possible to do so
14-07-2010 15:20:28 < sjoerd: yeah, i think i'd argue for option 1 in comment 35, but you and wjt apparently think that makes things harder
14-07-2010 15:26:13 < wjt: sjoerd: yeah, i guess i stand by that position| ~@~T it adds one more async step in any client using thiss
 interface
14-07-2010 15:27:52 < sjoerd: wjt: then again every client will get it wrong when retrieiving the contact lists takes a long time
14-07-2010 15:28:20 < sjoerd: and it means for editting every CM probably needs a queue
14-07-2010 15:30:33 < sjoerd: wjt: the extra async step you need, is the same you also need to handle the slow contact list retrieval error case though. it just moves it forward
14-07-2010 15:31:12 < wjt: mmm
14-07-2010 15:31:46 > smcv: I wonder whether writing (or at least documenting) the client API would be illustrative
14-07-2010 15:32:35 < sjoerd: the tp-glib API should just be turning on the feature which causes it to retrieve the contact list
14-07-2010 15:33:05 < sjoerd: it should hide all the silly timeout stuff for clients
14-07-2010 15:33:10 > smcv: right
14-07-2010 15:33:19 > smcv: but when should contact list manipulations work?
14-07-2010 15:33:28 > smcv: a) after the feature has been prepared
14-07-2010 15:33:35 > smcv: b) after you connected
14-07-2010 15:33:47 > smcv: c) your alternative here
14-07-2010 15:34:26 > smcv: bearing in mind that a contact list manipulation (RequestSubscription, say) is async anyway - it'd make most sense to have it encapsulate any other async round-tripping that needs to happen first, I think?
14-07-2010 15:37:02 < sjoerd: When can a CM return from RequestSubscription ?
14-07-2010 15:37:31 > smcv: preferably, after the server has acked the subscription request in some way
14-07-2010 15:37:42 > smcv: in practice Gabble can't do that, because a subscription request is presence :-/
14-07-2010 15:37:51 > smcv: hmm, actually
14-07-2010 15:37:57 < sjoerd: so network roundtrip, so timeout hell again
14-07-2010 15:38:13 > smcv: yes, but methods being a network round-trip is nothing new
14-07-2010 15:38:35 > smcv: the only reason this interface is unusual is that some servers have been observed to take longer than is sane to fetch the contact list
14-07-2010 15:38:38 > smcv: (apparently)
14-07-2010 15:39:22 < sjoerd: Maybe you're 3G is better then mine, but it's not uncommon to be on a train and a login taking forever
14-07-2010 15:39:31 > smcv: any "write", like setting your nickname or whatever, ought to wait for the network round-trip before returning, otherwise we can't indicate errors
14-07-2010 15:40:02 > smcv: but yes the difference is that the contact list is bigger, so it's a lot more likely that the initial contact list pull will take longer
14-07-2010 15:40:41 > smcv: and because we can't edit the contact list until we know its initial state, that potentially carries over into the other methods
14-07-2010 15:40:47 < sjoerd: yes
14-07-2010 15:43:13 > smcv: OOI, do you get a contact list successfully with current Telepathy? the long-round-trip is isomorphic to what you have to do at the moment, which is EnsureChannel() for a contact list channel
14-07-2010 15:44:59 < sjoerd: we set the timeout to MAXINT
14-07-2010 15:45:04 < sjoerd: well empathy does that
14-07-2010 15:45:08 < sjoerd: no simple client ever does that
14-07-2010 15:45:19 > smcv: right
14-07-2010 15:45:31 > smcv: (and I discovered while investigating that Qt can't :'-( )

(In reply to comment #37)
> I was thinking more about an enum ContactListState =
> (Initial|Retrieving|Retrieved|Failed)
> 
> The error could go to the method return, if you want to keep GCLA as a
> potentially slow roundtrip.

I've added something pretty similar in branch smcv/no-waiting. It's a quad-state, None|Waiting|Success|Failure.

GCLA is now instant-return again; in the Failure state, it raises an error, and in the None and Waiting states, it raises a new error, o.fd.T.E.NotYet, analogous to EWOULDBLOCK. Alternatively, we could make it block if people prefer.

We can solve the "client-side queue" issue that wjt and I complained about by making tp_connection_contact_list_request_subscription_async() (etc.) operate as follows:

* async-wait for the contact list to be retrieved or failed, or invalidation
* if invalidated, fail
* call GCLA
* if invalidated, or GCLA failed, fail with the same error
* async-call RequestSubscription
* fail or succeed as appropriate

and also doing the equivalent in telepathy-qt4.

> 14-07-2010 14:53:00 < sjoerd: smcv: can we call them ContactListPersis and
> CanChangeContactList btw ?

Done in the same branch.

----------------------

Still to do:

As currently implemented, Subscription_State_Rejected only applies to our subscription to others, not our publication to others - if we reject someone's request, we still represent that as No, to preserve the fact that all contacts not on the list have publish = subscribe = No. What do people think about this?

Concrete example:

* Alice requests presence from me
  ContactsChanged: Changed contains contact=Alice, sub=No, pub=Ask
* I reject the request with RemoveContacts()
  - ContactsChanged: Removed=[Alice]
  - or: ContactsChanged: Changed contains contact=Alice, sub=No, pub=Rejected;
        a second ContactsChanged signal has Removed=[Alice]
  - or: ContactsChanged: Changed contains contact=Alice, sub=No, pub=Rejected,
                         and Removed=[Alice]

> 14-07-2010 15:37:02 < sjoerd: When can a CM return from RequestSubscription ?
> 14-07-2010 15:37:31 > smcv: preferably, after the server has acked the
> subscription request in some way
> 14-07-2010 15:37:42 > smcv: in practice Gabble can't do that, because a
> subscription request is presence :-/

I still need to relax the MUSTs to SHOULDs so XMPP CMs can be spec-compliant.

> 14-07-2010 14:45:39 > smcv: I agree the description is too short, it should
> summarize what you do

I'd prefer to wait for the API to settle first :-)

An additional concern: we currently assume that the CM will fetch the contact list. There is a somewhat questionable use-case for not getting the contact list. Quoting RFC 3921:

"""Upon connecting to the server and becoming an active resource, a client SHOULD request the roster before sending initial presence (however, because receiving the roster may not be desirable for all resources, e.g., a connection with limited bandwidth, the client's request for the roster is OPTIONAL). If an available resource does not request the roster during a session, the server MUST NOT send it presence subscriptions and associated roster updates."""

Sjoerd thinks we don't need to care about this, and can just get the contact list unconditionally, because in practice nearly every client will want it. To consume less bandwidth, we should implement XEP-0237 Roster Versioning (Bug #26489 - getting the roster as a diff since the last known version) rather than trying to avoid getting the roster (which in practice won't happen).

However, if we do (or might) want it to be conditional, it should be opt-in now, because turning opt-in into guaranteed is compatible, but turning guaranteed into opt-out is not. The opt-in could be via client interests (Bug #27835) or something else. So, if anyone disagrees with Sjoerd, speak now!

(In reply to comment #39)
> I've added something pretty similar in branch smcv/no-waiting. It's a
> quad-state, None|Waiting|Success|Failure.
> 
> GCLA is now instant-return again; in the Failure state, it raises an error, and
> in the None and Waiting states, it raises a new error, o.fd.T.E.NotYet,
> analogous to EWOULDBLOCK.

Sjoerd seems to approve.

If we keep the contact list download as a guarantee, None and Waiting are synonyms, so we can just use Waiting|Success|Failure.

> We can solve the "client-side queue" issue that wjt and I complained about by
> making tp_connection_contact_list_request_subscription_async() (etc.) operate
> as follows:
> 
> * async-wait for the contact list to be retrieved or failed, or invalidation
> * if invalidated, fail
> * call GCLA
> * if invalidated, or GCLA failed, fail with the same error
> * async-call RequestSubscription
> * fail or succeed as appropriate

Sjoerd suggests allowing this method automatically prepare TP_CONNECTION_FEATURE_CONTACT_LIST, but require that TP_CONNECTION_FEATURE_CONNECTED is already prepared. I think that sounds reasonable.

> As currently implemented, Subscription_State_Rejected only applies to our
> subscription to others, not our publication to others - if we reject someone's
> request, we still represent that as No, to preserve the fact that all contacts
> not on the list have publish = subscribe = No.

After discussion with Sjoerd, I suggest we loosen ContactsChanged to say that the same contact can be in both Changed and Removed, Changed applies first, contacts not on the contact list can have either publish=No or publish=Rejected, and putting them in Removed implies a change to No *unless it's already No or Rejected*. Then we can have:

* Alice requests presence from me
  ContactsChanged: Changed={Alice: (sub=No, pub=Ask)}
* if I reject the request with RemoveContacts() or Unpublish():
  ContactsChanged: Changed={Alice: (sub=No, pub=Rejected)}, Removed=[Alice]
* or if Alice cancels it:
  ContactsChanged: Changed={}, Removed=[Alice]
  or if we want to be more explicit, this is equivalent:
  ContactsChanged: Changed={Alice: (sub=No, pub=No)}, Removed=[Alice]

(In reply to comment #40)
> > As currently implemented, Subscription_State_Rejected only applies to our
> > subscription to others, not our publication to others - if we reject someone's
> > request, we still represent that as No, to preserve the fact that all contacts
> > not on the list have publish = subscribe = No.
> 
> After discussion with Sjoerd, I suggest we loosen ContactsChanged to say that
> the same contact can be in both Changed and Removed, Changed applies first,
> contacts not on the contact list can have either publish=No or
> publish=Rejected, and putting them in Removed implies a change to No *unless
> it's already No or Rejected*.

This turns out to be harder to implement than you'd think, because tp_base_contact_list_contacts_changed enforces the condition that state be recoverable (the subclass must update its internal model, then pass a list of contacts to tp_base_contact_list_contacts_changed, and that method asks the subclass for the changed contacts' states.

I could change that API, but I think the need to change it is a symptom of breaking state-recoverability of things that ought to be state-recoverable, which we don't really want anyway.

One possibility is to replace Rejected with Remote_Removed or something, with the following semantics:

* For subscribe, it's the same as the current Rejected: subscribe changes from Ask to Remote_Removed when the remote user denies our request, and you can "acknowledge" by calling Unsubscribe() or RemoveContacts(), at which point subscribe changes to No (and the contact is removed if appropriate). If subscribe changes from Ask to No, that means either we cancelled it, or we don't know why it changed.

* For publish, if publish=Ask, Unpublish() or RemoveContacts() rejects a publish request (publish changes from Ask to No). If publish changes from Ask to No, that means either we (or another of our resources) cancelled it, or we don't know why it changed. If publish changes from Ask to Remote_Removed, that means the remote contact cancelled their request: you can "acknowledge" by calling Unpublish() or RemoveContacts(), at whick point Remote_Removed changes to No and the contact is (probably) removed.

This is fully state-recoverable, although more subtle.

Another possibility is to have an Actor or Locally_Initiated or something in ContactsChanged.

(In reply to comment #41)
> Another possibility is to have an Actor or Locally_Initiated or something in
> ContactsChanged.

I pressed [Commit] too soon... what I meant to say is "this is not at all state-recoverable, but if we're discarding state-recoverability, then this would be the way to do it".

Some other assorted thoughts based on the telepathy-glib implementation:

Splitting apart ContactsChanged and ContactsRemoved might make the API more understandable. In all of telepathy-glib and Gabble, there's only one call to tp_base_contact_list_contacts_changed where both parameters are non-NULL, and in practice I don't think that will ever happen - the initial roster download in Gabble we won't get removals, and in subsequent roster pushes, we only get one item at a time.

TpBaseContactList has a virtual method that creates groups (necessary to support CreateChannel/EnsureChannel for GROUP ContactList channels), but it can't be invoked from the new API. Should it be added? My inclination would be no: you can create a group as a side-effect, via AddToGroup("empty group", []) or SetGroupMembers("empty group", []), if you really want to :-)

(In reply to comment #42)
> Splitting apart ContactsChanged and ContactsRemoved might make the API more
> understandable. In all of telepathy-glib and Gabble, there's only one call to
> tp_base_contact_list_contacts_changed where both parameters are non-NULL, and
> in practice I don't think that will ever happen

It seems I spoke too soon. When I made the contactlist example a bit more realistic, I realised that if you have this behaviour (which is what it now emulates):

- a persistent protocol-level roster, like in XMPP, containing (say) [Alice, Bob]
- a transient list of people who want to add you, containing [Bob, Chris]
- GetContactListAttributes returns the union of the two lists
- protocol-level operations can act on multiple contacts (unlike XMPP)

then rejecting a subscription request from someone not on your protocol-level roster (Chris, but not Bob, here) causes them to be "removed" (they were never really stored in the first place). This means that Unpublish([Alice, Bob, Chris]) would naturally result in ContactsChanged(Changed={Alice: (...), Bob: (...)}, Removed=[Chris]). The example now behaves like this.

Of course, we could still split it into two signals if simplicity of each signal is considered more important than simplicity of the interface (i.e. fewer signals).

(I'd appreciate review on the spec changes for draft 3 - I think it's settled down again, I have an implementation in Gabble, and David has reviewed the corresponding telepathy-glib changes, so it'd be good to have the telepathy-glib branch reflect the current draft in spec releases.)

In Bug #28200, David wonders whether some of the "plural" change methods should instead only act on a single contact. If they're implemented as singular in telepathy-glib, I think they should also be singular on D-Bus.

How does SIP/SIMPLE (where the contact list isn't stored by the server) look in this API? Options are:

* UIs notice that SubscriptionsPersist is False but CanChangeSubscriptions is True, and respond by storing the contact list locally (in libfolks?), and dropping it into both AuthorizePublication and RequestSubscription every time you connect.

* High-quality connection managers for SIMPLE are required to store your contact list somewhere (~/.local/share/telepathy-sofiasip?) and have it take effect every time you connect; SubscriptionsPersist is either removed, or kept but marked as "for information only".

The spec currently says the former, but I think the latter may make more sense.

It's probably worth keeping SubscriptionsPersist for information - for instance, the Maemo 5 UI currently says "Your contacts will be copied from the server" if the protocol has ContactList channels, but if it was reimplemented in terms of this API, it would only say that on protocols where SubscriptionsPersist is True.

(In reply to comment #44)
> How does SIP/SIMPLE (where the contact list isn't stored by the server) look in
> this API? Options are:
> 
> * UIs notice that SubscriptionsPersist is False but CanChangeSubscriptions is
> True, and respond by storing the contact list locally (in libfolks?), and
> dropping it into both AuthorizePublication and RequestSubscription every time
> you connect.

Fair enough.

> * High-quality connection managers for SIMPLE are required to store your
> contact list somewhere (~/.local/share/telepathy-sofiasip?) and have it take
> effect every time you connect; SubscriptionsPersist is either removed, or kept
> but marked as "for information only".

I don't think basic SIMPLE presence deserves that much care. A "high-quality" SIP deployment should use XCAP for server-side contact list management. Plain SIMPLE with peer-to-peer SUBSCRIBE requests is not too sane for network usage. In fact, I don't know of any significant user base using anything SIMPLE. Does Ekiga do it without XCAP?

Latest draft (3) merged for 0.19.12 per Sjoerd's review.

I owe Sjoerd some wording changes to make contact metadata storage types clearer: if the CM has returned from the setter, it's expected to return the new metadata from the getter from that point on, even if it's actually just caching it for later use. The only exception is the "can't do this" case, where the setter should just fail.

Other than that, Sjoerd said he'd be happy to undraft as-is.

Two slightly open issues remaining:

1. Guaranteeing contact list download or not

> If we keep the contact list download as a guarantee, None and Waiting are
> synonyms, so we can just use Waiting|Success|Failure.

Do this or not? I'd be inclined to say leave it as it is.

2. What clients should do if the contact list is transient

See Comment #45 and the earlier comment quoted there. This is pretty minor tbh, we can address this if/when telepathy-sofiasip actually implements a contact list.

(In reply to comment #46)
> I owe Sjoerd some wording changes to make contact metadata storage types
> clearer

Now done, see referenced URL. HTML is here:
http://people.freedesktop.org/~smcv/telepathy-spec-contact_list/spec/Connection_Interface_Contact_List.html#Contact_Metadata_Storage_Type

I've rebased onto master and added a better docstring for Groups:

http://people.freedesktop.org/~smcv/telepathy-spec-contact_list/spec/Connection_Interface_Contact_Groups.html

Fixed in 0.21.0