Bug 30874

Summary: GetContactAttributesByID: GetContactAttributes but from identifiers instead of handles
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-specAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium CC: xclaesse
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/xclaesse/telepathy-spec.git/log/?h=by-id
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 27687    

Description Simon McVittie 2010-10-14 08:52:20 UTC
Subject says it all.

There is a subtlety, which is how to report invalid identifiers. TpContact's API for this is the best I could come up with, but isn't ideal.
Comment 1 Xavier Claessens 2012-03-30 01:00:39 UTC
Something like this?

GetContactAttributesByID (as: indentifiers, as: interfaces) -> a{sa{sv}}: attributes, as: failed
Comment 2 Xavier Claessens 2012-05-08 02:49:17 UTC
And tp-glib branch implementing the new spec in TpBaseConnection and TpContactsMixin:

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contacts
Comment 3 Xavier Claessens 2012-05-08 02:50:59 UTC
There is no test yet, so not yet ready to be merged. It will come once I implemented tp_connection_get_contact_by_id_async (bug #27687) that using this.
Comment 4 Simon McVittie 2012-05-08 04:15:06 UTC
The API in your spec branch, returning an a{ua{sv}}, doesn't work: if the protocol is case-insensitive and does not allow spaces, and I call

    GetContactAttributesByID(["Alice", "Bob", "Chris D"], [])

where Alice's handle is 1 and Bob's is 2, I will get back

    { 1: { ".../contact-id": "alice" }, 2: { ".../contact-id": "bob" } }

Without knowledge of how the protocol normalizes and validates names, I can't know that "Alice" and "alice" are the same contact, or even that "Chris D" was the identifier that wasn't syntactically valid! This is the same flaw that was present in drafts of Addressing1.

I think we should fix this by changing the signature to resemble the methods in Addressing, and while we're at it, we can shorten the name:

    GetContactsByID(as: Identifiers, as: Interfaces)
        -> a{su}: Requested, a{ua{sv}}: Attributes

so that

    GetContactAttributesByID(["Alice", "Bob", "Chris D"], [])

would return

    { "Alice": 1, "Bob": 2 },
    { 1: { ".../contact-id": "alice" }, 2: { ".../contact-id": "bob" } }
Comment 5 Xavier Claessens 2012-05-08 05:25:24 UTC
Actually decided to simplify this a lot by doing single-id only. In practice the only use case of getting a contact by id is when the user enters the ID in the UI to add contact. When multiple contacts are involved, the CM gives handle+id.

spec and tp-glib branches updated with this.
Comment 6 Simon McVittie 2012-05-08 09:58:50 UTC
+ <tp:docstring>
+ Return any number of contact attributes for the given identifier.
+ </tp:docstring>

<tp:rationale> for it being singular would be nice.

+ An identifier representing contacts.

a contact

+ <arg direction="out" type="a{sv}" name="Attributes"
+ tp:type="Single_Contact_Attributes_Map">
+ <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
+ <p>If contact attributes are not immediately known, the behaviour is

Should have a first line that says "All supported attributes of the contact on the given interfaces that can be returned without network round-trips." or something.

+ <p>Contact's attributes will always include at least the

The contact's ...

+ <tp:possible-errors>
+ <tp:error name="org.freedesktop.Telepathy.Error.Disconnected"/>
+ </tp:possible-errors>

Should also raise InvalidHandle if the identifier is syntactically invalid (that's what generally use to mean "invalid string corresponding to a handle").
Comment 7 Simon McVittie 2012-05-08 10:24:29 UTC
+ * @id: A strings representing the desired contact by its
+ * identifier in the IM protocol (XMPP JIDs, SIP URIs, MSN Passports,
+ * AOL screen-names etc.)

Grammar: A string representing ... (an XMPP JID, SIP URI, MSN Passport, AOL screen-name etc.)

+ * Create a #TpContact object and make asynchronous method call to

"any asynchronous method calls necessary to" or something

+TpContact *
+tp_connection_get_contact_by_id_finish (TpConnection *self,
+ GAsyncResult *result,
+ GError **error)

I wonder whether this should be _dup_ not _get_, since it returns a ref...

Since you've only added GetContactByID just now, I'd like a fallback path via RequestHandles and GetContactAttributes if it raises NotImplemented (or anything other than InvalidHandle, really), so that it can be guaranteed to work on any Contacts CM[1].

+ ContactFeatureFlags minimal_feature_flags = 0xFFFFFFFF;

Should be (ContactFeatureFlags) 0xFFFFFFFFU, really. Or just make it a guint and G_MAXUINT.

+ * @contacts: (element-type TelepathyGLib.Contact) (transfer container) (out) (allow-none):
+ * a location to set a #GPtrArray of upgraded #TpContact, or %NULL.

I'd prefer it if this returned the GPtrArray in the usual way, rather than outputting it through an out parameter.

+contacts_ready_cb (GObject *object,

In the example, I think this deserves a more appropriate (i.e. singular) name.

+ die_if (error, "tp_connection_get_contact_by_id_async()");

This leaks... I think the solution is to not use die_if there.

> _by_id_async() needs REQUEST iface on the connection, so the unit test

CONTACTS, surely?

+ * CMs. by_id and by_handle and used only be TpTextChannel and are needed for

Do you mean "are used only by"? If not, then I don't know what this means.

+_TP_DEPRECATED_IN_UNRELEASED
void tp_connection_get_contacts_by_handle (TpConnection *self,

I'm not sure that deprecating this immediately necessarily makes sense. If a client has obtained a handle from an extended (domain-specific) interface (perhaps a rubbish one, like Renaming), what's it meant to do with it? The answer in this branch seems to be "do GetContactAttributes yourself", and I don't think that's the answer we want.

[1] No, implementing it in the contacts mixin isn't sufficient to guarantee this, even if we assume that every CM uses tp-glib; consider a situation where you upgraded telepathy-glib and re-ran Empathy, but Gabble is still running with the old telepathy-glib.
Comment 8 Simon McVittie 2012-05-08 10:26:06 UTC
(In reply to comment #7)
> Since you've only added GetContactByID just now, I'd like a fallback path

Bonus penguin points if you fail-over to the deprecated tp_connection_get_contacts_by_id() for the fallback path :-)
Comment 9 Xavier Claessens 2012-05-09 01:16:03 UTC
(In reply to comment #7)
> Since you've only added GetContactByID just now, I'd like a fallback path via
> RequestHandles and GetContactAttributes if it raises NotImplemented (or
> anything other than InvalidHandle, really), so that it can be guaranteed to
> work on any Contacts CM[1].

I think this is useless, since all CM in the real world uses tp-glib now, and app wanting to work with hypotetical CM can just use deprecated symbols. Also the upgrade path has always been broken and will be even more with migration to 1.0. We don't like windows asking a reboot after update, be let's be serious, it's needed...

But anyway, did it.

> + * @contacts: (element-type TelepathyGLib.Contact) (transfer container) (out)
> (allow-none):
> + * a location to set a #GPtrArray of upgraded #TpContact, or %NULL.
> 
> I'd prefer it if this returned the GPtrArray in the usual way, rather than
> outputting it through an out parameter.

I do as an out param to make it optional. The user is supposed to already know its contacts, so it is common it won't need it. If I set it as return then user will have to unref it.

> + die_if (error, "tp_connection_get_contact_by_id_async()");
> 
> This leaks... I think the solution is to not use die_if there.

It is actually wrong usage of die_if(), that function does not make the process exit as I was expecting but just quit the main loop. So the caller still have to make the return.

> +_TP_DEPRECATED_IN_UNRELEASED
> void tp_connection_get_contacts_by_handle (TpConnection *self,
> 
> I'm not sure that deprecating this immediately necessarily makes sense. If a
> client has obtained a handle from an extended (domain-specific) interface
> (perhaps a rubbish one, like Renaming), what's it meant to do with it? The
> answer in this branch seems to be "do GetContactAttributes yourself", and I
> don't think that's the answer we want.

That's the whole point in deprecating stuff: users are notified that they need to change their code, fix rubish ifaces, etc. Just like I've discovered that Connection::self-handle and StreamTube::NewRemoteConnection does not give the id, so that code uses G_GNUC_BEGIN_IGNORE_DEPRECATIONS for now.



All the rest is now fixed in the branch
Comment 10 Simon McVittie 2012-05-09 04:28:45 UTC
Mostly looks good, some minor things:

+ g_set_error (&e, TP_ERROR, TP_ERROR_INVALID_ARGUMENT,
+ "We requested 1 id, and got an error for another id - Broken CM");

Please use (TP_DBUS_ERRORS, TP_DBUS_ERROR_INCONSISTENT) which specifically means "your CM/service is broken", and cannot be confused with a genuine D-Bus error.

+ g_set_error (&e, TP_ERROR, TP_ERROR_INVALID_ARGUMENT,
+ "We requested 1 id, but no contacts and no error - Broken CM");

Likewise.

+ * identifier in the IM protocol (an XMPP JIDs, SIP URIs, MSN Passports,
* AOL screen-names etc.)

Still unnecessarily plural: should be JID, URI, Passport, screen-name

(In reply to comment #9)
> > I'd prefer it if this returned the GPtrArray in the usual way, rather than
> > outputting it through an out parameter.
> 
> I do as an out param to make it optional. The user is supposed to already know
> its contacts, so it is common it won't need it. If I set it as return then user
> will have to unref it.

Hmm, I suppose so. It still seems odd, but if you frequently ignore it, OK.
Comment 11 Xavier Claessens 2012-05-09 06:00:54 UTC
Fixed lastest comments and pushed to spec and tp-glib

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.