[Bug 20831] ContactInfo: implement and undraft

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=20831


Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|will.thompson at collabora.co.u|andrunko at gmail.com
                   |k                           |




--- Comment #5 from Will Thompson <will.thompson at collabora.co.uk>  2009-12-06 09:23:48 PST ---
          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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.