Bug 30874

Something like this?

GetContactAttributesByID (as: indentifiers, as: interfaces) -> a{sa{sv}}: attributes, as: failed

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

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.

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" } }

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.

+ <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").

+ * @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.

(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 :-)

(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

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.

Fixed lastest comments and pushed to spec and tp-glib