[Bug 20831] ContactInfo: implement and undraft
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Tue Dec 1 00:37:07 CET 2009
http://bugs.freedesktop.org/show_bug.cgi?id=20831
--- Comment #3 from Andre Moreira Magalhaes <andrunko at gmail.com> 2009-11-30 15:37:06 PST ---
(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
--
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
More information about the telepathy-bugs
mailing list