Bug 27511 - Need API to represent capabilities
Summary: Need API to represent capabilities
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Guillaume Desmottes
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ca...
Whiteboard: review+
Keywords: patch
Depends on: 21097
Blocks: 21332 27618
  Show dependency treegraph
 
Reported: 2010-04-07 05:02 UTC by Guillaume Desmottes
Modified: 2010-04-14 05:50 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Comment 1 Guillaume Desmottes 2010-04-07 05:03:50 UTC
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.
"""
Comment 2 Guillaume Desmottes 2010-04-12 06:32:20 UTC
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.
Comment 3 Guillaume Desmottes 2010-04-13 05:27:25 UTC
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
Comment 4 Simon McVittie 2010-04-13 08:10:13 UTC
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.
Comment 5 Guillaume Desmottes 2010-04-14 01:54:26 UTC
(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.
Comment 6 Simon McVittie 2010-04-14 04:21:15 UTC
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+.
Comment 7 Guillaume Desmottes 2010-04-14 05:43:26 UTC
(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".
Comment 8 Guillaume Desmottes 2010-04-14 05:50:17 UTC
Merged. Will be in 0.11.3


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.