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.) 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 Created attachment 60080 [details] [review] [03/14] Test tp_account_channel_request_new_text, tp_account_channel_request_set_target_id Created attachment 60081 [details] [review] [04/14] tp_tests_connection_run_until_contact_by_id: add Created attachment 60082 [details] [review] [05/14] Test tp_account_channel_request_set_target_contact Created attachment 60083 [details] [review] [06/14] TpAccountChannelRequest: test that properties get through to the request Created attachment 60084 [details] [review] [07/14] tp_account_channel_request_set_request_property: add Created attachment 60085 [details] [review] [08/14] tp_account_channel_request_new_audio_call, audio_video_call: add Created attachment 60086 [details] [review] [09/14] Test requesting audio and audio/video calls, and arbitrary properties Created attachment 60087 [details] [review] [10/14] tp_account_channel_request_new_file_transfer: add Created attachment 60088 [details] [review] [11/14] Test tp_account_channel_request_new_file_transfer 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. 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. Created attachment 60091 [details] [review] [14/14] Test tp_account_channel_request_set_file_transfer_description etc. 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 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 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 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. 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 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 (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. (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. (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. 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 Created attachment 60576 [details] [review] [11/16 v2] Test tp_account_channel_request_new_file_transfer --- Unused GHashTable now removed. Created attachment 60577 [details] [review] [14/16 v2] Test tp_account_channel_request_set_file_transfer_description etc. --- Unused GHashTable now removed. Created attachment 60578 [details] [review] [15/16] Format bullet lists correctly gtk-doc's subset of Markdown allows "-" as a bullet point, but not "*". Created attachment 60579 [details] [review] [16/16] Don't (visibly) document ROOM-based Call channels yet No CM implements them, yet. Your new patches look good. (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 :) 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.
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(...)