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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #15 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-11-16 07:34:44 PST ---
(In reply to comment #13)
> 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.
Done

> > +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.
Done

> > +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);
Done

> 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).
Done

> 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.
Done

> 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.
Done

> > +  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:
Done

> > +          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.
Done

> 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.)
Done

>    else
>      {
> +      g_assert (jid != NULL);
>      }
> 
> Er... the "else" clause of "if (jid == NULL)" doesn't need this assertion :-P
> (Also applies to normalizing addresses.)
heh, done

> 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?
Forgot to remove this, moved to chan-addressing-wip only

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