[Bug 20831] ContactInfo: implement and undraft

[bugzilla-daemon at freedesktop.org]

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





--- Comment #7 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-12-10 09:11:35 PST ---
(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.
:/


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