Bug 27511

From bug #21332:

"""
The reason this is blocked by Bug #21097 is that ContactCapabilities and
RequestableChannelClasses are related:

* You should be able to ask the Connection for its RequestableChannelClasses,
as a Capabilities object (this is a feature in the sense of Bug #21097, IMO -
perhaps TP_CONNECTION_FEATURE_CAPABILITIES)

* You should also be able to ask a Contact for its capabilities, as a
Capabilities object

* If the Connection doesn't implement ContactCapabilities, every Contact should
wait for TP_CONNECTION_FEATURE_CAPABILITIES, then return a copy of the
Connection's Capabilities object, with a flag set to say "this is just a guess
really"

See the equivalent APIs in telepathy-qt4 for details; I basically want to
implement a GObject clone of those.
"""

I naively mimiced the Qt4 API. See http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/capabilities-27511

Let me know if that's something like that you are expecting.

Here is a first version of this API:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/capabilities-27511

Please assign things you're actively working on to yourself; telepathy-bugs is now the QA contact for everything, so it'll remain in the loop.

Code
====

> +tp_capabilities_is_specific_to_contact

Perhaps this would be better as some concept of "source" with three values?

* from the connection
* for a contact, but guessed from the connection's caps
* for a contact, from ContactCapabilities

and in future (Bug #20774) also:

* for a Protocol

(In telepathy-qt4 the class hierarchy provides most of this.)

I'm not sure whether ...supports_text_chats() is the best name we can have for a function about 1-1 chats, but I can't immediately think of anything better.

> +static gboolean
> +supports_text_channel (TpCapabilities *self,
> +    TpHandleType exported_handle_type)

Do you mean "expected" rather than "exported"?

I think this helper function could usefully take the channel type as a parameter too, and be called supports_simple_channel() or something.

> +      fixed =  g_value_get_boxed (g_value_array_get_nth (arr, 0));
[add blank line]
> +      if (g_hash_table_size (fixed) != 2)
> +        continue;

I'd prefer a blank line where indicated, and the same for if (!valid).

Tests
=====

> +++ b/tests/capabilities.c

I'd prefer new tests to use GTest (g_test_add() etc.), and use the g_assert_foo family (particularly g_assert_cmpuint and friends) in preference to MYASSERT().

> +  g_value_init (&class, TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS);

I'd prefer tp_value_array_build() rather than messing about with temporary GValues.

(In reply to comment #4)
> > +tp_capabilities_is_specific_to_contact
> 
> Perhaps this would be better as some concept of "source" with three values?
> 
> * from the connection
> * for a contact, but guessed from the connection's caps
> * for a contact, from ContactCapabilities
> 
> and in future (Bug #20774) also:
> 
> * for a Protocol

I can't really think of a use case when user will end up with a TpCapabilities without having requested from a TpConnection/TpContact/TpProtocol before; so I'm not sure that's that useful.
Furthermore, that'd introduce some kind of two-way relationship between classes.

 
> I'm not sure whether ...supports_text_chats() is the best name we can have for
> a function about 1-1 chats, but I can't immediately think of anything better.

I mimiced the name of the tp-qt4 method.

> > +static gboolean
> > +supports_text_channel (TpCapabilities *self,
> > +    TpHandleType exported_handle_type)
> 
> Do you mean "expected" rather than "exported"?

I do. Fixed.

> I think this helper function could usefully take the channel type as a
> parameter too, and be called supports_simple_channel() or something.

Done.

> > +      fixed =  g_value_get_boxed (g_value_array_get_nth (arr, 0));
> [add blank line]
> > +      if (g_hash_table_size (fixed) != 2)
> > +        continue;
> 
> I'd prefer a blank line where indicated, and the same for if (!valid).

Done.

> Tests
> =====
> 
> > +++ b/tests/capabilities.c
> 
> I'd prefer new tests to use GTest (g_test_add() etc.), and use the g_assert_foo
> family (particularly g_assert_cmpuint and friends) in preference to MYASSERT().

done.

> > +  g_value_init (&class, TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS);
> 
> I'd prefer tp_value_array_build() rather than messing about with temporary
> GValues.

Good point; I forgot about it. Done.


I commited changes in separated commits for easier reviewing. I'll squash them once you have looked at them.

The changes look good.

(In reply to comment #5)
> (In reply to comment #4)
> > > +tp_capabilities_is_specific_to_contact
> > 
> > Perhaps this would be better as some concept of "source" with three values?
> > 
> > * from the connection
> > * for a contact, but guessed from the connection's caps
> > * for a contact, from ContactCapabilities
> > 
> > and in future (Bug #20774) also:
> > 
> > * for a Protocol
> I can't really think of a use case when user will end up with a TpCapabilities
> without having requested from a TpConnection/TpContact/TpProtocol before; so
> I'm not sure that's that useful.

I suppose there's nothing inherently wrong with having tp_capabilities_is_specific_to_contact() exist and always return FALSE for Protocol and Connection caps.

> Furthermore, that'd introduce some kind of two-way relationship between
> classes.

Not really; TpConnection depends on TpCapabilities, and both depend on the D-Bus Connection object :-)

But yes, if you're happy with this API yourself, review+.

(In reply to comment #6)
> The changes look good.

http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/capabilities-27511

contains the squashed branch.

> (In reply to comment #5)
> I suppose there's nothing inherently wrong with having
> tp_capabilities_is_specific_to_contact() exist and always return FALSE for
> Protocol and Connection caps.

I think so. In pratice clients just care about "is this button sensitive or not".

Merged. Will be in 0.11.3