Bug 32692 - Support Protocol.Interface.Addressing in Gabble
Summary: Support Protocol.Interface.Addressing in Gabble
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Andre Moreira Magalhaes
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/an...
Whiteboard: review+ after dependencies released
Keywords: patch
Depends on: 32691
Blocks: 30296 41789 42918
  Show dependency treegraph
 
Reported: 2010-12-27 16:26 UTC by Eitan Isaacson
Modified: 2011-11-24 07:09 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Eitan Isaacson 2010-12-27 16:26:11 UTC
Once it's undrafted and supported in tp-glib, of course.
Comment 1 Simon McVittie 2011-01-03 06:45:42 UTC
+  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.
Comment 2 Eitan Isaacson 2011-01-03 16:26:02 UTC
OK,
pushed changes from your review and API adjustments to new proposed tp-glib API.
Comment 3 Andre Moreira Magalhaes 2011-11-13 17:56:04 UTC
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.
Comment 4 Simon McVittie 2011-11-14 05:33:15 UTC
> +  { "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.
Comment 5 Andre Moreira Magalhaes 2011-11-14 09:11:12 UTC
(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
Comment 6 Simon McVittie 2011-11-14 09:21:03 UTC
r+ when a suitable tp-glib has been released.
Comment 7 Andre Moreira Magalhaes 2011-11-21 07:30:28 UTC
Updated branch with latest tp-glib changes (Rename normalize_uri to normalize_contact_uri, etc). It will be in the next gabble release.
Comment 8 Will Thompson 2011-11-24 07:09:35 UTC
Merged to master for 0.15.1


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.