Bug 32692

+  if (!valid_field_or_scheme (vcard_field, addressing_vcard_fields))
+    {
+      g_set_error (error, TP_ERRORS, TP_ERROR_NOT_IMPLEMENTED,
+          "'%s' vCard field is not supported by this protocol", vcard_field);
+    }

This seems speculatively general, for a protocol that only supports one vCard field.

It's also the wrong shape for protocols that support many vCard fields via gatewaying: in such a protocol, the normalization will probably be different for different fields.

I'd tend to do:

if (it's "xmpp")
  {
    normalize it for XMPP
  }
else
  {
    fail
  }

with an obvious place to put an "else if" clause for hypothetical other fields.

+  /* excuse the poor man's URI parsing, couldn't find a GLib helper */
+  gchar *scheme = g_uri_parse_scheme (uri);

Er, you found one? :-)

This does look good enough to not block undraft (Bug #32690), though.

OK,
pushed changes from your review and API adjustments to new proposed tp-glib API.

Ok, I used this work in order to implement bug #41789. I rebased/fixed conflicts Eitan's changes on top of current upstream master and the changes can be found at URL.

> +  { "protocol",       GABBLE_DEBUG_PROTOCOL },
...
> +  GABBLE_DEBUG_PROTOCOL      = 1 << 28,
...
> +#define DEBUG_FLAG GABBLE_DEBUG_PROTOCOL
...
> +#include "debug.h"

I don't see any calls to DEBUG(), so I don't think you need any of this?

(I think it's fine that there's no debug-spam - Protocol is simple enough not to need any debug-spam.)

> addressing_normalize_vcard_address

I think this is in the wrong order: when you add x-facebook-id you're going to have to re-order it. As I said to Eitan before, I think it should be (pseudocode):

    if (it's x-jabber)
      {
        do Jabber things
      }
/* later, you will add:
    else if (it's x-facebook-id)
      {
        do Facebook things
      }
 */
    else
      {
        error!
      }

Likewise for the URI, except that the scheme == NULL case comes first, so "xmpp" is also an else-if.

> +      g_object_get (G_OBJECT (protocol), "immutable-properties", &props, NULL);

Nitpicking: we like one property per line, to emphasize the paired arguments:

    g_object_get (G_O (p),
        "i-p", &props,
        NULL);

> +PROTOCOL_IFACE_ADDRESSING = \
> +   'org.freedesktop.Telepathy.Protocol.Interface.Addressing'

Nitpicking: could be PROTOCOL + '.Interface.Addressing', which would probably make it fit on one line.

(In reply to comment #4)
> > +  { "protocol",       GABBLE_DEBUG_PROTOCOL },
> ...
> > +  GABBLE_DEBUG_PROTOCOL      = 1 << 28,
> ...
> > +#define DEBUG_FLAG GABBLE_DEBUG_PROTOCOL
> ...
> > +#include "debug.h"
> 
> I don't see any calls to DEBUG(), so I don't think you need any of this?
> 
> (I think it's fine that there's no debug-spam - Protocol is simple enough not
> to need any debug-spam.)
Done

> > addressing_normalize_vcard_address
> 
> I think this is in the wrong order: when you add x-facebook-id you're going to
> have to re-order it. As I said to Eitan before, I think it should be
> (pseudocode):
> 
>     if (it's x-jabber)
>       {
>         do Jabber things
>       }
> /* later, you will add:
>     else if (it's x-facebook-id)
>       {
>         do Facebook things
>       }
>  */
>     else
>       {
>         error!
>       }
> 
> Likewise for the URI, except that the scheme == NULL case comes first, so
> "xmpp" is also an else-if.
Done

> > +      g_object_get (G_OBJECT (protocol), "immutable-properties", &props, NULL);
> 
> Nitpicking: we like one property per line, to emphasize the paired arguments:
> 
>     g_object_get (G_O (p),
>         "i-p", &props,
>         NULL);
Done
 
> > +PROTOCOL_IFACE_ADDRESSING = \
> > +   'org.freedesktop.Telepathy.Protocol.Interface.Addressing'
> 
> Nitpicking: could be PROTOCOL + '.Interface.Addressing', which would probably
> make it fit on one line.
Done

r+ when a suitable tp-glib has been released.

Updated branch with latest tp-glib changes (Rename normalize_uri to normalize_contact_uri, etc). It will be in the next gabble release.

Merged to master for 0.15.1