[Bug 20831] ContactInfo: implement and undraft

[bugzilla-daemon at freedesktop.org]

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





--- Comment #2 from Will Thompson <will.thompson at collabora.co.uk>  2009-11-27 07:14:19 PST ---
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.


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