Bug 32691

(This can't be merged until the undrafted spec has landed and been released.)

I'm not really sure that Addressing should be in the TpBaseProtocol vtable: we're running out of slots, and Addressing doesn't seem particularly fundamental. Maybe it should be a GInterface (see how TpBaseContactList does its optional functionality, for instance). I could be persuaded either way.

> + * TpBaseProtocolGetAddressingDetailsFunc:

...Dup... rather than Get, really.

> + * Since: 0.13.11

No such version yet, so who can say what'll be in it? 0.13.UNRELEASED, please.

> TpBaseProtocolAddressingNormalizeVCardAddressFunc

I don't think the word Addressing adds value here.

>+ * TpBaseProtocolAddressingNormalizeUriFunc:

Likewise, and I'd also prefer the acronym capitalized: TpBaseProtocolNormalizeURIFunc.

> +    G_IMPLEMENT_INTERFACE (TP_TYPE_SVC_PROTOCOL_INTERFACE_ADDRESSING,
> +      addressing_iface_init));

That trailing semicolon isn't needed, and causes an empty statement to appear, which breaks some compilers.

> +  if (tp_strv_contains ((const gchar * const*) self->priv->interfaces,

const * (with a space)

(In reply to comment #1)
> (This can't be merged until the undrafted spec has landed and been released.)
> 
> I'm not really sure that Addressing should be in the TpBaseProtocol vtable:
> we're running out of slots, and Addressing doesn't seem particularly
> fundamental. Maybe it should be a GInterface (see how TpBaseContactList does
> its optional functionality, for instance). I could be persuaded either way.

That would be a good use of GInterface, I was having a hard time taking up two more precious slots. Plus, I don't like the get_details function, I think they should each be a separate one, but I was conserving slots. Anyway, the HEAD has this implemented as a GInterface along with all the other review bits you gave.

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.

Patches with substantial changes from you shouldn't really be --author=Eitan any more, IMO. Attribute such patches to yourself, with "Based on a patch from Eitan Isaacson <...>" in the commit message?

> Undraft Protocol.Interface.Addressing.

Not really valid, but will go away when this branch is rebased onto a version of tp-glib with a version of tp-spec where P.I.A is undrafted.

> Make Protocol.Interface.Addressing functionality a GInterface.

I'd have vaguely preferred this one squashed into the previous commits - you'd have to split it, since half of it is squashed into "add the feature" and the other half into "... and add a test".

Review in detail in a moment (I'll review a squashed version rather than using gitweb).

reassigning to Andre

The implementation looks fine; some naming/documentation/g-i issues.

+/**
+ * TpBaseProtocolGetSupportedVCardFields:

Needs a Returns: line. Did you build this branch with gtk-doc? Did it tell you you were missing this?

The Returns: line gives you a chance to say "a list of vCard fields in lower case, e.g. [x-sip, tel]" or whatever - SIP makes a great example for this sort of thing, because it typically supports both SIP and telephony, and its vCard field and URI scheme don't have the same name.

It's actually Returns: (allow-none) (out) (transfer full): ... in order to have the g-i annotations right, I think.

Neither you nor Eitan fixed this:

> > + * TpBaseProtocolGetAddressingDetailsFunc:
>
> ...Dup... rather than Get, really.

Also, I think a typedef for a function pointer should generally say ...Func at the end (this one did in Eitan's initial branch, not sure what happened to that).

+/**
+ * TpBaseProtocolGetSupportedURISchemes:

All comments about SupportedVCardFields apply here. Again, consider using SIP as your example, to have [sip, sips, tel].

(In reply to comment #6)
> The implementation looks fine; some naming/documentation/g-i issues.
> 
> +/**
> + * TpBaseProtocolGetSupportedVCardFields:
> 
> Needs a Returns: line. Did you build this branch with gtk-doc? Did it tell you
> you were missing this?
> 
> The Returns: line gives you a chance to say "a list of vCard fields in lower
> case, e.g. [x-sip, tel]" or whatever - SIP makes a great example for this sort
> of thing, because it typically supports both SIP and telephony, and its vCard
> field and URI scheme don't have the same name.
> 
> It's actually Returns: (allow-none) (out) (transfer full): ... in order to have
> the g-i annotations right, I think.
> 
> Neither you nor Eitan fixed this:
Done, used the same annotation above.

> > > + * TpBaseProtocolGetAddressingDetailsFunc:
> >
> > ...Dup... rather than Get, really.
> 
> Also, I think a typedef for a function pointer should generally say ...Func at
> the end (this one did in Eitan's initial branch, not sure what happened to
> that).
Actually this has changed now and does not exist anymore. Updated telepathy-glib-sections.txt accordingly.

> +/**
> + * TpBaseProtocolGetSupportedURISchemes:
> 
> All comments about SupportedVCardFields apply here. Again, consider using SIP
> as your example, to have [sip, sips, tel].
Done

Also fixed a bug in the protocol-objects test.

(In reply to comment #7)
> > > > + * TpBaseProtocolGetAddressingDetailsFunc:
> > >
> > > ...Dup... rather than Get, really.

No longer exists, but TpBaseProtocolGetSupportedVCardFields and TpBaseProtocolGetSupportedURISchemes have the same issue...

> > Also, I think a typedef for a function pointer should generally say ...Func at
> > the end

... and also this one.

(In reply to comment #8)
> (In reply to comment #7)
> > > > > + * TpBaseProtocolGetAddressingDetailsFunc:
> > > >
> > > > ...Dup... rather than Get, really.
> 
> No longer exists, but TpBaseProtocolGetSupportedVCardFields and
> TpBaseProtocolGetSupportedURISchemes have the same issue...
True, done.

> > > Also, I think a typedef for a function pointer should generally say ...Func at
> > > the end
> 
> ... and also this one.
Done.

r+ after the next spec release comes out.

Updated branch with latest changes in Proto.I.Addr (rename NormalizeURI to NormalizeContactURI). It will be in the next tp-glib release.

Released in 0.17.2