Bug 20831

Somewhat high priority, since it blocks undrafting both ContactInfo and ContactSearch, and shouldn't be so hard.

Review up to 1988a05:

Did you mean to commit a different version of Wocky in the first patch? It looks like you've downgraded it accidentally? (If `git status` is telling you that lib/ext/wocky has changed, `git submodule update` should convince it otherwise.)

In _request_vcard_cb (for instance), you don't need to go via GValue to make a GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST. You can just say (in general):

    GPtrArray *contact_info = dbus_g_type_specialized_construct (
        GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST);

    /* do stuff */

    g_boxed_free (GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST, contact_info);

I think _parse_vcard() should return a new GPtrArray *, or NULL if it can't parse the vCard for some reason, rather than accepting a pointer to one and poking stuff into it.

I thought this code:

>           gchar **field_values;
> 
>           field_values = g_new0 (gchar *, 2);
>           field_values[0] = g_strdup (lm_message_node_get_value (node));
>           _insert_contact_field (contact_info, node->name,
>               NULL, field_values);

was leaking field_values, but it's not because _insert_contact_field ... frees
its arguments. I think this would be clearer:

    const gchar * const field_values[2] = {
        lm_message_node_get_value (node),
        NULL
    };

    _insert_contact_field (contact_info, node->name, NULL, field_values);

>   /* we can simply omit a type if not found */
>   for (i = 0; i < supported_types_size; ++i)
>     {
>        child_node = lm_message_node_get_child (node, supported_types[i]);
>        if (child_node == NULL)
>          continue;
> 
>        types = g_slist_prepend (types, child_node->name);
>        ++types_count;
>     }
> 
>   if (types_count > 0)
>     {
>       GSList *walk;
> 
>       i = 0;
>       field_params = g_new0 (gchar *, types_count + 1);
>       for (walk = types; walk != NULL; walk = walk->next)
>         {
>           type = g_ascii_strdown ((const gchar *) walk->data, -1);
>           field_params[i++] = g_strconcat ("type=", type, NULL);
>           g_free (type);
>         }
>     }

I'd build the NULL-terminated array of params and values with GPtrArray, like
this:

    GPtrArray *params = g_ptr_array_new ();
    GPtrArray *values = g_ptr_array_sized_new (
        g_strv_length (mandatory_fields) + 1);

    for (i = 0; i < supported_types_size; ++i)
      {
        gchar *tmp;

        child_node = lm_message_node_get_child (node, supported_types[i]);
        if (child_node == NULL)
          continue;

        tmp = g_ascii_strdown (child_node->name);
        g_ptr_array_add (params, g_strdup_printf ("type=%s", tmp));
        g_free (tmp);
      }

    g_ptr_array_add (params, NULL);

    for (i = 0; i < mandatory_fields_size; ++i)
      {
        child_node = lm_message_node_get_child (node, mandatory_fields[i]);
        if (child_node != NULL)
          g_ptr_array_add (values, lm_message_node_get_value (child_node));
        else
          g_ptr_array_add (values, "");
      }

    g_ptr_array_add (values, NULL);


    /* ... */
    _insert_contact_field (contact_info, node->name, params->data,
        field_values);

    /* We allocated the strings in params, so need to free them */
    g_strfreev (g_ptr_array_free (params, FALSE));
    /* But we borrowed the ones in values, so just free the box */
    g_ptr_array_free (values, TRUE);

>               guint j;
>               gchar **nicknames = g_strsplit (node_value, ",", -1);
> 
>               for (j = 0; nicknames[j]; ++j)
>                 {

I'd iterate this directly:

    gchar **nicknames = g_strsplit (node_value, ",", -1);
    gchar **p;

    for (p = nicknames; *p != NULL; p++)
      {

There are two blocks which claim to handle the ORG field. They can't both be right. :-)

why does _parse_vcard () claim it can fail, but always return TRUE? :-)

>       if (!node->name || strcmp (node->name, "") == 0)
>         continue;

If message nodes have null or empty names, the XML parser should have cried already. But it doesn't hurt to be defensive.

I wonder whether the similar cases in _parse_vcard could use a lookup table. Something like:

    typedef struct {
        const gchar *name;
        const gchar * const types[];
        const gchar * const elements[];
    } Foo;

    Foo adr = {
        adr,
        { "HOME", "WORK", "POSTAL",
              "PARCEL", "DOM", "INTL", "PREF", NULL },
        { "POBOX", "EXTADD", "STREET",
              "LOCALITY", "REGION", "PCODE", "CTRY", NULL }
    };

    Foo fields[] = {
        adr,
        ...,
        { NULL, }
    };

Depends on the C rules for defining nested arrays, which I can never remember and always trip me up, but it would make _parse_vcard much easier to read.


> static void
> _create_contact_field_extended (GPtrArray *contact_info,
>                                 LmMessageNode *node,
>                                 const gchar **supported_types,
>                                 const gchar **mandatory_fields)

Those should be const gchar * const *.

gabble_connection_get_contact_info() leaks the hash table it returns.

>           /* TODO what now? we have the cached vcard but it cannot be parsed,
>            * skipping */

Good question. We don't have a good way to report that a contact's supposed vCard is garbage with this API. But then again, maybe just omitting it is all we need. We return an error from RequestContactInfo, which is enough.

But it would be worth adding a DEBUG() here.

I'm not sure why RequestVCardContext is needed. The GabbleConnection and GabbleSvcConnectionInterfaceContactInfo members are the same pointer, and the handle is supplied to the callback anyway.

Instead, I'd store the return value of gabble_vcard_manager_request() in self->vcard_requests.

Maybe GetContactInfo should be tolerant of invalid handles, and just ignore them.

>       GError tp_error = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
>           vcard_error->message };

We could map GABBLE_VCARD_MANAGER_ERROR_CANCELLED to TP_ERROR_CANCELLED, for instance.

>   if (gabble_vcard_manager_get_cached (self->vcard_manager,
>                                        contact, &vcard_node))
>     {
>       _request_vcard_cb (self->vcard_manager, NULL, contact, vcard_node, NULL,
>           context);
>     }
>   else
>     {
>       gabble_vcard_manager_request (self->vcard_manager, contact, 0,
>           _request_vcard_cb, context, NULL);
>     }

Hmm. Maybe this calls for a helper function in GabbleVCardManager. But even without one, I'd rather this didn't call the callback directly, passing NULL for @request (which is normally non-NULL for GabbleVCardManagerCbs). Could you move the body of _request_vcard_cb() to a function which just takes the arguments it uses, and make the callback call that?

>   if (G_VALUE_HOLDS (&supported_fields, GABBLE_ARRAY_TYPE_FIELD_SPECS))
>     {
>       g_value_init (&supported_fields, GABBLE_ARRAY_TYPE_FIELD_SPECS);

I think this is backwards, so it will never be initialized. But it would be better just to add conn_contact_info_class_init, called from GabbleConnection's class_init, which unconditionally initializes it. And as with GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST, you could just make this with dbus_g_type_specialized_construct() and have the global be a GPtrArray *.

> static TpDBusPropertiesMixinPropImpl props[] = {
>       { "ContactInfoFlags", GUINT_TO_POINTER (
>               GABBLE_CONTACT_INFO_FLAG_CAN_SET | GABBLE_CONTACT_INFO_FLAG_PUSH),

XMPP shouldn't have the Push flag set. The spec says:

> Indicates that the protocol pushes all contacts' information to the connection manager without prompting. If set, RequestContactInfo will not cause a network roundtrip and ContactInfoChanged will be emitted whenever contacts' information changes.

I don't think this is true for XMPP — we don't have updated vCards pushed to us.

(In reply to comment #2)
> Review up to 1988a05:
> 
> Did you mean to commit a different version of Wocky in the first patch? It
> looks like you've downgraded it accidentally? (If `git status` is telling you
> that lib/ext/wocky has changed, `git submodule update` should convince it
> otherwise.)
Nope my fault, I had to rebase in order to remove this

> 
> In _request_vcard_cb (for instance), you don't need to go via GValue to make a
> GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST. You can just say (in general):
> 
>     GPtrArray *contact_info = dbus_g_type_specialized_construct (
>         GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST);
> 
>     /* do stuff */
> 
>     g_boxed_free (GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST, contact_info);
Done
 
> I think _parse_vcard() should return a new GPtrArray *, or NULL if it can't
> parse the vCard for some reason, rather than accepting a pointer to one and
> poking stuff into it.
Done

> I thought this code:
> 
> >           gchar **field_values;
> > 
> >           field_values = g_new0 (gchar *, 2);
> >           field_values[0] = g_strdup (lm_message_node_get_value (node));
> >           _insert_contact_field (contact_info, node->name,
> >               NULL, field_values);
> 
> was leaking field_values, but it's not because _insert_contact_field ... frees
> its arguments. I think this would be clearer:
> 
>     const gchar * const field_values[2] = {
>         lm_message_node_get_value (node),
>         NULL
>     };
> 
>     _insert_contact_field (contact_info, node->name, NULL, field_values);
Done
 
> >   /* we can simply omit a type if not found */
> >   for (i = 0; i < supported_types_size; ++i)
> >     {
> >        child_node = lm_message_node_get_child (node, supported_types[i]);
> >        if (child_node == NULL)
> >          continue;
> > 
> >        types = g_slist_prepend (types, child_node->name);
> >        ++types_count;
> >     }
> > 
> >   if (types_count > 0)
> >     {
> >       GSList *walk;
> > 
> >       i = 0;
> >       field_params = g_new0 (gchar *, types_count + 1);
> >       for (walk = types; walk != NULL; walk = walk->next)
> >         {
> >           type = g_ascii_strdown ((const gchar *) walk->data, -1);
> >           field_params[i++] = g_strconcat ("type=", type, NULL);
> >           g_free (type);
> >         }
> >     }
> 
> I'd build the NULL-terminated array of params and values with GPtrArray, like
> this:
> 
>     GPtrArray *params = g_ptr_array_new ();
>     GPtrArray *values = g_ptr_array_sized_new (
>         g_strv_length (mandatory_fields) + 1);
> 
>     for (i = 0; i < supported_types_size; ++i)
>       {
>         gchar *tmp;
> 
>         child_node = lm_message_node_get_child (node, supported_types[i]);
>         if (child_node == NULL)
>           continue;
> 
>         tmp = g_ascii_strdown (child_node->name);
>         g_ptr_array_add (params, g_strdup_printf ("type=%s", tmp));
>         g_free (tmp);
>       }
> 
>     g_ptr_array_add (params, NULL);
> 
>     for (i = 0; i < mandatory_fields_size; ++i)
>       {
>         child_node = lm_message_node_get_child (node, mandatory_fields[i]);
>         if (child_node != NULL)
>           g_ptr_array_add (values, lm_message_node_get_value (child_node));
>         else
>           g_ptr_array_add (values, "");
>       }
> 
>     g_ptr_array_add (values, NULL);
> 
> 
>     /* ... */
>     _insert_contact_field (contact_info, node->name, params->data,
>         field_values);
> 
>     /* We allocated the strings in params, so need to free them */
>     g_strfreev (g_ptr_array_free (params, FALSE));
>     /* But we borrowed the ones in values, so just free the box */
>     g_ptr_array_free (values, TRUE);
> 
> >               guint j;
> >               gchar **nicknames = g_strsplit (node_value, ",", -1);
> > 
> >               for (j = 0; nicknames[j]; ++j)
> >                 {
Done with slightly changes. Values param is still gchar ** instead of GPtrArray.
 
> I'd iterate this directly:
> 
>     gchar **nicknames = g_strsplit (node_value, ",", -1);
>     gchar **p;
> 
>     for (p = nicknames; *p != NULL; p++)
>       {
Done

> There are two blocks which claim to handle the ORG field. They can't both be
> right. :-)
Fixed
 
> why does _parse_vcard () claim it can fail, but always return TRUE? :-)
> 
> >       if (!node->name || strcmp (node->name, "") == 0)
> >         continue;
> 
> If message nodes have null or empty names, the XML parser should have cried
> already. But it doesn't hurt to be defensive.
:)
 
> I wonder whether the similar cases in _parse_vcard could use a lookup table.
> Something like:
> 
>     typedef struct {
>         const gchar *name;
>         const gchar * const types[];
>         const gchar * const elements[];
>     } Foo;
> 
>     Foo adr = {
>         adr,
>         { "HOME", "WORK", "POSTAL",
>               "PARCEL", "DOM", "INTL", "PREF", NULL },
>         { "POBOX", "EXTADD", "STREET",
>               "LOCALITY", "REGION", "PCODE", "CTRY", NULL }
>     };
> 
>     Foo fields[] = {
>         adr,
>         ...,
>         { NULL, }
>     };
> 
> Depends on the C rules for defining nested arrays, which I can never remember
> and always trip me up, but it would make _parse_vcard much easier to read.
I didn't change this, it would be a big change and I don't see a strong reason to do so.

> 
> > static void
> > _create_contact_field_extended (GPtrArray *contact_info,
> >                                 LmMessageNode *node,
> >                                 const gchar **supported_types,
> >                                 const gchar **mandatory_fields)
> 
> Those should be const gchar * const *.
Done

> gabble_connection_get_contact_info() leaks the hash table it returns.
Fixed

> >           /* TODO what now? we have the cached vcard but it cannot be parsed,
> >            * skipping */
> 
> Good question. We don't have a good way to report that a contact's supposed
> vCard is garbage with this API. But then again, maybe just omitting it is all
> we need. We return an error from RequestContactInfo, which is enough.
> 
> But it would be worth adding a DEBUG() here.
Done
 
> I'm not sure why RequestVCardContext is needed. The GabbleConnection and
> GabbleSvcConnectionInterfaceContactInfo members are the same pointer, and the
> handle is supplied to the callback anyway.
> 
> Instead, I'd store the return value of gabble_vcard_manager_request() in
> self->vcard_requests.
Done, removed RequestVCardContext completely

> Maybe GetContactInfo should be tolerant of invalid handles, and just ignore
> them.
>
> >       GError tp_error = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
> >           vcard_error->message };
> 
> We could map GABBLE_VCARD_MANAGER_ERROR_CANCELLED to TP_ERROR_CANCELLED, for
> instance.
>
I see one problem here, IMHO the spec should state this first then we implement it. The spec does not say anything about invalid handles netiher TP_ERROR_CANCELLED is a valid error. I let this as is and then we can decide what to do later (update spec first? implement first? ...)
 
> >   if (gabble_vcard_manager_get_cached (self->vcard_manager,
> >                                        contact, &vcard_node))
> >     {
> >       _request_vcard_cb (self->vcard_manager, NULL, contact, vcard_node, NULL,
> >           context);
> >     }
> >   else
> >     {
> >       gabble_vcard_manager_request (self->vcard_manager, contact, 0,
> >           _request_vcard_cb, context, NULL);
> >     }
> 
> Hmm. Maybe this calls for a helper function in GabbleVCardManager. But even
> without one, I'd rather this didn't call the callback directly, passing NULL
> for @request (which is normally non-NULL for GabbleVCardManagerCbs). Could you
> move the body of _request_vcard_cb() to a function which just takes the
> arguments it uses, and make the callback call that?
Done

> >   if (G_VALUE_HOLDS (&supported_fields, GABBLE_ARRAY_TYPE_FIELD_SPECS))
> >     {
> >       g_value_init (&supported_fields, GABBLE_ARRAY_TYPE_FIELD_SPECS);
> 
> I think this is backwards, so it will never be initialized. But it would be
> better just to add conn_contact_info_class_init, called from GabbleConnection's
> class_init, which unconditionally initializes it. And as with
> GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST, you could just make this with
> dbus_g_type_specialized_construct() and have the global be a GPtrArray *.
Done
 
> > static TpDBusPropertiesMixinPropImpl props[] = {
> >       { "ContactInfoFlags", GUINT_TO_POINTER (
> >               GABBLE_CONTACT_INFO_FLAG_CAN_SET | GABBLE_CONTACT_INFO_FLAG_PUSH),
> 
> XMPP shouldn't have the Push flag set. The spec says:
> 
> > Indicates that the protocol pushes all contacts' information to the connection manager without prompting. If set, RequestContactInfo will not cause a network roundtrip and ContactInfoChanged will be emitted whenever contacts' information changes.
> 
> I don't think this is true for XMPP — we don't have updated vCards pushed to
> us.
Fixed

Accepting this bug to review the branch.

          const gchar *node_value = lm_message_node_get_value (node);

          if (strchr (node_value, ','))
            {
              gchar **nicknames = g_strsplit (node_value, ",", -1);

Can nicknames contain escaped commas? If so, this doesn't do the right thing in those cases.

          /* TODO what now? we have the cached vcard but it cannot be parsed,
           * skipping */

I don't think this is a TODO: what else would we do? :)

GabbleVCardManagerEditInfo:

Firstly, rather than:

+  edit_info = g_new0 (GabbleVCardManagerEditInfo, 1);
+  edit_info->element_name = g_strdup ("PHOTO");

there should be a _new () function to build an edit. They should be slice-allocated. Maybe the function would look something like this:

gabble_vcard_edit_new (
    const gchar *field,
    const gchar *value,
    gboolean accept_multiple,
    gboolean to_del,
    ...);

where the varargs are a NULL-terminated list of (child node, value) pairs?

Given that every existing user of gabble_vcard_manager_edit() passes one pair to it, I'd suggest renaming it to gabble_vcard_manager_edit_one()—changing its signature accordingly—and renaming _edit_extended() to _edit().

Please document that gabble_vcard_manager_edit() takes ownership of the list and its contents.

Are there any vCard fields that need more than two levels of nesting? I wonder whether using a LmMessageNode rather than a GHashTable would be clearer if so. But probably not.

+      g_hash_table_foreach (info->to_edit, delete_edit_info_to_edit_foreach,
+          NULL);
+      g_hash_table_destroy (info->to_edit);

If you pass g_free as the key and value destructors to g_hash_table_new_full() you don't need to iterate manually.

+          GabbleVCardManagerEditInfo *info = g_new0 (GabbleVCardManagerEditInfo, 1);
+          info->element_name = g_strdup ("NICKNAME");
+          info->element_value = alias;
+          priv->edits = g_slist_append (priv->edits, info);

why can't this use the normal _edit() function?

If this happens:

• SetAvatar(self, stuff)
• Gabble asks for the vCard
• SetContactInfo(...)
• Gabble gets the vCard

What does PHOTO get set to? What about if the SetAvatar and SetContactInfo calls are the other way round?

  node = lm_message_node_get_child (vcard_node, info->element_name);
  if (info->to_del)
    {
      while (node)
        {
          if (node)
            {
              lm_message_node_unlink (node, vcard_node);
              lm_message_node_unref (node);
            }
          node = lm_message_node_get_child (vcard_node, info->element_name);
        }
      return;
    }

while (node) { if (node) { ... } } is redundant. And, node != NULL is our C style for pointers.

  if (node && !info->accept_multiple)
    lm_message_node_set_value (node, info->element_value);
  else
    node = lm_message_node_add_child (vcard_node,
        info->element_name, info->element_value);

So... if we're not accepting multiple copies of a field, we replace the first one we see and hope there aren't any others?

SetContactInfo implementation:

Hey look. Now you have gabble_connection_set_contact_info() which duplicates the Telepathy↔XMPP field mapping in _parse_vcard, but backwards. I think this is a strong reason to have a lookup table, rather than two massive, parallel if/else if/else if/... blocks.

Why call

+      if (_gabble_connection_signal_own_presence (conn, &error))

in _set_contact_info_cb()? If we changed the PHOTO field, the avatar code will have done that; if we didn't, this is redundant.

      else if (strcmp (field_name, "label") == 0)
        {
          const gchar * const elements[] = { "LINE", NULL };

What does this field mean, OOI?

Can you maybe test a more complicated vCard? For instance, plural NICKNAMEs? And also add test cases for overlapping SetAvatar/SetAlias/SetContactInfo calls?

I'd forgotten how often you have to cast between gchar ** and const gchar * const * in practice if you use the latter. Sigh. C.

Removing the patch keyword while you respond to Will's review; please put it back when this branch is ready for more review.

(In reply to comment #3)
> > >       GError tp_error = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
> > >           vcard_error->message };
> > 
> > We could map GABBLE_VCARD_MANAGER_ERROR_CANCELLED to TP_ERROR_CANCELLED, for
> > instance.
> >
> I see one problem here, IMHO the spec should state this first then we implement
> it. The spec does not say anything about invalid handles netiher
> TP_ERROR_CANCELLED is a valid error. I let this as is and then we can decide
> what to do later (update spec first? implement first? ...)

As spec maintainer, my position is that any method can raise any error; the ones in the spec provide a hint to clients about the errors they can expect to see, and provide a hint to services about the correct representation for particular errors. If there are more errors that would be appropriate, please use them, and open an enhancement bug against telepathy-spec (ideally with a patch).

(In reply to comment #5)
>           const gchar *node_value = lm_message_node_get_value (node);
> 
>           if (strchr (node_value, ','))
>             {
>               gchar **nicknames = g_strsplit (node_value, ",", -1);
> 
> Can nicknames contain escaped commas? If so, this doesn't do the right thing in
> those cases.
Done.

> 
>           /* TODO what now? we have the cached vcard but it cannot be parsed,
>            * skipping */
> 
> I don't think this is a TODO: what else would we do? :)
Done

> GabbleVCardManagerEditInfo:
> 
> Firstly, rather than:
> 
> +  edit_info = g_new0 (GabbleVCardManagerEditInfo, 1);
> +  edit_info->element_name = g_strdup ("PHOTO");
> 
> there should be a _new () function to build an edit. They should be
> slice-allocated. Maybe the function would look something like this:
> 
> gabble_vcard_edit_new (
>     const gchar *field,
>     const gchar *value,
>     gboolean accept_multiple,
>     gboolean to_del,
>     ...);
> 
> where the varargs are a NULL-terminated list of (child node, value) pairs?
Done.

> Given that every existing user of gabble_vcard_manager_edit() passes one pair
> to it, I'd suggest renaming it to gabble_vcard_manager_edit_one()—changing
> its signature accordingly—and renaming _edit_extended() to _edit().
Done

> Please document that gabble_vcard_manager_edit() takes ownership of the list
> and its contents.
Done

> Are there any vCard fields that need more than two levels of nesting? I wonder
> whether using a LmMessageNode rather than a GHashTable would be clearer if so.
> But probably not.
Nope, there is not field that requires more than 2 levels of nesting. But I wouldn't change that, I think it's easier to use GHashTable than LmMessageNode IMHO.

> +      g_hash_table_foreach (info->to_edit, delete_edit_info_to_edit_foreach,
> +          NULL);
> +      g_hash_table_destroy (info->to_edit);
> 
> If you pass g_free as the key and value destructors to g_hash_table_new_full()
> you don't need to iterate manually.
Done. I wasn't using this as someone could create the hash table using g_hash_table_new, but we now mandate that the hash is created with new_full.

> +          GabbleVCardManagerEditInfo *info = g_new0
> (GabbleVCardManagerEditInfo, 1);
> +          info->element_name = g_strdup ("NICKNAME");
> +          info->element_value = alias;
> +          priv->edits = g_slist_append (priv->edits, info);
> 
> why can't this use the normal _edit() function?
Done

> If this happens:
> 
> • SetAvatar(self, stuff)
> • Gabble asks for the vCard
> • SetContactInfo(...)
> • Gabble gets the vCard
> 
> What does PHOTO get set to? What about if the SetAvatar and SetContactInfo
> calls are the other way round?
ContactInfo does not change avatar at all. It ignores PHOTO fields.
 
>   node = lm_message_node_get_child (vcard_node, info->element_name);
>   if (info->to_del)
>     {
>       while (node)
>         {
>           if (node)
>             {
>               lm_message_node_unlink (node, vcard_node);
>               lm_message_node_unref (node);
>             }
>           node = lm_message_node_get_child (vcard_node, info->element_name);
>         }
>       return;
>     }
> 
> while (node) { if (node) { ... } } is redundant. And, node != NULL is our C
> style for pointers.
Done :D
 
>   if (node && !info->accept_multiple)
>     lm_message_node_set_value (node, info->element_value);
>   else
>     node = lm_message_node_add_child (vcard_node,
>         info->element_name, info->element_value);
> 
> So... if we're not accepting multiple copies of a field, we replace the first
> one we see and hope there aren't any others?
Yep, that is what we do. Usually replace_vcard is set to true before SetContactInfo, so the vcard will be replaced, and there will be no duplicated field.
 
> SetContactInfo implementation:
> 
> Hey look. Now you have gabble_connection_set_contact_info() which duplicates
> the Telepathy↔XMPP field mapping in _parse_vcard, but backwards. I think this
> is a strong reason to have a lookup table, rather than two massive, parallel
> if/else if/else if/... blocks.
> 
This would require lots of changes, if you really think this is neede I can change it, but I prefer the way it is.

The ideal would be:
{ "element_name", get_contact_info_method, set_contact_info_method }

We would have to add various new methods and change most of the code.

> Why call
> 
> +      if (_gabble_connection_signal_own_presence (conn, &error))
> 
> in _set_contact_info_cb()? If we changed the PHOTO field, the avatar code will
> have done that; if we didn't, this is redundant.
>
Done, removed the call.
 
>       else if (strcmp (field_name, "label") == 0)
>         {
>           const gchar * const elements[] = { "LINE", NULL };
> 
> What does this field mean, OOI?
From rfc2426 -> "Type purpose: To specify the formatted text corresponding to delivery address of the object the vCard represents." 
 
> Can you maybe test a more complicated vCard? For instance, plural NICKNAMEs?
> And also add test cases for overlapping SetAvatar/SetAlias/SetContactInfo
> calls?
Done

> I'd forgotten how often you have to cast between gchar ** and const gchar *
> const * in practice if you use the latter. Sigh. C.
:/

(In reply to comment #6)
> Removing the patch keyword while you respond to Will's review; please put it
> back when this branch is ready for more review.
> 
> (In reply to comment #3)
> > > >       GError tp_error = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
> > > >           vcard_error->message };
> > > 
> > > We could map GABBLE_VCARD_MANAGER_ERROR_CANCELLED to TP_ERROR_CANCELLED, for
> > > instance.
> > >
> > I see one problem here, IMHO the spec should state this first then we implement
> > it. The spec does not say anything about invalid handles netiher
> > TP_ERROR_CANCELLED is a valid error. I let this as is and then we can decide
> > what to do later (update spec first? implement first? ...)
> 
> As spec maintainer, my position is that any method can raise any error; the
> ones in the spec provide a hint to clients about the errors they can expect to
> see, and provide a hint to services about the correct representation for
> particular errors. If there are more errors that would be appropriate, please
> use them, and open an enhancement bug against telepathy-spec (ideally with a
> patch).
> 
Added more possible error codes to RequestContactInfo.

I will open a spec enhancement bug as soon as this branch is approved.

Re-add patch keyword. Ready for review again.

Just a quick comment before full review...

(In reply to comment #7)
> (In reply to comment #5)
> > If this happens:
> > 
> > • SetAvatar(self, stuff)
> > • Gabble asks for the vCard
> > • SetContactInfo(...)
> > • Gabble gets the vCard
> > 
> > What does PHOTO get set to? What about if the SetAvatar and SetContactInfo
> > calls are the other way round?
> ContactInfo does not change avatar at all. It ignores PHOTO fields.

Okay, can we have some tests that test this?

I want to keep this bug moving, so we can finally undraft ContactInfo and hence ContactSearch, so I'll take it.

(In reply to comment #10)
> Okay, can we have some tests that test this?

I'll look into it, and see how feasible it would be to have lookup tables (which I think would probably be a very good idea).

I merged Gabble 0.9.5 into the branch, for which I had to make the following three non-trivial edits, and basically rewrite vcard_node_changed() (from master) to cope with the more complex data structure used in the contact-info branch.

Do these merges look OK?

--- a/extensions/Connection_Interface_Contact_Info.xml
+++ b/extensions/Connection_Interface_Contact_Info.xml
@@@ -162,7 -162,7 +162,7 @@@ Foundation, Inc., 51 Franklin Street, F
          name="Contact_Info"/>
      </tp:mapping>
  
--    <signal name="ContactInfoChanged" tp:name-for-bindings="ContactInfoChanged">
++    <signal name="ContactInfoChanged" tp:name-for-bindings="Contact_Info_Changed">
        <arg name="Contact" type="u" tp:type="Contact_Handle">
          <tp:docstring>
            An integer handle for the contact whose info has changed.

--- a/src/conn-contact-info.c
+++ b/src/conn-contact-info.c
 +static GSList *
 +_insert_edit_info (GSList *edits,
...
 +  for (p = field_params; *p != NULL; ++p)
 +    {
-       // params should be in the format type=foo
-       gchar *delim = strchr(*p, '=');
++      /* params should be in the format type=foo */
++      gchar *delim = strchr (*p, '=');
 +      if (!delim)
 +        continue;
 +
 +      g_hash_table_insert (edit_info->to_edit,
-           g_ascii_strup(delim + 1, -1),
++          g_ascii_strup (delim + 1, -1),
 +          NULL);
 +    }

--- a/src/connection.c
+++ b/src/connection.c
    static TpDBusPropertiesMixinIfaceImpl prop_interfaces[] = {
--        { GABBLE_IFACE_OLPC_GADGET,
++        /* 0 */ { GABBLE_IFACE_OLPC_GADGET,
            conn_olpc_gadget_properties_getter,
            NULL,
            olpc_gadget_props,
          },
--        { TP_IFACE_CONNECTION_INTERFACE_LOCATION,
++        /* 1 */ { TP_IFACE_CONNECTION_INTERFACE_LOCATION,
            conn_location_properties_getter,
            conn_location_properties_setter,
            location_props,
          },
--        { TP_IFACE_CONNECTION_INTERFACE_AVATARS,
++        /* 2 */ { TP_IFACE_CONNECTION_INTERFACE_AVATARS,
            conn_avatars_properties_getter,
            NULL,
            NULL,
          },
-         { GABBLE_IFACE_CONNECTION_INTERFACE_CONTACT_INFO,
 -        { GABBLE_IFACE_CONNECTION_INTERFACE_GABBLE_DECLOAK,
++        /* 3 */ { GABBLE_IFACE_CONNECTION_INTERFACE_CONTACT_INFO,
 +          conn_contact_info_properties_getter,
 +          NULL,
 +          NULL,
 +        },
++        /* 4 */ { GABBLE_IFACE_CONNECTION_INTERFACE_GABBLE_DECLOAK,
+           tp_dbus_properties_mixin_getter_gobject_properties,
+           tp_dbus_properties_mixin_setter_gobject_properties,
+           decloak_props,
+         },

vcard/overlapping-sets.py is currently failing, which I suspect is probably because my rewrite of vcard_node_changed() is faulty; I'll put that here for review when I've fixed it.

These merges seem ok, also it's good to remember that some tests fail due to some race conditions. Usually when I run the tests directly they work, but some fail with make check.

> vcard/overlapping-sets.py is currently failing

Not a regression, as it turns out; it fails on Andre's branch too :-P

Here's my new version of vcard_node_changed (and the top of the function that calls it) for your reviewing convenience:

static gboolean
vcard_node_changed (GabbleConnection *conn,
                    LmMessageNode *vcard_node,
                    GabbleVCardManagerEditInfo *edit)
{
  LmMessageNode *node;

  if (conn->features & GABBLE_CONNECTION_FEATURES_GOOGLE_ROSTER)
    {
      /* Google's server only stores N, FN and PHOTO */
      if (tp_strdiff (edit->element_name, "N") &&
          tp_strdiff (edit->element_name, "FN") &&
          tp_strdiff (edit->element_name, "PHOTO"))
        {
          DEBUG ("ignoring vcard node %s because this is a Google server",
              edit->element_name);
          return FALSE;
        }
    }

  if (edit->accept_multiple)
    {
      /* we're adding, not replacing, which is always a difference */
      DEBUG ("vcard node %s to be appended, vcard needs update",
          edit->element_name);
      return TRUE;
    }

  node = lm_message_node_get_child (vcard_node, edit->element_name);

  if (edit->to_del)
    {
      /* deleting and any other operation are mutually exclusive */
      g_assert (edit->to_edit == NULL);
      g_assert (edit->element_value == NULL);

      if (node != NULL)
        {
          DEBUG ("vcard node %s deleted, vcard needs update",
              edit->element_name);
          return TRUE;
        }
      else
        {
          return FALSE;
        }
    }

  if (node == NULL)
    {
          DEBUG ("vcard node %s added, vcard needs update",
              edit->element_name);
          return TRUE;
    }
  else
    {
      const gchar *node_value = lm_message_node_get_value (node);
      const gchar *edit_value = edit->element_value;

      /* NULL and "" are the same, as far as XML content goes */
      if (edit_value == NULL)
        edit_value = "";

      if (node_value == NULL)
        node_value = "";

      if (tp_strdiff (node_value, edit_value))
        {
          DEBUG ("vcard node %s changed, vcard needs update",
              edit->element_name);
          return TRUE;
        }

      if (edit->to_edit != NULL)
        {
          GHashTableIter iter;
          gpointer k, v;

          g_hash_table_iter_init (&iter, edit->to_edit);

          while (g_hash_table_iter_next (&iter, &k, &v))
            {
              LmMessageNode *child = lm_message_node_get_child (node, k);

              node_value = lm_message_node_get_value (child);

              if (v == NULL)
                v = "";

              if (node_value == NULL)
                node_value = "";

              if (tp_strdiff (node_value, v))
                {
                  DEBUG ("vcard node %s/%s changed, vcard needs update",
                      edit->element_name, (const gchar *) k);
                  return TRUE;
                }
            }
        }
    }

  return FALSE;
}

static void
manager_patch_vcard (GabbleVCardManager *self,
                     LmMessageNode *vcard_node)
{
  GabbleVCardManagerPrivate *priv = self->priv;
  LmMessage *msg;
  LmMessageNode *patched_vcard;
  GList *li;
  GSList *edit_link;
  gboolean vcard_changed = FALSE;

  /* Bail out if we don't have outstanding edits to make, or if we already
   * have a set request in progress.
   */
  if (priv->edits == NULL || priv->edit_pipeline_item != NULL)
      return;

  for (edit_link = priv->edits; edit_link != NULL; edit_link = edit_link->next)
    {
      if (vcard_node_changed (priv->connection, vcard_node, edit_link->data))
        {
          vcard_changed = TRUE;
          break;
        }
    }

  if (!vcard_changed)
    {
      DEBUG ("nothing changed, not updating vcard");
      goto out;
    }

I'm still working on this, so I'll take it off the review queue for now.

I think this is ready for review, although it could benefit from some refactoring post-merge.

All I can say is that I really liked the changes. Haven't tested them yet but if it builds, passes tests I am all for it.

So ++

[Adding review+ to whiteboard based on Comment #17]

So, remaining problems I see with this code:

* The SupportedFields property is not connection-specific, so we can't separate it for GTalk. My suggestion would be to have a "filtered" version for less capable connections (GTalk, right now), and a global "unfiltered" version for connections where we think we can set everything.

* It's permissive about incoming vCard fields, but then silently drops anything it doesn't like. My instinct is that in an initial implementation, we should fail loudly (InvalidArgument) on any input we don't like, to encourage initial UIs to actually check SupportedFields. We can relax this later if silently dropping fields turns out to be preferred.

* SupportedFields should list their supported type-parameters, and SupportedFields that can't have any type-parameters at all should have the Parameters_Mandatory flag.

* Minor: it would be nice to detect the Facebook XMPP server, and on connections to that server, clear the SupportedFields and the Can_Set bit.

Another flaw here is that ContactInfoChanged doesn't optimize away no-op changes. Is this OK? I think it probably is; I don't really want to have to implement a vCard-semantic-aware diff.

Removing review+ since there's new code; I've appended patches to fix the major problems and improve test coverage.

(In reply to comment #18)
> * The SupportedFields property is not connection-specific, so we can't separate
> it for GTalk.

Fixed

> * It's permissive about incoming vCard fields, but then silently drops anything
> it doesn't like.

Fixed

> * SupportedFields should list their supported type-parameters, and
> SupportedFields that can't have any type-parameters at all should have the
> Parameters_Mandatory flag.

Fixed

> * Minor: it would be nice to detect the Facebook XMPP server, and on
> connections to that server, clear the SupportedFields and the Can_Set bit.

Not fixed, I'll open a bug for this

(In reply to comment #19)
> Another flaw here is that ContactInfoChanged doesn't optimize away no-op
> changes.

Not fixed, I think this is acceptable

review+ for the branch so far, from Andre on IRC. I'll merge it up. However, I'm leaving this bug open for the further things needed before we undraft this:

* get the branch from Bug #13350 merged

* check the implementation of GetContactInfo and the contact attribute to verify that they don't cause network traffic

* update the spec in Gabble from Bug #13350 and implement RefreshContactInfo

I don't think I'm necessarily the right person to be doing those, but I can review. Tentatively assigning to Andre.

In addition to what I mentioned in Comment #21, when I tried merging the branch so far into master, vcard/overlapping-sets.py started failing (but only during distcheck, not during check, for some reason). I'm trying to focus on other things at the moment, so someone else will have to sort that out.

Updated my branch with latest spec changes (added RefreshContactInfo support and update GetContactInfo, ...). 
Updating branch url

This looks good as an implementation of your current draft. Please merge :-)

Merged upstream. It will be in next release 0.10.2