Once it's undrafted and supported in tp-glib, of course.
+ 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
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.