Bug 23492

Summary: Implement final ContactCapabilities interface
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: gabbleAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/caps-draft2
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 19683    
Bug Blocks:    

Description Simon McVittie 2009-08-24 13:33:52 UTC
+++ This bug was initially created as a clone of Bug #19683 +++

When the contact capabilities refactor (Bug #19683) has been reviewed, Gabble's ContactCapabilities implementation should be brought up to draft 2 status.
Comment 1 Simon McVittie 2009-08-26 12:19:24 UTC
\o/
Comment 2 Alban Crequy 2009-09-08 02:07:12 UTC
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.
Comment 3 Simon McVittie 2009-09-08 04:11:12 UTC
(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.
Comment 4 Simon McVittie 2009-09-08 04:33:03 UTC
(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.)
Comment 5 Simon McVittie 2009-09-10 05:02:34 UTC
\o\ /o/ \o/

Will be in 0.9.0.

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.