[Bug 30296] Add Conn.I.Addressing support to Gabble

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=30296

--- Comment #13 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-11-14 06:13:18 PST ---
Looking at the connection branch only, for now.

addressing-util.c
> +#include "debug.h"
> +#include "util.h"
> +#include "connection.h"

Nitpicking: I'd prefer alphabetical order.

> +gabble_parse_uri (const gchar *uri,
> +gabble_parse_vcard_address (const gchar *vcard_field,

These whole functions should be replaced with the versions from the
Proto.I.Addr branch (after you've fixed those to be in the right order). Some
of the code in these is wrong, but I'm not going to enumerate the problems in
detail, because the Proto.I.Addr version was better anyway.

> +gchar **
> +gabble_uris_for_handle (TpHandleRepoIface *contact_repo,
> +    TpHandle contact)
> +{
> +  guint len = g_strv_length ((gchar **) addressable_uri_schemes);
> +  guint i;
> +  gchar **uris = g_new0 (gchar *, len + 1);
> +
> +  for (i = 0; i < len; i++)
> +    uris[i] = gabble_uri_for_handle (contact_repo, addressable_uri_schemes[i], contact);
> +
> +  return uris;
> +}

This goes wrong if you support two URI schemes, the URI for the first scheme
comes out NULL, and the URI for the second scheme doesn't.

This can't yet happen, because we only support one scheme and everyone has it -
but now that Skype and Windows Live (the former MSN) have XMPP interop, we'll
probably want to add "skype:example" and/or
"msnim:add?contact=example at hotmail.com" to the address lists of contacts
bridged from those services.

Use this pseudocode, which illustrates the common "use a GPtrArray to build a
GStrv" pattern:

    arr = g_ptr_array_new ();

    foreach (scheme)
      {
        tmp = gabble_uri_for_handle (contact_repo, scheme, contact);

        if (tmp != NULL)
          g_ptr_array_add (arr, tmp);
      }

    g_ptr_array_add (arr, NULL);
    return g_ptr_array_free (arr, FALSE);

Similarly, in the vCard version I think you should avoid inserting into the
hash table if the value is going to be NULL. This could even crash dbus-glib,
if you use it as an a{ss} directly (which you do).

The implementations of gabble_vcard_address_for_handle and
gabble_uri_for_handle are pretty simplistic - you're going to have to enhance
them as soon as we support other addressing/URI schemes. I suggest
pre-emptively turning them into the same "if (x-jabber) {...} else if
(x-facebook-id) {...} else { error }" structure that I recommended for the
Proto.I.Addr branch.

conn-addressing:
> +    const gchar **in_URIs,
> +    const gchar **in_Interfaces,

Nitpicking: you're meant to rename these to something nice rather than copying
the names from the codegen (which prioritize non-collision over readability).
"uris", "interfaces" would be good.

> +  contacts = g_hash_table_get_keys (result);

This deserves a comment: "copy the keys, because we'll be modifying the hash
table while we iterate over them".

Same comments apply to conn_addressing_get_contacts_by_vcard_field, and also:

> +          if (error->code == TP_ERROR_NOT_IMPLEMENTED)

Also check that the domain is TP_ERROR, please; otherwise this logic is
meaningless. g_error_matches() is useful for this.

protocol:

+  if (jid == NULL)
     {
-      g_set_error (error, TP_ERRORS, TP_ERROR_NOT_IMPLEMENTED,
-          "'x-jabber' is the only vCard field supported by this protocol");
+      /* InvalidHandle makes no sense in Protocol */
+      if ((*error)->code == TP_ERROR_INVALID_HANDLE)
+        (*error)->code = TP_ERROR_INVALID_ARGUMENT;
     }

If error is NULL, you'll crash. Don't. Also, check the domain, as above. (Also
applies to normalizing addresses.)

   else
     {
+      g_assert (jid != NULL);
     }

Er... the "else" clause of "if (jid == NULL)" doesn't need this assertion :-P
(Also applies to normalizing addresses.)

tubestestutil:
> -        assert len(channel_props['Interfaces']) == 0, channel_props['Interfaces']
> +        assert channel_props['Interfaces'] == [cs.CHANNEL_IFACE_ADDRESSING], \
> +            channel_props['Interfaces']
...
> -            dbus.Array([cs.CHANNEL_IFACE_TUBE], signature='s'), \
> -            channel_props['Interfaces']
> +            dbus.Array([cs.CHANNEL_IFACE_ADDRESSING, cs.CHANNEL_IFACE_TUBE],
> +                       signature='s'), channel_props['Interfaces']

This looks wrong for the connection branch. Move it to the channel branch.
Likewise offer-private-dbus-tube.py, outgoing-basics.py,
initial-audio-video.py, file_transfer_helper.py. Do these tests even pass?

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