Bug 30296

The branch is still a wip. Hold your scathing reviews, unless you want to get a head start and see the kind of crack I will be introducing.

The only way to implement this stuff properly is in tp-glib.

Ok, I put into this almost a full week. And it is still far from perfect.
This really needs to end up in tp-glib, any work done in gabble (which is not a trivial delta) will pretty much be thrown away once it goes into tp-glib.

I'm not going to bother rebasing this, unless you think this is something we could actually introduce into master.

Some random thoughts:
* The protocol stuff could go into TpBaseProtocol.
* Static gchar* arrays for interfaces make it extremely unflexible. It worked fine when you just do +1 for removing a group iface, but what about two mutually exclusive interfaces? like group and addressing. There is probably a better way of dealing with this, but my current branch just has random undocumented +1s.
* I hope I understood RCCs. From what I gather, every RCC that had a fixed handle type of contact there will now be two more, one for vcard addresses and one for URIs. This is an extremely large amount of RCCs! Most of the caps related tests are failing now, they need to be more streamlined to support this multiplication. Same goes with all the foreach_channel_class functions, they need to multiply all the 1-on-1 classes. I set up a helper function for that. I think that tp-glib should probably do that automatically when it builds the RCCs (if the cm supports addressing).
* We never talked about how addressing works with MUCs, for now it just doesn't. (ie, a TargetVcardAddress request will assume a TargetHandleType of contact).
* tp-glib does not allow having a handle type without a handle or identifier in a channel request. So for now that means that using the addressing bits for requests requires handle type to be omitted.
* Some 20 odd tests are still failing, they are mostly caps related, and can't deal with the RCCs being so different.

That's it, letting go for now...

This is going to be a pretty immense comment. :) First up, general comments on the design and your points above:

I agree that most of this should live in telepathy-glib, as you say. I like the design here a lot: it makes me happy that the changes to channel (manager) implementations were pretty small.

For channel requests: one other tactic we could use would be to make telepathy-glib's Requests infrastructure transform target addresses/URIs to TargetHandle internally, just like it already transforms TargetID to TargetHandle internally. This would do the job for Gabble at least;]

Regarding static arrays of interfaces: one possible other way to deal with this I noticed in telepathy-ring is to have #defines that expand to a bunch of standard allowed properties/interfaces. Then we could easily define three different arrays of properties rather than having to do these error-prone offset tricks.

The RCC explosion is a bit surprising, but I don't think it's actively bad. The test situation is a pain, though. I saw you changed some assertLength(1, ...)s to assertLength(4, ..)... I'm not sure how best we should deal with this.

‘...using the addressing bits for
requests requires handle type to be omitted.’: i think that's fine. you might not know up-front what handle type the URI corresponds to. You could have an XMPP URI for a MUC. (I don't know if such a beast exists, the internet connection here isn't working.)

===

Here are some nitpicks on the implementation itself:

+ * addressing-mixin.c - Source for GabbleAddressingMixin
+ * Copyright (C) 2006-2008 Collabora Ltd.
+ * Copyright (C) 2006-2008 Nokia Corporation

These copyright dates are not right.

+#define DEBUG_FLAG GABBLE_DEBUG_PROPERTIES

Make a new debug flag?

+#define GABBLE_ADDRESSING_MIXIN_OFFSET(o) \
+  (GPOINTER_TO_UINT (g_type_get_qdata (G_OBJECT_TYPE (o), \
+                                       GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK)))

I think this should probably be GPOINTER_TO_SIZE(), but I assume this is copypasta from somewhere.

+        "gabble_addressing_mixin_get_offset_quark@0.7.7");

I guess the number here is the version of tp-glib in which some other mixin was added there?

+  g_type_set_qdata (G_OBJECT_TYPE (obj),
+                    GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK,
+                    GINT_TO_POINTER (offset));

Shouldn't this go in a mixin_class_init() rather than in an instance init? We don't need to re-annotate the type using this mixin every time an instance is constructed: the mixin's always at the same offset into the struct. Maybe _init_dbus_properties() could become _class_init() and take

+  TpHandleRepoIface *contact_handles =
+    tp_base_connection_get_handles (base_conn, TP_HANDLE_TYPE_CONTACT);
+
+  DEBUG ("%p", obj);
+
+  g_object_unref (mixin->priv->connection);
+
+  if (mixin->priv->target != 0)
+    tp_handle_unref (contact_handles, mixin->priv->target);

I think it's probably not kosher to use the handle repo borrowed from the connection after unreffing the connection.

gabble_addressing_mixin_get_dbus_property should have an else clause that does something useful like g_return_if_reached().

+  if (name == q_target_vcard_field)
+    {
+      g_value_set_static_string (value, vcard_fields[0]);

I guess the idea is that, in a CM that supported more than one vCard field­—like telepathy-sofiasip—you'd pass which field is actually being used to addressing_mixin_init(), rather than just always picking the first one?

+  /* excuse the poor man's URI parsing, couldn't find a GLib helper */
+  gchar **tokenized_uri = g_strsplit(uri, ":", 2);

Yeah, glib has g_uri_parse_scheme(), and I guess you could do something like:

  gchar *scheme = g_uri_parse_scheme (uri);
  const gchar *rest = uri + strlen (scheme) + 1;

which is a bit ghetto too. And then gio/gnetworkaddress.h has some almost-useful functions. And actually glib does secretly have a URI parser in gio/gdummyfile.c which handles queries and fragments and everything, but it's not public. (I found this yesterday because it's actually buggy and breaks clicking on certain perfectly-valid links in Empathy. Sigh.)

+  for (i=0;i<len;i++)
+  for (field=addressable_vcard_fields;*field!=NULL;field++)

Nitpicking: there should be more spaces in these.

gabble_make_addressing_requestable_properties_foreach_channel_class

Wow. This function is pretty magical. I see what it's there for—and it's good that it reduces duplication—but I find it hard to follow. I'm not entirely sure how to make it clearer; I'll have a think.

gabble_addressing_resolve_channel_target() probably wants to return FALSE if neither condition is true?

+++ b/src/conn-addressing.c
@@ -0,0 +1,216 @@
+/*
+ * conn-addressing.h - Header for Gabble connection code handling addressing.

That is no header!

tp_contacts_mixin_get_contact_attributes(): I don't think you should have to pass TP_IFACE_CONNECTION into it. tp-glib should make that assumption for us, surely?

+          if (error->code == TP_ERROR_NOT_IMPLEMENTED)
+            {
+              error->code = TP_ERROR_INVALID_ARGUMENT;

A little distressing that the same condition leads to two different errors in different places.

+    /* If more interfaces are added, either keep Group as the first, or change
+     * the implementations of gabble_tube_stream_get_interfaces () and
+     * gabble_tube_stream_get_property () too */

It looks like you've made the change the comment was asking for, so the comment could be binned.

(In reply to comment #3)
> ‘...using the addressing bits for
> requests requires handle type to be omitted.’: i think that's fine. you might
> not know up-front what handle type the URI corresponds to. You could have an
> XMPP URI for a MUC. (I don't know if such a beast exists, the internet
> connection here isn't working.)

I'd assumed that the scope of Addressing was limited to Handle_Type_Contact, although I suppose if an organization can have a vCard (which it can), then so can a chatroom...

If Addressing *isn't* implicitly Contact-based, then we need to include the handle type in the request (and amend telepathy-glib to cope) - given an XMPP JID or URI, I don't believe we can generally tell whether it's a contact or a MUC (in an ideal world, possibly we could if Google's MUC servers responded to disco queries).

(In reply to comment #3)
> This is going to be a pretty immense comment. :) First up, general comments on
> the design and your points above:
> 
> I agree that most of this should live in telepathy-glib, as you say. I like the
> design here a lot: it makes me happy that the changes to channel (manager)
> implementations were pretty small.
> 

They would be even smaller in the future if more channels derived from TpBaseChannel.

> For channel requests: one other tactic we could use would be to make
> telepathy-glib's Requests infrastructure transform target addresses/URIs to
> TargetHandle internally, just like it already transforms TargetID to
> TargetHandle internally. This would do the job for Gabble at least;]
> 

Yup, that is where it belongs...
Also, TpDynamicHandleRepo should probably be extended to handle addressing, and TpHandleRepoIface should have addressing virtual methods. Brothers and sisters to tp_handle_(lookup/ensure) and tp_handle_inspect.

> Regarding static arrays of interfaces: one possible other way to deal with this
> I noticed in telepathy-ring is to have #defines that expand to a bunch of
> standard allowed properties/interfaces. Then we could easily define three
> different arrays of properties rather than having to do these error-prone
> offset tricks.

I have noticed that you really enjoy preprocessor Jedi mind tricks :)
I'll try to think up a macro, but feel free to come up with something. It's not my strong card.

> 
> The RCC explosion is a bit surprising, but I don't think it's actively bad. The
> test situation is a pain, though. I saw you changed some assertLength(1, ...)s
> to assertLength(4, ..)... I'm not sure how best we should deal with this.
> 
> ‘...using the addressing bits for
> requests requires handle type to be omitted.’: i think that's fine. you might
> not know up-front what handle type the URI corresponds to. You could have an
> XMPP URI for a MUC. (I don't know if such a beast exists, the internet
> connection here isn't working.)

This is the opposite of what Simon suggests. I kind of like the idea of just requesting a channel with a URI without any predetermined conception of it being a user or a group. And then getting the channel and inspecting it's props. That might not work well with current UIs, and that is probably Simon's beef.

> 
> ===
> 
> Here are some nitpicks on the implementation itself:

To follow..

Another idea for tp-glib and RCCs... I think the whole part about TargetID and TargetHandle being mutually exclusive in channel requests needs to be expressed in the RCCs, and not just in the English spec alone.

Currently a typical contact channel RCC would look like:

[chat-1on1]
TargetHandleType=1
...
allowed=TargetID;TargetHandle;...;

This should really be two separate classes:

[chat-1on1-1]
TargetHandleType=1
...
allowed=TargetID;...;

[chat-1on1-2]
TargetHandleType=1
...
allowed=TargetHandle;...;

This needs to be programatically expanded not unlike what I did in gabble_make_addressing_requestable_properties_foreach_channel_class. This could be a more generalized function in tp-glib that takes an RCC with a fixed TargetHandleType property and expands it as necessary. Or it could be in TpBaseConnection's get_requestables_foreach directly. I like the former, since the latter does implicit magic that is out of the CM's control.

(In reply to comment #3)
> Here are some nitpicks on the implementation itself:
> 
> + * addressing-mixin.c - Source for GabbleAddressingMixin
> + * Copyright (C) 2006-2008 Collabora Ltd.
> + * Copyright (C) 2006-2008 Nokia Corporation
> 
> These copyright dates are not right.

d9c0f21

> 
> +#define DEBUG_FLAG GABBLE_DEBUG_PROPERTIES
> 
> Make a new debug flag?

75ed12f

> 
> +#define GABBLE_ADDRESSING_MIXIN_OFFSET(o) \
> +  (GPOINTER_TO_UINT (g_type_get_qdata (G_OBJECT_TYPE (o), \
> +                                       GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK)))
> 
> I think this should probably be GPOINTER_TO_SIZE(), but I assume this is
> copypasta from somewhere.
> 

busted! 77e76e9

> +        "gabble_addressing_mixin_get_offset_quark@0.7.7");
> 
> I guess the number here is the version of tp-glib in which some other mixin was
> added there?
> 

busted again! 77e76e9

> +  g_type_set_qdata (G_OBJECT_TYPE (obj),
> +                    GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK,
> +                    GINT_TO_POINTER (offset));
> 
> Shouldn't this go in a mixin_class_init() rather than in an instance init? We
> don't need to re-annotate the type using this mixin every time an instance is
> constructed: the mixin's always at the same offset into the struct. Maybe
> _init_dbus_properties() could become _class_init() and take
> 

Seems like most tp-glib mixins have this in the constructor. Uh oh, busted! again! 77e76e9

> +  TpHandleRepoIface *contact_handles =
> +    tp_base_connection_get_handles (base_conn, TP_HANDLE_TYPE_CONTACT);
> +
> +  DEBUG ("%p", obj);
> +
> +  g_object_unref (mixin->priv->connection);
> +
> +  if (mixin->priv->target != 0)
> +    tp_handle_unref (contact_handles, mixin->priv->target);
> 
> I think it's probably not kosher to use the handle repo borrowed from the
> connection after unreffing the connection.
> 

Good catch 267a11b

> gabble_addressing_mixin_get_dbus_property should have an else clause that does
> something useful like g_return_if_reached().

0199ce5

> 
> +  if (name == q_target_vcard_field)
> +    {
> +      g_value_set_static_string (value, vcard_fields[0]);
> 
> I guess the idea is that, in a CM that supported more than one vCard
> field­—like telepathy-sofiasip—you'd pass which field is actually being used to
> addressing_mixin_init(), rather than just always picking the first one?
> 

Yeah, this will need to be redone.

> +  /* excuse the poor man's URI parsing, couldn't find a GLib helper */
> +  gchar **tokenized_uri = g_strsplit(uri, ":", 2);
> 
> Yeah, glib has g_uri_parse_scheme(), and I guess you could do something like:
> 
>   gchar *scheme = g_uri_parse_scheme (uri);
>   const gchar *rest = uri + strlen (scheme) + 1;

fd2eb00

> 
> which is a bit ghetto too. And then gio/gnetworkaddress.h has some
> almost-useful functions. And actually glib does secretly have a URI parser in
> gio/gdummyfile.c which handles queries and fragments and everything, but it's
> not public. (I found this yesterday because it's actually buggy and breaks
> clicking on certain perfectly-valid links in Empathy. Sigh.)

Yeah, the stuff in gdummyfile.c would actually be useful.

> 
> +  for (i=0;i<len;i++)
> +  for (field=addressable_vcard_fields;*field!=NULL;field++)
> 
> Nitpicking: there should be more spaces in these.

0316d17

> 
> gabble_make_addressing_requestable_properties_foreach_channel_class
> 
> Wow. This function is pretty magical. I see what it's there for—and it's good
> that it reduces duplication—but I find it hard to follow. I'm not entirely sure
> how to make it clearer; I'll have a think.

Yeah, I could probably add some comments there.

> 
> gabble_addressing_resolve_channel_target() probably wants to return FALSE if
> neither condition is true?
> 

You would think. The function that this functions serves is *_requestotron, which differentiates between returning FALSE and an error. It's confusing, and I would like to see this function go away, that is why tp-glib should translate addressing requests like TargetID.

> +++ b/src/conn-addressing.c
> @@ -0,0 +1,216 @@
> +/*
> + * conn-addressing.h - Header for Gabble connection code handling addressing.
> 
> That is no header!

c8ff335

> 
> tp_contacts_mixin_get_contact_attributes(): I don't think you should have to
> pass TP_IFACE_CONNECTION into it. tp-glib should make that assumption for us,
> surely?

This is for bug 30310.

> 
> +          if (error->code == TP_ERROR_NOT_IMPLEMENTED)
> +            {
> +              error->code = TP_ERROR_INVALID_ARGUMENT;
> 
> A little distressing that the same condition leads to two different errors in
> different places.

I agree. Alternatives? The issue is that one consumer of the error is Create/Ensure where NotImplemented is an appropriate error. And the other consumer is Conn.I.Addressing.GetContactsByVCardField, where NotImplemented would be very confusing. The UI might assume that the CM has a partial Addressing implementation.

> 
> +    /* If more interfaces are added, either keep Group as the first, or change
> +     * the implementations of gabble_tube_stream_get_interfaces () and
> +     * gabble_tube_stream_get_property () too */
> 
> It looks like you've made the change the comment was asking for, so the comment
> could be binned.

77df07f

(In reply to comment #3)
> +  g_type_set_qdata (G_OBJECT_TYPE (obj),
> +                    GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK,
> +                    GINT_TO_POINTER (offset));
> 
> Shouldn't this go in a mixin_class_init() rather than in an instance init? We
> don't need to re-annotate the type using this mixin every time an instance is
> constructed: the mixin's always at the same offset into the struct. Maybe
> _init_dbus_properties() could become _class_init() and take
> 

Ok, so if you were wondering why most mixin classes we have do this in the constructor, and not in the class_init, it's because of base classes.
If we mix this into a base class, and pass the base's struct offset, the derived class has no direct access to it.
For example,
In gabble_base_call_channel_class_init we provide gabble_addressing_mixin_class_init with an offset in GabbleBaseCallChannel.
But when gabble_base_call_channel_constructed is called, it is a derived object, so the gtype would be GabbleCallChannel. The only way I see getting this to work is either moving all of it to the derived classes, or just the class_init part. None of those options look good to me. I think I will revert this back to annotating the type in the constructor.

(In reply to comment #6)
> Another idea for tp-glib and RCCs... I think the whole part about TargetID and
> TargetHandle being mutually exclusive in channel requests needs to be expressed
> in the RCCs, and not just in the English spec alone.

I don't think this is really a good idea; this would double the number of RCCs, and for what benefit? (What would a client do differently?)

IMO, it'd be OK if Addressing was something of a second-class citizen here - Target{Handle,ID} are fundamental to Telepathy and Addressing is not. (That's why Addressing isn't part of the core Channel interface...)

First having a go at Protocol.Interface.Addressing alone (bug 32692).
Much more straightforward and would provide much benefit.

Ok, I used this work in order to implement bug #41789. 

I've rebased, updated and split Eitan's changes in 2 branches, one for Conn.I.Addressing (working with tests) and another one for Chan.I.Addressing (which is still WIP and some tests will fail, but compiles)

I chose to split it as I believe most of the work done to support Chan.I.Addressing should go to tp-glib, while the work to support Conn.I.Addressing can live just fine in tp-gabble. Also the main reason was to support bug #41789, which does not require Chan.I.Addressing. 

So I would like to suggest that we split this bug in 2, one to support Conn.I.Addressing and another one for Chan.I.Addressing and we first push Proto.Addressing (bug #32692 - needs undraft) and Conn.I.Addressing if needed, so we don't block in Chan.I.Addressing being undrafted. 

The changes can be found on the branches conn-addressing (ready for review) and chan-addressing-wip at my personal repo found at URL.

Changing bug to be specific to Conn.I.Addressing, Channel.I.Addressing support is now bug #42918. 

URL updated to point to the correct repo implementing Conn.I.Addressing.

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@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?

(In reply to comment #13)
> > +  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".

With my suggested change to the D-Bus API over on Bug #26866, you won't need to do this anyway. \o/

(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@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

I now realise that gabble_parse_uri() doesn't perform RFC 3986 (URI generic syntax) processing. If a JID is "wtf?@example.com", then its correct URI form is "xmpp:wtf%63@example.com" - you probably want to use g_uri_escape_string() and g_uri_unescape_string() for this.

(This is really more of a bug in the Protocol layer of URI support than the Connection layer, though.)

I feel as though gabble_parse_uri() is the wrong function name: it's really gabble_normalize_uri(). It'd maybe make more sense to split it into gabble_uri_to_jid() and gabble_jid_to_uri()?

gabble_uri_for_handle() doesn't do URI escaping either, and should probably delegate to gabble_jid_to_uri() for that.

I'm not sure whether the "jid" side of the JID <-> URI functions should be a JID, or a (node or NULL, hostname, resource or NULL) tuple. Probably the latter, using wocky_decode_jid() (or gabble_decode_jid(), but that's just a thin wrapper around the Wocky equivalent) - you can use gabble_encode_jid() to go the other way.

Perhaps functions to map between URIs and JIDs, or URIs and JID component tuples, should eventually move into Wocky anyway? Probably out-of-scope for this branch, though.

The vCard cases look fine now.

(In reply to comment #16)
> I now realise that gabble_parse_uri() doesn't perform RFC 3986 (URI generic
> syntax) processing. If a JID is "wtf?@example.com", then its correct URI form
> is "xmpp:wtf%63@example.com" - you probably want to use g_uri_escape_string()
> and g_uri_unescape_string() for this.
The implementation now does NormalizeURI("wtf?@example.com") -> "xmpp:wtf%3F@example.com" and the contact attribute ".../uris" also contains the escaped versions.
I am escaping the jid components separately (escape(domain), escape(...)), as escaping the whole jid leads to "@" being escaped, but I am not sure if the domain and resource should be escaped also.
Please let me know if this is what you meant.

> 
> I feel as though gabble_parse_uri() is the wrong function name: it's really
> gabble_normalize_uri(). It'd maybe make more sense to split it into
> gabble_uri_to_jid() and gabble_jid_to_uri()?
> 
> gabble_uri_for_handle() doesn't do URI escaping either, and should probably
> delegate to gabble_jid_to_uri() for that.
Done. Split in 3 methods, normalize_uri, uri_to_jid and jid_to_uri and updated gabble_uri_for_handle accordingly.

normalize_uri will always return the uri escaped as described above, as well as jid_to_uri. 
uri_to_jid always return a normalized jid, with the uri unescaped.

> I'm not sure whether the "jid" side of the JID <-> URI functions should be a
> JID, or a (node or NULL, hostname, resource or NULL) tuple. Probably the
> latter, using wocky_decode_jid() (or gabble_decode_jid(), but that's just a
> thin wrapper around the Wocky equivalent) - you can use gabble_encode_jid() to
> go the other way.
If you are talking about the method param, I kept "jid" there as it makes the code simpler. I am using gabble_decode/encode_jid internally though.

(In reply to comment #17)
> > then its correct URI form is "xmpp:wtf%63@example.com"
> "xmpp:wtf%3F@example.com"

To clarify: you're right, I was misreading ascii(7). (63 = 0x3F)

> I am escaping the jid components separately (escape(domain), escape(...)), as
> escaping the whole jid leads to "@" being escaped, but I am not sure if the
> domain and resource should be escaped also.

Yes they should. I don't think it can actually matter for a correct domain, but the resource can contain all sorts of strange things (spaces!), and what you're doing here is the maximally-correct thing.

> + <p>A mapping from requested vCard addresses and the corresponding

from requested vCard addresses *to* the corresponding

> + g_hash_table_insert (requested, (gpointer) g_strdup (*uri),
> GUINT_TO_POINTER (h));

The (gpointer) cast is unnecessary now - we were using it as the C equivalent of const_cast<>, and now that you g_strdup the keys, they aren't const anyway.

> +gabble_uri_to_jid (const gchar *uri,

This can't tell the difference between component boundaries and escaped component boundaries in the URI: if you feed it certain invalid URIs like xmpp:invalid%40domain.example.com, it will re-interpret the escaped @ as an unescaped @, and consider it to be invalid@example.com, rather than noticing that "invalid@domain" isn't a valid hostname.

I'm not sure that this is really a practical problem, though. Perhaps clone it as a bug and get on with your life?

To fix this we'd need to have a function like:

gboolean gabble_parse_xmpp_uri (const gchar *uri,
                                gchar **node_or_null,
                                gchar **domain,
                                gchar **resource_or_null)

which split at @ and / *first*, then unescaped each part individually, then finally checked each part for validity. Given that (iirc) Wocky doesn't yet do proper nodeprep/resourceprep to validate/normalize nodes and resources in JIDs according to the RFCs, I think this is a job for another bug.

> +gabble_jid_to_uri (const gchar *scheme,
[...]
> + uri = g_strdup_printf ("%s:%s", scheme, jid);
> +
> + normalized_uri = gabble_normalize_uri (uri, error);

This doesn't look right: you're relying on gabble_normalize_uri accepting insufficiently-escaped JIDs. Which it does, at the moment, because it starts with a call to gabble_uri_to_jid, which is buggy as described above... but still.

I think you should move most of the body of gabble_normalize_uri into this function, so that this function starts at the gabble_decode_jid call; then gabble_normalize_uri can just be

    gabble_jid_to_uri (gabble_uri_to_jid (uri))

(except with memory management and error handling).

I'd like to see some non-URI-allowed characters (using "?" again would do nicely) in the regression test's resource, to check that that works.

Branch updated with latest suggestions.

+ escaped_node = g_uri_escape_string (node, NULL, TRUE);
+ if (escaped_node == NULL)
+ {
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "'%s' is not a valid JID", jid);

How could this possibly fail, given that node is already non-NULL? Likewise for the other two g_uri_escape_string calls.

+ if (!gabble_decode_jid (jid, &tmp_node, &tmp_domain, &tmp_resource))
+ {
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "'%s' is not a valid XMPP URI", uri);
+ goto OUT;
+ }

You probably want wocky_decode_jid instead of gabble_decode_jid (it's a trivial wrapper).

This is relying on the fact that wocky_decode_jid will allow "%" in all three components, I think? Not all valid escaped-JIDs are valid JIDs. I think it'd be enough to open this as a bug and move on, though.

I can't help feeling that gabble_parse_xmpp_uri() should validate the node, domain and resource individually - at the moment it does a *lot* of JID -> tuple -> JID -> tuple round-trips - but to do that properly, we'd need Wocky to expose wocky_validate_jid_node(), wocky_validate_jid_domain(), and a new wocky_validate_jid_domain() (at the moment the first two are static and the third is just "is a non-empty string").

(In reply to comment #20)
> + escaped_node = g_uri_escape_string (node, NULL, TRUE);
> + if (escaped_node == NULL)
> + {
> + g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
> + "'%s' is not a valid JID", jid);
> 
> How could this possibly fail, given that node is already non-NULL? Likewise for
> the other two g_uri_escape_string calls.
Done

> + if (!gabble_decode_jid (jid, &tmp_node, &tmp_domain, &tmp_resource))
> + {
> + g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
> + "'%s' is not a valid XMPP URI", uri);
> + goto OUT;
> + }
> 
> You probably want wocky_decode_jid instead of gabble_decode_jid (it's a trivial
> wrapper).
Done. Will also setup a trivia branch on top of master removing gabble_decode_jid.
 
> This is relying on the fact that wocky_decode_jid will allow "%" in all three
> components, I think? Not all valid escaped-JIDs are valid JIDs. I think it'd be
> enough to open this as a bug and move on, though.
> 
> I can't help feeling that gabble_parse_xmpp_uri() should validate the node,
> domain and resource individually - at the moment it does a *lot* of JID ->
> tuple -> JID -> tuple round-trips - but to do that properly, we'd need Wocky to
> expose wocky_validate_jid_node(), wocky_validate_jid_domain(), and a new
> wocky_validate_jid_domain() (at the moment the first two are static and the
> third is just "is a non-empty string").
Indeed. Let's open a bug for this. Want some comments in the code?

(In reply to comment #21)
> > I can't help feeling that gabble_parse_xmpp_uri() should validate the node,
> > domain and resource individually
> Indeed. Let's open a bug for this. Want some comments in the code?

Yes please, with the bug number.

> + normalized_uri = proto.Addressing.NormalizeURI(
> + "xmpp:EITAN?@example.COM/resourc?e")
> +
> + assertEquals("xmpp:eitan%3F@example.com", normalized_uri)

I'm not really sure whether this should normalize to this, or to "xmpp:eitan%3F@example.com/resourc%3Fe". Ignoring the resource is certainly good enough for now, but the resource is sometimes significant (for chatroom members, mainly).

It comes down to what we want "NormalizeURI" to mean, I think. Possibilities:

1) here is a contact, please discard unnecessary cruft and reduce it to the
   minimal information necessary to identify this contact

   (this should discard the resource, a query-string if any, etc.)

2) normalize this URI without changing its meaning

   (this should not discard the resource or query-string)

I wonder whether the method should have been called NormalizeContactURI if what we want is (1)?

Oh, another bug to be filed for later: normalizing verb-like URIs (xmpp:smcv@example.com?message;body=Hello%20World) doesn't currently do anything sensible. At a minimum, we should truncate at "?" (assuming interpretation 1).

(In reply to comment #22)
> (In reply to comment #21)
> > > I can't help feeling that gabble_parse_xmpp_uri() should validate the node,
> > > domain and resource individually
> > Indeed. Let's open a bug for this. Want some comments in the code?
> 
> Yes please, with the bug number.
bug #43140

> > + normalized_uri = proto.Addressing.NormalizeURI(
> > + "xmpp:EITAN?@example.COM/resourc?e")
> > +
> > + assertEquals("xmpp:eitan%3F@example.com", normalized_uri)
> 
> I'm not really sure whether this should normalize to this, or to
> "xmpp:eitan%3F@example.com/resourc%3Fe". Ignoring the resource is certainly
> good enough for now, but the resource is sometimes significant (for chatroom
> members, mainly).
> 
> It comes down to what we want "NormalizeURI" to mean, I think. Possibilities:
> 
> 1) here is a contact, please discard unnecessary cruft and reduce it to the
>    minimal information necessary to identify this contact
> 
>    (this should discard the resource, a query-string if any, etc.)
> 
> 2) normalize this URI without changing its meaning
> 
>    (this should not discard the resource or query-string)
> 
> I wonder whether the method should have been called NormalizeContactURI if what
> we want is (1)?
We opted for renaming NormalizeURI to NormalizeContactURI and implement #1.

> Oh, another bug to be filed for later: normalizing verb-like URIs
> (xmpp:smcv@example.com?message;body=Hello%20World) doesn't currently do
> anything sensible. At a minimum, we should truncate at "?" (assuming
> interpretation 1).
And that would be bug #43142

Branch updated with latest changes in Proto.I.Addr support. It will be in the
next gabble release.

Released in 0.15.1.