Bug 41789

Branch at URL attempts to fix this by adding a X-FACEBOOK-ID field in the ContactInfo for facebook contacts.

+    contact_info = [(u'x-facebook-id', [], [u'test']),]

+        {handle: {cs.CONN_IFACE_CONTACT_INFO + '/info': contact_info,
+                  'org.freedesktop.Telepathy.Connection/contact-id': 'test@chat.facebook.com'}},

Do you really intend to leave the leading '-' on the Facebook IDs? JIDs on Facebook look like -1407062450@chat.facebook.com; this will yield 'x-facebook-id': '-1407062450' in the contact info. The test case should definitely use something formatted like a real Facebook Chat JID.

The first patch is kind of a PITA to review because it's actually two patches: one preserving the context for the vCard callback, and the other inserting the Facebook stuff…

 } VCardField;
+typedef struct {

There should be a space in between the two definitions.


+   if (username && tp_strdiff (server, "chat.facebook.com") == 0)

username != NULL; and tp_strdiff returns a gboolean, so you should use !tp_strdiff (server, "chat.facebook.com"). But in fact, I think I would be happier if there was a flag on the connection to say whether or not we're on Facebook (see http://cgit.collabora.com/git/user/wjt/telepathy-gabble-wjt.git/commit/?h=facebook-url for my WIP for the same feature); then _insert_server_specific_info() could just early-return if we're not on Facebook, rather than decomposing every single JID on every other connection.


+      VCardRequestInfo *request_info = g_new (VCardRequestInfo, 1);

Use g_slice_new(). I would be happier if there were new and free functions for VCardRequestInfo.

Ok, we decided to implement this using Addressing instead of ContactInfo. URL is now updated to point to the correct repo.

Branch updated with the latest changes in bug #30296 and dependencies.

> @@ -223,6 +223,28 @@ gabble_parse_vcard_address (const gchar *vcard_field,

This seems to be taking a JID and converting it into a Facebook ID. I'm pretty sure that's wrong: it should be expecting an unsigned ASCII integer (e.g. 12345) and returning -12345@chat.facebook.com, or raising an error if the vCard field value isn't an unsigned ASCII integer.

Now that I look at that function again: is it meant to return a JID, or a normalized value for that vCard field? For x-jabber they're equivalent, but for x-facebook-id they're not! You'll need to split it into "normalize", "to_jid", "jid_to", as for URIs.

Here are some high-level things that we want to be true:

    NormalizeVCardAddress("x-facebook-id", "12345") -> "12345"
    NormalizeVCardAddress("x-facebook-id", "not a number") -> error
    NormalizeVCardAddress("x-facebook-id", "-12345") -> error
    NormalizeVCardAddress("x-facebook-id", "-12345@chat.facebook.com") -> error
    NormalizeVCardAddress("x-jabber", "-12345@CHAT.FaceBook.com")
        -> "-12345@chat.facebook.com")

    GetContactsByVCardField("x-facebook-id", ["12345"], [])
        -> details of the contact with JID "-12345@chat.facebook.com"
    GetContactsByVCardField("x-facebook-id", [x], [])
        -> no result for any x that produced an error above

    "/addresses" attr of "-12345@chat.facebook.com"
        -> { "x-jabber": "-12345@chat.facebook.com", "x-facebook-id": "12345" }
    "/addresses" attr of "dave@example.com"
        -> { "x-jabber": "dave@example.com" }

Something we should have as a general principle is that for all values of field and old_value that don't cause this call to raise an error:

    new_value = NormalizeVCardAddress(field, old_value)

we should be able to normalize it again and get the same thing:

    NormalizeVCardAddress(field, new_value) == new_value

Otherwise we can't really call it "normalization". I don't think that's true in this branch.

(In reply to comment #5)
> > @@ -223,6 +223,28 @@ gabble_parse_vcard_address (const gchar *vcard_field,
> 
> This seems to be taking a JID and converting it into a Facebook ID. I'm pretty
> sure that's wrong: it should be expecting an unsigned ASCII integer (e.g.
> 12345) and returning -12345@chat.facebook.com, or raising an error if the vCard
> field value isn't an unsigned ASCII integer.
> 
> Now that I look at that function again: is it meant to return a JID, or a
> normalized value for that vCard field? For x-jabber they're equivalent, but for
> x-facebook-id they're not! You'll need to split it into "normalize", "to_jid",
> "jid_to", as for URIs.
> 
> Here are some high-level things that we want to be true:
> 
>     NormalizeVCardAddress("x-facebook-id", "12345") -> "12345"
>     NormalizeVCardAddress("x-facebook-id", "not a number") -> error
>     NormalizeVCardAddress("x-facebook-id", "-12345") -> error
>     NormalizeVCardAddress("x-facebook-id", "-12345@chat.facebook.com") -> error
>     NormalizeVCardAddress("x-jabber", "-12345@CHAT.FaceBook.com")
>         -> "-12345@chat.facebook.com")
> 
>     GetContactsByVCardField("x-facebook-id", ["12345"], [])
>         -> details of the contact with JID "-12345@chat.facebook.com"
>     GetContactsByVCardField("x-facebook-id", [x], [])
>         -> no result for any x that produced an error above
> 
>     "/addresses" attr of "-12345@chat.facebook.com"
>         -> { "x-jabber": "-12345@chat.facebook.com", "x-facebook-id": "12345" }
>     "/addresses" attr of "dave@example.com"
>         -> { "x-jabber": "dave@example.com" }
Tnx for the review, I've updated the branch with the suggested changes and also rebased it on top of the changes in bug #30296.

> addressing-util: Refactor gabble_parse_vcard_address in
> gabble_normalize_vcard_address, gabble_vcard_address_to_jid
> and gabble_jid_to_vcard_address.

Er, this isn't (just) refactoring... it's also "... and make x-facebook-id parsing expect a number, not a JID". Please be a bit more careful with your commit messages - they're all the context you'll have in n years' time when you're debugging this stuff and have forgotten how it worked :-)

> + while (*s && (g_ascii_isalnum (*s))) s++;

That's not "is numeric" :-)

(also, our coding style would have you use two lines for that)

The implementation looks OK now.

(In reply to comment #8)
> > addressing-util: Refactor gabble_parse_vcard_address in
> > gabble_normalize_vcard_address, gabble_vcard_address_to_jid
> > and gabble_jid_to_vcard_address.
> 
> Er, this isn't (just) refactoring... it's also "... and make x-facebook-id
> parsing expect a number, not a JID". Please be a bit more careful with your
> commit messages - they're all the context you'll have in n years' time when
> you're debugging this stuff and have forgotten how it worked :-)
Amended the commit with a better msg :)

> > + while (*s && (g_ascii_isalnum (*s))) s++;
> 
> That's not "is numeric" :-)
Haha, I swear I misread isnum there, changed to isdigit now :P
 
> (also, our coding style would have you use two lines for that)
Done also.

Looks OK, blocked by previous branches.

0.15.1