Bug 48780 - add API to request channels without speaking fluent D-Bus
Summary: add API to request channels without speaking fluent D-Bus
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://cgit.freedesktop.org/~smcv/tel...
Whiteboard:
Keywords: patch
Depends on:
Blocks: 30422
  Show dependency treegraph
 
Reported: 2012-04-16 11:32 UTC by Simon McVittie
Modified: 2012-05-03 10:42 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
[01/14] TpAccountChannelRequest: copy the hash table with a specific memory model (1.93 KB, patch)
2012-04-16 11:39 UTC, Simon McVittie
Details | Splinter Review
02/14] tp_account_channel_request_new_text, tp_account_channel_request_set_target_contact, tp_account_channel_request_set_target_id: add (7.80 KB, patch)
2012-04-16 11:39 UTC, Simon McVittie
Details | Splinter Review
[03/14] Test tp_account_channel_request_new_text, tp_account_channel_request_set_target_id (1.39 KB, patch)
2012-04-16 11:40 UTC, Simon McVittie
Details | Splinter Review
[04/14] tp_tests_connection_run_until_contact_by_id: add (2.09 KB, patch)
2012-04-16 11:40 UTC, Simon McVittie
Details | Splinter Review
[05/14] Test tp_account_channel_request_set_target_contact (1.88 KB, patch)
2012-04-16 11:40 UTC, Simon McVittie
Details | Splinter Review
[06/14] TpAccountChannelRequest: test that properties get through to the request (5.08 KB, patch)
2012-04-16 11:40 UTC, Simon McVittie
Details | Splinter Review
[07/14] tp_account_channel_request_set_request_property: add (3.83 KB, patch)
2012-04-16 11:41 UTC, Simon McVittie
Details | Splinter Review
[08/14] tp_account_channel_request_new_audio_call, audio_video_call: add (8.39 KB, patch)
2012-04-16 11:41 UTC, Simon McVittie
Details | Splinter Review
[09/14] Test requesting audio and audio/video calls, and arbitrary properties (4.51 KB, patch)
2012-04-16 11:42 UTC, Simon McVittie
Details | Splinter Review
[10/14] tp_account_channel_request_new_file_transfer: add (4.54 KB, patch)
2012-04-16 11:42 UTC, Simon McVittie
Details | Splinter Review
[11/14] Test tp_account_channel_request_new_file_transfer (2.36 KB, patch)
2012-04-16 11:42 UTC, Simon McVittie
Details | Splinter Review
[12/14] Add and test functions to check for particular file transfer properties (14.63 KB, patch)
2012-04-16 11:43 UTC, Simon McVittie
Details | Splinter Review
[13/14] tp_account_channel_request_set_file_transfer_description etc.: add (8.57 KB, patch)
2012-04-16 11:43 UTC, Simon McVittie
Details | Splinter Review
[14/14] Test tp_account_channel_request_set_file_transfer_description etc. (3.86 KB, patch)
2012-04-16 11:43 UTC, Simon McVittie
Details | Splinter Review
[12/16 v2] Add and test functions to check for particular file transfer properties (14.69 KB, patch)
2012-04-25 08:09 UTC, Simon McVittie
Details | Splinter Review
[11/16 v2] Test tp_account_channel_request_new_file_transfer (2.55 KB, patch)
2012-04-25 08:09 UTC, Simon McVittie
Details | Splinter Review
[14/16 v2] Test tp_account_channel_request_set_file_transfer_description etc. (3.82 KB, patch)
2012-04-25 08:10 UTC, Simon McVittie
Details | Splinter Review
[15/16] Format bullet lists correctly (2.06 KB, patch)
2012-04-25 08:10 UTC, Simon McVittie
Details | Splinter Review
[16/16] Don't (visibly) document ROOM-based Call channels yet (1.43 KB, patch)
2012-04-25 08:11 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2012-04-16 11:32:37 UTC
At the moment, requesting a channel requires that you construct a GHashTable and populate it with D-Bus properties.

It should be possible to do simple channel requests without doing all that, using TpAccountChannelRequest as the high-level API.

Here's what I propose (using PyGI as pseudocode):

    acr = Tp.AccountChannelRequest.new_file_transfer(account,
        # in other channel types' helper functions, the mandatory properties
        # go here (e.g. Text doesn't have any)
        "warez.rar",             # filename
        "application/x-rar",     # MIME type
        1024 * 1024,             # file size
        Tp.USER_ACTION_TIME_CURRENT_TIME)
   
    # properties which can be done several ways (target) are a method
    acr.set_target_contact(alice)
    # equivalent: acr.set_target_id(Tp.HandleType.CONTACT, "alice@example.com")

    # non-mandatory properties work like this too
    acr.set_file_transfer_uri("file:///tmp/warez.rar")

    # just like before
    acr.create_and_handle_channel_async(...)
Comment 1 Simon McVittie 2012-04-16 11:39:41 UTC
Created attachment 60078 [details] [review]
[01/14] TpAccountChannelRequest: copy the hash table with a  specific memory model

Forcing use of a particular key and value destructor makes it safe to
edit the hash table after the request has been created. (Setting every
property in the constructor doesn't scale, in the presence of optional
properties, and we want high-level API for this so apps can be a bit
less D-Bus-API-dependent.)
Comment 2 Simon McVittie 2012-04-16 11:39:59 UTC
Created attachment 60079 [details] [review]
02/14] tp_account_channel_request_new_text,                 tp_account_channel_request_set_target_contact,  tp_account_channel_request_set_target_id: add
Comment 3 Simon McVittie 2012-04-16 11:40:12 UTC
Created attachment 60080 [details] [review]
[03/14] Test tp_account_channel_request_new_text,            tp_account_channel_request_set_target_id
Comment 4 Simon McVittie 2012-04-16 11:40:25 UTC
Created attachment 60081 [details] [review]
[04/14] tp_tests_connection_run_until_contact_by_id: add
Comment 5 Simon McVittie 2012-04-16 11:40:39 UTC
Created attachment 60082 [details] [review]
[05/14] Test tp_account_channel_request_set_target_contact
Comment 6 Simon McVittie 2012-04-16 11:40:53 UTC
Created attachment 60083 [details] [review]
[06/14] TpAccountChannelRequest: test that properties get    through to the request
Comment 7 Simon McVittie 2012-04-16 11:41:36 UTC
Created attachment 60084 [details] [review]
[07/14] tp_account_channel_request_set_request_property: add
Comment 8 Simon McVittie 2012-04-16 11:41:52 UTC
Created attachment 60085 [details] [review]
[08/14] tp_account_channel_request_new_audio_call,            audio_video_call: add
Comment 9 Simon McVittie 2012-04-16 11:42:07 UTC
Created attachment 60086 [details] [review]
[09/14] Test requesting audio and audio/video calls, and      arbitrary properties
Comment 10 Simon McVittie 2012-04-16 11:42:23 UTC
Created attachment 60087 [details] [review]
[10/14] tp_account_channel_request_new_file_transfer: add
Comment 11 Simon McVittie 2012-04-16 11:42:46 UTC
Created attachment 60088 [details] [review]
[11/14] Test tp_account_channel_request_new_file_transfer
Comment 12 Simon McVittie 2012-04-16 11:43:08 UTC
Created attachment 60089 [details] [review]
[12/14] Add and test functions to check for particular file   transfer properties

These functions make the simplifying assumption that all of these    
properties are orthogonal, so if both (say) InitialOffset and Description      
are supported, then they'll be supported in the same requestable               
channel class.                                                                           
This is about the best you can do without exposing the
requestable channel classes themselves as API and having applications           iterate through them, which seems lower-level than we want.
Comment 13 Simon McVittie 2012-04-16 11:43:31 UTC
Created attachment 60090 [details] [review]
[13/14] tp_account_channel_request_set_file_transfer_description  etc.: add

These are partly useful in their own right, and partly a demonstration
of how any other optional properties should work in this API.
Comment 14 Simon McVittie 2012-04-16 11:43:54 UTC
Created attachment 60091 [details] [review]
[14/14] Test tp_account_channel_request_set_file_transfer_description  etc.
Comment 15 Simon McVittie 2012-04-16 11:51:18 UTC
I haven't done Tubes yet, but you get the idea! I think I'd do them by having Service or ServiceName passed to the new_stream_tube or new_dbus_tube constructor. Both are mandatory-to-implement and mandatory-to-request, so, easy.

(In reply to comment #7)
> [07/14] tp_account_channel_request_set_request_property: add

The observant will notice that the API is in terms of GVariant. I'd rather not add a GHashTable version of this, even though that would be more efficient in the short term.

(In reply to comment #12)
> This is about the best you can do without exposing the
> requestable channel classes themselves as API and having applications          
> iterate through them, which seems lower-level than we want.

If we want to do this, we should probably do it by having TpCapabilities be iterable, and return a list of TpCapabilities objects representing the individual channel classes? Or something like that - halfway between D-Bus and high-level.
Comment 16 Jonny Lamb 2012-04-17 10:35:22 UTC
Comment on attachment 60079 [details] [review]
02/14] tp_account_channel_request_new_text,                 tp_account_channel_request_set_target_contact,  tp_account_channel_request_set_target_id: add

Review of attachment 60079 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/account-channel-request.c
@@ +1820,5 @@
> +  g_return_val_if_fail (TP_IS_ACCOUNT (account), NULL);
> +
> +  request = tp_asv_new (
> +      TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, TP_IFACE_CHANNEL_TYPE_TEXT,
> +      NULL);

Didn't you just say you shouldn't use tp_asv_new here? This just gets duped and the value freed with g_free() no? Am I missing something here?
Comment 17 Jonny Lamb 2012-04-17 10:53:32 UTC
Comment on attachment 60085 [details] [review]
[08/14] tp_account_channel_request_new_audio_call,            audio_video_call: add

Review of attachment 60085 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/account-channel-request.c
@@ +1915,5 @@
> + * takes place in a named chatroom, by calling
> + * tp_account_channel_request_set_target_id() with @handle_type
> + * set to %TP_HANDLE_TYPE_ROOM. To test whether this is possible, use
> + * tp_capabilities_supports_audio_call() with @handle_type set to
> + * %TP_HANDLE_TYPE_ROOM.

Perhaps we should half implement this anywhere before advertising it in documentation.
Comment 18 Jonny Lamb 2012-04-17 11:28:56 UTC
Comment on attachment 60091 [details] [review]
[14/14] Test tp_account_channel_request_set_file_transfer_description  etc.

Review of attachment 60091 [details] [review]:
-----------------------------------------------------------------

::: tests/dbus/account-channel-request.c
@@ +358,5 @@
> +{
> +  GHashTable *request;
> +  TpAccountChannelRequest *req;
> +
> +  request = create_request ();

You don't use this.
Comment 19 Jonny Lamb 2012-04-17 11:31:51 UTC
All your patches are fine except for my comments.

(In reply to comment #15)
> (In reply to comment #12)
> > This is about the best you can do without exposing the
> > requestable channel classes themselves as API and having applications          
> > iterate through them, which seems lower-level than we want.
> 
> If we want to do this, we should probably do it by having TpCapabilities be
> iterable, and return a list of TpCapabilities objects representing the
> individual channel classes? Or something like that - halfway between D-Bus and
> high-level.

I think the functions you've added are fine. If we were going to grow another 100 properties on FT then the iterating route would be better, but we're not.
Comment 20 Xavier Claessens 2012-04-25 05:31:18 UTC
Comment on attachment 60089 [details] [review]
[12/14] Add and test functions to check for particular file   transfer properties

Review of attachment 60089 [details] [review]:
-----------------------------------------------------------------

tbh, I would have _supports_file_transfer_full(self, flags) and expose FTCapFlags
Comment 21 Simon McVittie 2012-04-25 07:30:58 UTC
(In reply to comment #16)
> 02/14] tp_account_channel_request_new_text,                
> tp_account_channel_request_set_target_contact, 
> tp_account_channel_request_set_target_id: add

> Didn't you just say you shouldn't use tp_asv_new here? This just gets duped and
> the value freed with g_free() no? Am I missing something here?

This is a little confusing but I think it's right.

request (local variable) has static keys, so it's OK to use tp_asv_new() (which assumes immortal keys and sliced values).

It's deep-copied into priv->request, which, yes, must not use tp_asv_new(), because we want to allow arbitrary (non-immortal) keys in priv->request.
Comment 22 Simon McVittie 2012-04-25 07:33:10 UTC
(In reply to comment #17)
> Perhaps we should half implement this anywhere before advertising it in
> documentation.

I thought Gabble already implemented Muji calls, as Call/ROOM channels, but apparently not. Fair enough, I'll remove that paragraph.
Comment 23 Simon McVittie 2012-04-25 07:52:34 UTC
(In reply to comment #20)
> tbh, I would have _supports_file_transfer_full(self, flags) and expose
> FTCapFlags

My concern about this whole commit is: how does anything complex work in a UI? Realistically, the UI will want to ask, for each input box: do I show this or not? - and leave it at that.

If flags is an "out" argument, it's flattening potentially multiple requestable channel classes into one. (Imagine that Gabble supports both Jingle and SI file transfers, and they have different capabilities.) The TpCapabilities object has to either take an arbitrary File Transfer RCC (the first?), the union, or the intersection; any of those is basically misleading, but we can't do better without having the UI understand that there are multiple sub-protocols, and iterate over them.

The API I implemented is effectively the same as taking the union - the UI will offer everything supported by any of the sub-protocols, and if the user specifies too many things, the request might fail. It at least does that without falsely implying that a complete set of things is supported, though...

If flags is an "in" argument, in principle the UI could play "20 questions" with the TpCapabilities to get the most complete possible list of fields:

    do you support FT with a description, a date and an offset?
    - no
    do you support FT with a description?
    - yes
    do you support FT with a description and a date?
    - no
    do you support FT with a description and an offset?
    - yes
    (show description, offset inputs, but hide date input)

but the number of questions required is a combinatorial explosion, so... no. Realistically, nobody's going to do that anyway.

A more likely (mis-)implementation is a UI that has this "conversation" with a basic FT implementation that lacks all the optional extras:

    do you support FT with a description, a date and an offset?
    - no
    (hide FT support entirely)

and we don't want that.
Comment 24 Simon McVittie 2012-04-25 08:09:03 UTC
Created attachment 60575 [details] [review]
[12/16 v2] Add and test functions to check for particular file  transfer properties

These functions make the simplifying assumption that all of these
properties are orthogonal, so if both (say) InitialOffset and Description
are supported, then they'll be supported in the same requestable
channel class.
 
This is about the best you can do without exposing the
requestable channel classes themselves as API and having applications
iterate through them, which seems lower-level than we want.

---

Merged into current master
Comment 25 Simon McVittie 2012-04-25 08:09:50 UTC
Created attachment 60576 [details] [review]
[11/16 v2] Test tp_account_channel_request_new_file_transfer

---

Unused GHashTable now removed.
Comment 26 Simon McVittie 2012-04-25 08:10:26 UTC
Created attachment 60577 [details] [review]
[14/16 v2] Test  tp_account_channel_request_set_file_transfer_description  etc.

---

Unused GHashTable now removed.
Comment 27 Simon McVittie 2012-04-25 08:10:49 UTC
Created attachment 60578 [details] [review]
[15/16] Format bullet lists correctly

gtk-doc's subset of Markdown allows "-" as a bullet point, but not "*".
Comment 28 Simon McVittie 2012-04-25 08:11:34 UTC
Created attachment 60579 [details] [review]
[16/16] Don't (visibly) document ROOM-based Call channels yet

No CM implements them, yet.
Comment 29 Jonny Lamb 2012-04-25 09:14:50 UTC
Your new patches look good.
Comment 30 Xavier Claessens 2012-05-02 05:36:55 UTC
(In reply to comment #23)
> (In reply to comment #20)
> A more likely (mis-)implementation is a UI that has this "conversation" with a
> basic FT implementation that lacks all the optional extras:
> 
>     do you support FT with a description, a date and an offset?
>     - no
>     (hide FT support entirely)
> 
> and we don't want that.

That's what I had in mind, yes. But indeed that could be a bad idea :)
Comment 31 Simon McVittie 2012-05-03 10:42:41 UTC
In git for 0.19.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.