Bug 23492

\o/

Review not finished, but so far I have some comments:

src/capabilities.c
static TpHandleRepoIface *feature_handles = NULL;
Add a comment to say the content of this pool is not TpHandle in the meaning of the Tp Spec, but we just reuse the ref-counted string pool implementation which is called TpHandleRepoIface.  

      DEBUG ("not emitting signal: not connected yet");
Do not confuse with dbus signal: s/signal/presence stanza/

gabble_caps_channel_manager_represent_client(): Why is it called like this? Why "represent"?

"<ContactCapabilities.DRAFT1>"
Add a comment to say this string is choosen to not conflict with a possible well-known telepathy client name

In _emit_capabilities_changed():
The comments /* Capabilities */, etc.: use the full name org.f.T.... or at least write /* Deprecated interface Capability */ because it is not clear it refers to the D-Bus interface.

+      /* for now, accept either the current InitialAudio (which is in
+       * the FUTURE) or the hypothetical final version */
Does NewChannels signal put both FUTURE and hypothetical final properties too ? If not, it looks non-consistent

At different places: use g_value_array_get_nth() instead of "GValueArray->va + n"?

advertise-caps2.py: do a test with more than 1 client?
Idem in caps/tube-caps.py

Test with a client which advertise a tube channel with audio/video token caps (sic). Gabble should not add that in any channel class since it is invalid.

(In reply to comment #2)
> static TpHandleRepoIface *feature_handles = NULL;
> Add a comment to say the content of this pool is not TpHandle in the meaning of
> the Tp Spec, but we just reuse the ref-counted string pool implementation which
> is called TpHandleRepoIface.

Will do.

> 
>       DEBUG ("not emitting signal: not connected yet");
> Do not confuse with dbus signal: s/signal/presence stanza/

I'll fix that.

> gabble_caps_channel_manager_represent_client(): Why is it called like this? Why
> "represent"?

The reasoning was "convert the client's capabilities to their XMPP representation".

> "<ContactCapabilities.DRAFT1>"
> Add a comment to say this string is choosen to not conflict with a possible
> well-known telepathy client name

Will do, although I don't think that's very important tbh.

> In _emit_capabilities_changed():
> The comments /* Capabilities */, etc.: use the full name org.f.T.... or at
> least write /* Deprecated interface Capability */ because it is not clear it
> refers to the D-Bus interface.

I'll try to provide some better wording.

> +      /* for now, accept either the current InitialAudio (which is in
> +       * the FUTURE) or the hypothetical final version */
> Does NewChannels signal put both FUTURE and hypothetical final properties too ?
> If not, it looks non-consistent

No, the NewChannels signal only has the current (draft, FUTURE) properties; but I hope that we can undraft InitialAudio/InitialVideo simultaneously with ContactCapabilities, in practice.

The rationale for accepting a hypothetical final version of IA/IV is that it gives this Gabble compatibility with more clients; on the other hand, we could just hurry up and undraft this stuff.

> At different places: use g_value_array_get_nth() instead of "GValueArray->va +
> n"?

I find the latter form clearer; some of the "helpful" GLib macros like g_value_array_get_nth just obscure what's happening, IMO.

> advertise-caps2.py: do a test with more than 1 client?
> Idem in caps/tube-caps.py

I think doing this in advertise-draft2 would be plenty: it's the same code path.

> Test with a client which advertise a tube channel with audio/video token caps
> (sic). Gabble should not add that in any channel class since it is invalid.

Good thinking.

(In reply to comment #3)
> (In reply to comment #2)
> > static TpHandleRepoIface *feature_handles = NULL;
> > Add a comment [...]

Fixed in the branch

> >       DEBUG ("not emitting signal: not connected yet");
> > Do not confuse with dbus signal: s/signal/presence stanza/

Fixed in the branch

> > "<ContactCapabilities.DRAFT1>"
> > Add a comment to say this string is choosen to not conflict with a possible
> > well-known telepathy client name
> 
> Will do, although I don't think that's very important tbh.

These strings actually only appear in a debug message. The one you complained about is deleted later in the branch (when I switch from draft 1, which doesn't have client names, to draft 2, which does) but I've added a comment for the use of "<legacy Capabilities>".

> > In _emit_capabilities_changed():
> > The comments /* Capabilities */, etc.: use the full name org.f.T.... or at
> > least write /* Deprecated interface Capability */ because it is not clear it
> > refers to the D-Bus interface.

Fixed in the branch, I used /* o.f.T.C.Capabilities */ etc.

> > advertise-caps2.py: do a test with more than 1 client?

I did. AbiWord's caps are set before Connect(), and KCall's are added afterwards; both clients' caps are then removed, and a couple of versions of KCall's caps are re-added.

> > Idem in caps/tube-caps.py
> 
> I think doing this in advertise-draft2 would be plenty: it's the same code
> path.

I don't think this is necessary; the general code path for "advertise some clients' caps" has been tested, in caps/advertise-draft2.py, so we only need to check the specifics of Tubes in caps/tube-caps.py.

> > Test with a client which advertise a tube channel with audio/video token caps
> > (sic). Gabble should not add that in any channel class since it is invalid.
> 
> Good thinking.

I think the correct result is actually "Tubes caps are advertised, but Gabble is still not callable"; tokens that don't (seem to) make sense for any supported channel class should just be ignored. I've added a test (which failed), and corrected the media factory's behaviour to make it pass.

(Conceptually, the set of tokens isn't bound to a channel class - they're properties of the whole Client, not of any particular channel class.)

\o\ /o/ \o/

Will be in 0.9.0.