TpConnection object should have a "new-channel" signal that gives a TpChannel object once it's ready. So TpConnection should connect to NewChannel signal, create a TpChannel object and wait for it to be ready before emitting "new-channel". I could be useful to have tp_connection_list_channels() that returns all TpChannel objects that are ready. So TpConnection should cache TpChannel objects to be able to return them directly. Finaly we could have tp_connection_request_channel() that returns a TpChannel object and block until it gets ready, and tp_connection_request_channel_async() that gives a ready TpChannel in an async cb.
1) WONTFIX. Creating a TpChannel signifies that you care about the channel, and causes automatic introspection stuff to run. It's wasteful and possibly actively harmful to do this for channels you don't care about. The handler for NewChannel, provided by the user of telepathy-glib, should decide whether it cares about the channel; if yes, *then* it should create a TpChannel. 2) WONTFIX. Likewise. (It could be interesting for the TpConnection to cache weak references so it will only ever create a new TpChannel if there was no existing TpChannel, but I'm not sure that's worth the effort; it still wouldn't be a guarantee that every distinct TpChannel represents a distinct TpConnection, unless TpConnection also had some sort of weak-ref-based uniquification. I'm undecided about this.) 3) Entirely reasonable.
I'm not going to fix this until the new request/dispatch API has been designed, to avoid bloating the API with deprecated versions.
Changing subject to what I'd prefer to implement.
We should follow telepathy-qt4's lead on this, since I already designed this feature once :-) I'm no longer sure that we want high-level API for channel creation on the Connection, since the Right Way™ is now to use the ChannelDispatcher. As medium-level API, we should have methods similar to the proposed ones (with an a{sv} parameter), but on the TpAccount (as an implementation detail, they'll talk to the channel dispatcher). As high-level API, in telepathy-qt4 we have API on the Tp::Account like this: PendingOperation *ensureTextChat(QString contact_id, ...); PendingOperation *ensureTextChat(Tp::Contact contact, ...); and similar for ensureMediaCall, ensureTextChatroom, etc. As we gain higher-level API for more channel types, we'll also have createFileTransfer, createContactSearch and so on. We'll only wrap the mode (create/ensure) that is more useful for a particular channel type - for calls and chats you almost always want to ensure, for file transfers and contact searches you always want to create. There is more prior art in libmcclient.
(In reply to comment #4) > I'm no longer sure that we want high-level API for channel creation on the > Connection, since the Right Way™ is now to use the ChannelDispatcher. Danielle has pointed out that for language bindings, we do want a non-generated GAsyncResult API for this. This is represented by Bug #27602. > As medium-level API, we should have methods similar to the proposed ones (with > an a{sv} parameter), but on the TpAccount (as an implementation detail, they'll > talk to the channel dispatcher). Not yet being worked on. > As high-level API, in telepathy-qt4 we have API on the Tp::Account like this: > > PendingOperation *ensureTextChat(QString contact_id, ...); > PendingOperation *ensureTextChat(Tp::Contact contact, ...); Similarly, not yet being worked on.
Ok. So I've had a very interesting idea here. It would be pretty neat to populate the detail for the GSignal with the last part of the channel type (e.g. Text, ContactList) so that the user can connect new-channel::text if she wishes. In fact, rather than simply using the channel type, we could extend this further by also considering the handle type, perhaps allowing for the following possible details: ::chat (channel-type Text, handle-type Contact) ::muc (channel-type Text, handle-type Room) ::contact-list (channel-type ContactList, handle-type List) ::contact-group (channel-type ContactList, handle-type Group) ::dbus-tube ::stream-tube ::file-transfer We could then conceivably take this one step further and use g_signal_has_handler_pending() to know whether or not we have a pending handler, to decide whether or not to prepare a TpChannel for this handler. However maybe this is going too far (it's not like preparation is a big step on top of receiving the signal).
(In reply to comment #5) > > As medium-level API, we should have methods similar to the proposed ones (with > > an a{sv} parameter), but on the TpAccount (as an implementation detail, they'll > > talk to the channel dispatcher). > > Not yet being worked on. I can work on this next. It seems that the high-level API you propose would use this API internally, so this is more generic and thus more useful.
+ tp_cli_connection_interface_requests_call_ensure_channel (self, -1, Shouldn't we use a bigger timeout? Or let the user choose it? When Empathy requests contact list channels (on the Connection), it passes G_MAXINT as this operation can take a while if the server is slow to send the contact list. I think MC uses a bigger timeout when requesting channels as well.
(In reply to comment #6) > It would be pretty neat to populate the detail for the GSignal with the last > part of the channel type (e.g. Text, ContactList) so that the user can connect > new-channel::text if she wishes. As Sjoerd said on IRC, and I said in Comment #2, the only part of Xavier's initial request that I want to keep is the requesting. Having every Telepathy client that has a Connection make D-Bus calls to prepare every Channel, as an API guarantee that can't be reverted, would be a massive mistake (been there, done that: see OLPC Sugar during the time I was working on it, although hopefully it's been fixed since).
I implemented the "create/ensure channel and let me handle it" case in: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-creation-13422 I kinda simplified the channel creation and dispatching in my fake ChannelDispatcher to avoid having to re-implement half of MissionControl :)
In the successful case, the temporary handler should stick around until the channel is invalidated, for MC crash-recovery (it needs to be able to say it's handling the channel, otherwise MC will close the channel while recovering from a crash). I think it'd make sense to replace request_ctx with a private subclass of TpBaseClient - perhaps called TpAccountCreateAndHandleHelper? - which survives until either the channel request fails, or the channel is invalidated. The request_ctx shouldn't need to have the TpDBusDaemon in it; I've added tp_base_client_get_dbus_daemon() in my trivia branch. > + GError error = { TP_ERRORS, TP_ERROR_INVALID_ARGUMENT, > + "We are supposed to handle only one channel" }; > + > + tp_handle_channels_context_fail (context, &error); > + /* FIXME: Should we fail the operation ? */ Yes, we should, otherwise the operation will never finish. Shouldn't this async operation be cancellable? The meaning of cancelling the GCancellable would be something like this: * if CreateChannel hasn't returned yet, do nothing * when CreateChannel returns: if already cancelled, call Cancel on the ChannelRequest * if CreateChannel has already returned, but the ChannelRequest hasn't been invalidated, call Cancel on it * if the ChannelRequest has already been invalidated, we're too late to cancel, so ignore it This would mean that cancelling the GCancellable would raise TP_ERROR_CANCELLED instead of G_IO_ERROR_CANCELLED, which I think is OK. It'd also raise TP_ERROR_CANCELLED if someone other than us cancelled the request for some reason, or if the TpConnection was disconnected by our request. > + if (tp_cli_channel_request_connect_to_failed (ctx->chan_request, > + channel_request_failed_cb, ctx, NULL, > + G_OBJECT (ctx->result), &err) == NULL) Connect to TpProxy::invalidated, so you'll handle an MC crash correctly. Succeeded maps to TP_DBUS_ERROR_OBJECT_REMOVED, so you'll need to ignore that case with g_error_matches(), if you wanted to ignore Succeeded. > + if (g_list_length (channels) != 1) G_UNLIKELY?
(In reply to comment #11) > In the successful case, the temporary handler should stick around until the > channel is invalidated, for MC crash-recovery (it needs to be able to say it's > handling the channel, otherwise MC will close the channel while recovering from > a crash). Isn't that what I did in http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=2a4047d81dbed1afd7525d9755f0f681b4b0502b ? > I think it'd make sense to replace request_ctx with a private > subclass of TpBaseClient - perhaps called TpAccountCreateAndHandleHelper? - > which survives until either the channel request fails, or the channel is > invalidated. I will look at that. > The request_ctx shouldn't need to have the TpDBusDaemon in it; I've added > tp_base_client_get_dbus_daemon() in my trivia branch. > > > + GError error = { TP_ERRORS, TP_ERROR_INVALID_ARGUMENT, > > + "We are supposed to handle only one channel" }; > > + > > + tp_handle_channels_context_fail (context, &error); > > + /* FIXME: Should we fail the operation ? */ > > Yes, we should, otherwise the operation will never finish. done. > Shouldn't this async operation be cancellable? The meaning of cancelling the > GCancellable would be something like this: > > * if CreateChannel hasn't returned yet, do nothing > * when CreateChannel returns: if already cancelled, call Cancel on the > ChannelRequest > * if CreateChannel has already returned, but the ChannelRequest hasn't been > invalidated, call Cancel on it > * if the ChannelRequest has already been invalidated, we're too late to cancel, > so ignore it > > This would mean that cancelling the GCancellable would raise TP_ERROR_CANCELLED > instead of G_IO_ERROR_CANCELLED, which I think is OK. It'd also raise > TP_ERROR_CANCELLED if someone other than us cancelled the request for some > reason, or if the TpConnection was disconnected by our request. Ok, I'll do that. > > + if (tp_cli_channel_request_connect_to_failed (ctx->chan_request, > > + channel_request_failed_cb, ctx, NULL, > > + G_OBJECT (ctx->result), &err) == NULL) > > Connect to TpProxy::invalidated, so you'll handle an MC crash correctly. > Succeeded maps to TP_DBUS_ERROR_OBJECT_REMOVED, so you'll need to ignore that > case with g_error_matches(), if you wanted to ignore Succeeded. done. > > + if (g_list_length (channels) != 1) > > G_UNLIKELY? done.
(In reply to comment #12) > Isn't that what I did in > http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=2a4047d81dbed1afd7525d9755f0f681b4b0502b > ? Sorry, I was reviewing an older version. If you don't convert request_ctx into a TpBaseClient subclass, its name should be in CamelCase anyway, because it's a type. You seem to have a lot of non-breaking spaces in your docs, notably in tp_account_ensure_and_handle_channel_async. Please set your editor to highlight them - in particular, I suspect they break GObject-Introspection annotations. > > > + if (tp_cli_channel_request_connect_to_failed (ctx->chan_request, > > > + channel_request_failed_cb, ctx, NULL, > > > + G_OBJECT (ctx->result), &err) == NULL) > > > > Connect to TpProxy::invalidated, so you'll handle an MC crash correctly. > > Succeeded maps to TP_DBUS_ERROR_OBJECT_REMOVED, so you'll need to ignore that > > case with g_error_matches(), if you wanted to ignore Succeeded. tp_channel_request_failed_cb() invalidates it with an error other than TP_DBUS_ERROR_OBJECT_REMOVED, and Succeeded with TP_DBUS_ERROR_OBJECT_REMOVED, so you don't need to watch for the individual signals at all - just use invalidated. The purpose of invalidated is to give you a single code path for all the possible reasons why your object caught fire and sank into the swamp :-) Doing that will also mean you don't need to worry about disconnecting from Failed and Succeeded when the context is freed, which you don't seem to do at the moment anyway. Either tp_account_ensure_and_handle_channel_async or tp_account_ensure_and_handle_channel_finish or both should document that in particular, if the channel goes to a different handler, you'll see TP_ERROR_NOT_YOURS. > tp_account_ensure_and_handle_channel_finish () Use foo() in gtk-doc, to make a hyperlink. I haven't reviewed the tests yet.
(In reply to comment #11) > The request_ctx shouldn't need to have the TpDBusDaemon in it; I've added > tp_base_client_get_dbus_daemon() in my trivia branch. done. > Shouldn't this async operation be cancellable? done. (In reply to comment #13) > If you don't convert request_ctx into a TpBaseClient subclass, its name should > be in CamelCase anyway, because it's a type. I'm not convinced having a subclass will buy us anything so I renamed it to RequestCtx (I usually don't CamelCase such structure which are not an object). > You seem to have a lot of non-breaking spaces in your docs, notably in > tp_account_ensure_and_handle_channel_async. Please set your editor to highlight > them - in particular, I suspect they break GObject-Introspection annotations. TODO > tp_channel_request_failed_cb() invalidates it with an error other than > TP_DBUS_ERROR_OBJECT_REMOVED, and Succeeded with TP_DBUS_ERROR_OBJECT_REMOVED, > so you don't need to watch for the individual signals at all - just use > invalidated. The purpose of invalidated is to give you a single code path for > all the possible reasons why your object caught fire and sank into the swamp > :-) fixed. > Either tp_account_ensure_and_handle_channel_async or > tp_account_ensure_and_handle_channel_finish or both should document that in > particular, if the channel goes to a different handler, you'll see > TP_ERROR_NOT_YOURS. done. > > tp_account_ensure_and_handle_channel_finish () > > Use foo() in gtk-doc, to make a hyperlink. fixed.
(In reply to comment #14) > > You seem to have a lot of non-breaking spaces in your docs, notably in > > tp_account_ensure_and_handle_channel_async. Please set your editor to highlight > > them - in particular, I suspect they break GObject-Introspection annotations. > > TODO done.
Please add account-channels.c to the whitelist in introspect.am. > + * If the channel is properly ensured but goes to a different handler then the > + * operation will fail with %TP_ERROR_NOT_YOURS as error. "successfully ensured" would be better English. I think this would be better still: If the channel already exists and is already being handled, or if a newly created channel is sent to a different handler, this operation will fail with the error %TP_ERROR_NOT_YOURS. The other handler will be notified that the channel was requested again, and can move its window to the foreground, if applicable. > + * tp_account_ensure_and_handle_channel_finish: ... > + * Returns: %TRUE if the channel was successful created and you are handling > + * it, otherwise %FALSE "successfully created" (also in the _create_ version). I think this docstring should also remind the API user about NotYours (just the first sentence of the above, perhaps). In request_and_handle_channel_cb, if ctx->cancellable has already been cancelled when you call g_cancellable_connect (in practice this means it was cancelled while the Create/EnsureChannel call was in-flight), then you'll call Cancel() followed by Proceed(). You can detect that by only calling Proceed if (ctx->cancellable == NULL || ctx->cancel_id != 0), I think? > + /* Object has ben properly removed so ChannelRequest succeeded */ "has been removed without error, so" Why not do this, which has one less g_error_new? GError e = { domain, code, message }; if (domain == TP_ERRORS && code == TP_DBUS_ERROR_OBJECT_REMOVED) { ... } ... request_ctx_fail (ctx, &e); > + DEBUG ("Proceed sucess; waiting for the channel to be handled"); "Proceed succeeded; waiting" > + /* Request succeed */ "Request succeeded" > + "TpGlibTempHandler", TRUE, handle_channels, ctx, NULL); It's not all that temporary any more? I think I'd prefer "TpGLibRequestAndHandle".
(Tests still to be reviewed, I got distracted by a bee.)
(In reply to comment #16) > Please add account-channels.c to the whitelist in introspect.am. done. > > + * If the channel is properly ensured but goes to a different handler then the > > + * operation will fail with %TP_ERROR_NOT_YOURS as error. > > "successfully ensured" would be better English. I think this would be better > still: > > If the channel already exists and is already being handled, or if a > newly created channel is sent to a different handler, this operation > will fail with the error %TP_ERROR_NOT_YOURS. The other handler > will be notified that the channel was requested again, and can > move its window to the foreground, if applicable. done. > > + * tp_account_ensure_and_handle_channel_finish: > ... > > + * Returns: %TRUE if the channel was successful created and you are handling > > + * it, otherwise %FALSE > > "successfully created" (also in the _create_ version). fixed. > I think this docstring should also remind the API user about NotYours (just the > first sentence of the above, perhaps). done. > In request_and_handle_channel_cb, if ctx->cancellable has already been > cancelled when you call g_cancellable_connect (in practice this means it was > cancelled while the Create/EnsureChannel call was in-flight), then you'll call > Cancel() followed by Proceed(). You can detect that by only calling Proceed if > (ctx->cancellable == NULL || ctx->cancel_id != 0), I think? I just added an early return if g_cancellable_is_cancelled() returns TRUE. > > + /* Object has ben properly removed so ChannelRequest succeeded */ > > "has been removed without error, so" fixed. > Why not do this, which has one less g_error_new? > > GError e = { domain, code, message }; > > if (domain == TP_ERRORS && code == TP_DBUS_ERROR_OBJECT_REMOVED) > { > ... > } > ... > request_ctx_fail (ctx, &e); Good point; done. > > + DEBUG ("Proceed sucess; waiting for the channel to be handled"); > > "Proceed succeeded; waiting" fixed. > > + /* Request succeed */ > > "Request succeeded" fixed. > > + "TpGlibTempHandler", TRUE, handle_channels, ctx, NULL); > > It's not all that temporary any more? I think I'd prefer > "TpGLibRequestAndHandle". done.
> { > ctx->cancel_id = g_cancellable_connect (ctx->cancellable, > G_CALLBACK (operation_cancelled_cb), ctx, NULL); > + > + /* We just aborted the operation so we're done */ > + if (g_cancellable_is_cancelled (ctx->cancellable)) > + return; > } Be careful with this sort of thing - part of the point of GCancellable is that it can be cancelled at any time, even from another thread. I think you're actually correct here, as it happens - if the cancellable is cancelled after g_cancellable_connect releases the lock but before g_cancellable_is_cancelled checks it, then operation_cancelled_cb has been scheduled to run in an idle in our thread, there's no point in calling Proceed, and operation_cancelled_cb will run shortly after this early-return.
> +gboolean tp_account_create_and_handle_channel_finish (TpAccount *account, > + GAsyncResult *result, > + TpChannel **channel, > + GError **error); Shouldn't these be TpChannel *blahblah_finish (TpAccount *, GAR *, GE **)? We only need to return a boolean if the method is conceptually "void but can raise exceptions". (Precedent: g_async_initable_new_finish, g_data_input_stream_read_line_finish, g_file_input_stream_query_info_finish, etc.)
The tests look fine. A missing bit of API here that I think clients will want in practice: if the channel is re-requested and HandleChannels is called on the hidden handler, something ought to emit a signal to the API user so they can put their window in the foreground. We only give the API user a TpChannel at the moment, and bolting an unrelated and non-guaranteed signal onto TpChannel seems wrong, so we'd have to introduce a new object: TpChannel *tp_account_create_and_handle_channel_finish (TpAccount *account, GAsyncResult *result, TpHandlerNotifier *notifier, GError **error) where notifier may be NULL, and if not, *notifier is set to a trivial object that just emits a signal every time the channel is re-handled? The signal that's emitted on re-handling ought to contain the TpHandleChannelsContext, so the handler can look at the TpHandleChannelsContext:handler-info if it wants to. Perhaps the _finish function should also optionally output a TpHandleChannelsContext?
I have implemented that in the branch. I didn't implement the queuing of HandleChannels() calls as that seems overkill to me. If you really care about those you should just call the _finish function in the callback (which is what we always do anyway).
Over in Bug #29218 I wondered whether this API needs to be encapsulated as an object (which would essentially be the RequestCtx, turned into a public boxed object or GObject); if we introduce TpChannel subclasses, we need somewhere to put the desired GType. Similarly, if we add API to TpBaseClient for "don't bother giving me channels until they have FEATURE_FOO prepared", this API in its current form will get left behind. Straw man: TpAccountChannelRequest *tp_account_channel_request_new (TpAccount *, GHashTable *); void tp_account_channel_request_set_desired_type (TpAccountChannelRequest *, GType); void tp_account_channel_request_require_feature (TpAccountChannelRequest *, GQuark); void tp_account_channel_request_ensure_async (... async stuff here ...); /* usage */ acr = tp_account_channel_request_new (account, asv); tp_account_channel_request_set_desired_type (acr, MY_TYPE_CHANNEL_SUBCLASS); tp_account_channel_request_require_feature (acr, TP_CHANNEL_FEATURE_BEES); tp_account_channel_request_require_feature (acr, MY_CHANNEL_FEATURE_GOATS); tp_account_channel_request_ensure_async (...); /* off we go! */
I created a new branch (partially based on the old one) implementing this new API: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-creation-13422-awesome
> @@ -13,3 +13,4 @@ VOID:OBJECT,UINT,INT,STRING > VOID:OBJECT,OBJECT > VOID:OBJECT,STRING,STRING > VOID:VOID > +VOID:INT64,OBJECT I'd prefer these kept in alphabetical order. Other than that, this does indeed look considerably more awesome :-) I'll have another look through the whole branch to confirm, but I think this is basically ready to go.
(In reply to comment #25) > > @@ -13,3 +13,4 @@ VOID:OBJECT,UINT,INT,STRING > > VOID:OBJECT,OBJECT > > VOID:OBJECT,STRING,STRING > > VOID:VOID > > +VOID:INT64,OBJECT > > I'd prefer these kept in alphabetical order. done. > I'll have another look through the whole branch to confirm, but I think this is > basically ready to go. Hourrah \o/ Should I open another bug for the "request and forget" case or do you prefer to continue using this one?
(In reply to comment #26) > Should I open another bug for the "request and forget" case or do you prefer to > continue using this one? A new bug's easier, I think; I'll clone one.
Humm maybe the "re-handled" signal should pass the TpChannel as well for extra convenience?
I've added 5 new commits coming from my "request and forget" branch but which could be merged with this branch.
The new patches look good, and you've pre-empted a request I was about to make, which was "return_if_fail that only one async method is called" :-) (In reply to comment #28) > Humm maybe the "re-handled" signal should pass the TpChannel as well for extra > convenience? That'd be nice; emit the exact same instance. Re-review pass: Don't bother now unless you're feeling really keen, but in future I'd prefer it if new static function names indicated their context a bit better, for easier debugging (tp_account_channel_request_channel_invalidated_cb or account_channel_request_invalidated_cb or acr_channel_invalidated_cb or something, not just channel_invalidated_cb.) This is particularly important if they DEBUG(), since the G_STRFUNC goes in the debug message. > + * TpAccountChannelRequest:user-action-time: ... > + * This property can't be %NULL. No, no it can't, and that's because it's not a pointer :-P > + * tp_account_channel_request_get_account: > ... > + * Returns: the value of #TpAccountChannelRequest:account The default 'transfer' for return values is 'full', so this needs to be "Returns: (transfer none): the value..." > + * tp_account_channel_request_get_request: Also Returns: (transfer none): ... If you like, you can also add Returns: (transfer full): to the functions that do return a new ref, to make it explicit (g-i can guess, but the annotation also gets into the gtk-doc, where it might be useful to remind someone). tp_account_channel_request_new should technically also be G_GNUC_WARN_UNUSED_RESULT, although hopefully nobody is that stupid! :-)
> (In reply to comment #28) > > Humm maybe the "re-handled" signal should pass the TpChannel as well for extra > > convenience? > > That'd be nice; emit the exact same instance. done. > Re-review pass: > > Don't bother now unless you're feeling really keen, but in future I'd prefer it > if new static function names indicated their context a bit better, for easier > debugging (tp_account_channel_request_channel_invalidated_cb or > account_channel_request_invalidated_cb or acr_channel_invalidated_cb or > something, not just channel_invalidated_cb.) This is particularly important if > they DEBUG(), since the G_STRFUNC goes in the debug message. I prefixed functions using DEBUG() with acr_ > > + * TpAccountChannelRequest:user-action-time: > ... > > + * This property can't be %NULL. > > No, no it can't, and that's because it's not a pointer :-P oops :) fixed. > > + * tp_account_channel_request_get_account: > > ... > > + * Returns: the value of #TpAccountChannelRequest:account > > The default 'transfer' for return values is 'full', so this needs to be > "Returns: (transfer none): the value..." fixed. > > + * tp_account_channel_request_get_request: > > Also Returns: (transfer none): ... fixed. > If you like, you can also add Returns: (transfer full): to the functions that > do return a new ref, to make it explicit (g-i can guess, but the annotation > also gets into the gtk-doc, where it might be useful to remind someone). done. > tp_account_channel_request_new should technically also be > G_GNUC_WARN_UNUSED_RESULT, although hopefully nobody is that stupid! :-) added.
Merged to master \o/
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.