Bug 34228

Dependency because we don't want to declare the new {create,ensure}AndHandle methods without a hints param in the first place.

Ok, so my 2 cents on how this should be implemented follow.

So, the requirements boil down to:
 1) It's possible to request a channel from the Channel Dispatcher using a Tp::Account and handle it without explicitly being a Handler - as easily as one could (incorrectly) request a channel directly from the Connection
 2) The resulting channel, contacts in it and any connection proxy created for the channel should all be pre-existing instances on the Account's factories, or if there isn't any, created and made ready accordingly to the factory settings
 3) We need to still adhere to the spec, in particular:
   a) the Client.Handler.HandledChannels property needs to always have the channels handled by any Handler on the same D-Bus connection / unique name (note that this is not service / client name specific, but *unique* connection name). I think this is already guaranteed via a static map in the Handler adaptor though, but check for this in the unit test please.
   b) if somebody else ensures a channel matching one we're already handling via this API, the CD will invoke HandleChannels again to tell us to bring our window/tab/whatever for the channel to the foreground
   c) we shouldn't affect any other Handlers by e.g. hijacking Channels they're expecting

Not requiring being a Handler (requirement 1) implies that you don't necessarily have to be any kind of Client at all, which would imply we don't want to require an existing ClientRegistrar to be passed to this API, or to be in existence at all.

Also, the factory requirement (requirement 2) means that even though there would be a ClientRegistrar in existence already, we couldn't necessarily use it, as it might have different factories from the ones in the Account (which is sadly true for many existing applications which haven't grasped the awesomeness of the ClientRegistrar::create(AccountManager) method yet).

Therefore, we have to create a ClientRegistrar internally ourselves for the purposes of just requesting and handling this single channel. This is fine efficiency-wise: Creating a ClientRegistrar is actually almost free as it doesn't do any D-Bus traffic or connect to any signals etc. Being able to do this is why I wanted us to drop the ClientRegistrar singleton guarantee for tp-qt 0.5.

So, the request and handle PendingOperation would first of all create a ClientRegistrar, then generate a unique client name for that request only (suggestion: use the PID and a static variable incremented for each request), and register a Handler with an empty filter on the created ClientRegistrar. Empty filter, because PreferredHandler bypasses handler filters, and we don't want any other channels (req 3c).

However, creating a ClientRegistrar currently requires one to pass in an AccountFactory. This is not really needed for our case, as the only account ever required to be created is the Account the request is made on. Hence I think the cleanest way to do this, rather than having a separate code path inside ClientRegistrar for having an AccountFactory or not, would be to just make a fake AccountFactory which just always returns the Account the request was made through (but compares the object paths and warns if they differ or something, this'd indicate some madness in the Channel Dispatcher with it giving us a channel for a different account).

Having done that, the request and handle PendingOp would then Proceed the ChannelRequest, just as for a normal "request and forget" request.

At some point the channel comes in. Our hidden ClientRegistrar and Handler then construct the relevant Channel and Connection proxy for us according to the factory settings. At this point, we should finish, and give the resulting Channel to whoever requested it.

However, the fun doesn't stop there. To be able to fulfill req 3b, we need to continue being a Handler with the same client name for as long as the channel is in existence. We also need some way to signal the application about the reinvocation.

Therefore, we need an additional QObject, let's call it HandledChannelNotifier or somesuch, which just has a handledAgain() signal, and which is kept alive as long as the Channel is alive (and not invalidated). As long as the notifier is alive, it should keep the handler registered too. So, it should receive the shared pointers to the ClientRegistrar and the ownership of the Handler from the original request and handle PendingOperation when it finishes.

Needless to say, no matter if it takes quite a few hours, this all needs a proper regression test :) One should obviously cut corners in implementing the fake channel dispatcher required for this, though. The one in the AccountChannelDispatcher test shouldn't require too much modification, except for testing the re-handling case (req 3b).

(In reply to comment #2)

> Having done that, the request and handle PendingOp would then Proceed the
> ChannelRequest, just as for a normal "request and forget" request.
> 

Well, obviously it would have to also actually request the channel before doing this, passing our full generated Handler client name as the PreferredHandler, and do the normal ChannelRequest proceed dance etc.

Note that there are multiple failure cases: The ChannelRequest may fail, for example. Also our handler getting a channel for some other Account than the one the request was on is also a failure pretty much - there's something crazy going on in the CD, and we shouldn't rather keep waiting for the "correct" one to arrive - as likely it won't arrive, and we'll just keep our pendingop not finished forever.

Andre has a work-in-progress branch at the URL. Review following.

The internal handler class
==========================

SimpleHandler is a rather generic name. We might even want to use it in the future as a common base class name for a family of Handler classes for easy handling of text, voice and especially tube channels I have some ideas for. So let's try to be a bit more specific and indicate its particular use: RequestTemporaryHandler?

+
+    bool bypassApproval() const { return true; }
+

Handlers we request ourselves never go through the approvers anyway. And this handler shouldn't get any channels we didn't request - hence I'd rather make this always false to leave slightly less room for the CD to get confused and give some channel we didn't request to us, without even asking an approver first. Though if the CD isn't confused it shouldn't really matter - our filter is empty anyway.

+    void channelReceived(const Tp::ChannelPtr &channel);
+

You need to relay at least the user action time so that whoever's using the API can decide whether to display their window/tab for it when it's re-requested and given to it again. Also, at least the hints from the ChannelRequest should be given (a ready ChannelRequest will be given in the requestsSatisfied param for every new request) so that they don't get lost.

wjt has proposed an interesting usecase for hints for requested text channels in particular: you could use them to invoke the text chat UI, or whoever is currently handling a text chat to a particular contact, for filling a particular message to be sent but still leaving the user a way to edit it before sending in the text UI. This could be used for e.g. sharing links: you'd press "share link to X" from a context menu in your browser, and a text chat window would pop up OR a previously open text chat window to that contact would be focused, allowing you to describe the link you're sharing.

Note that while notifications on somebody else requesting the channel you're currently handling through the Request and Handle API should give the hints to you, it's fine for us to disallow passing any hints to the original request made to the Request and Handle API: any successful request and handle will result in the handling being done by the requester by definition, so the hints would be just passing data uselessly from the requester back to it through the CD.

+    if (account != mAccount) {
+        emit error(TP_QT4_ERROR_SERVICE_CONFUSED,
+                QLatin1String("Account received is not the same as the account which made the request"));
+        context->setFinishedWithError(TP_QT4_ERROR_SERVICE_CONFUSED,
+                QLatin1String("Account received is not the same as the account which made the request"));
+        return;
+    }
+

I think you should set the context finished with error in this case. You're not handling (incl. closing) the channel, although the CD assumes you are because you return successfully.

+    if (mChannel) {
+        Q_ASSERT(mChannel == channels.first());
+        mChannel = channels.first();
+    }
+

An assert here is too strict. It could be triggered by a race where our previous channel gets invalidated for some reason and then we yet get it passed to us as a Handler (this can happen because the invalidation events come from the CM, but the handling is invoked from the CD, which are different processes and can run in an arbitrary order). The ChannelFactory would construct a new proxy in this case, because the old one was invalidated.

Even more so, having such an assert enables anybody to crash any application using the TpQt4 request and handle API by calling HandleChannels on it with a different channel object path. Not very cool.

So I'd make this similar to the "wrong account" case as well: finish the handling context unsuccessfully and log a warning. We've succesfully got the first channel already and given it to the API user though so it's not an error from the request and handle API user's perspective, hence we shouldn't propagate the error here in that direction. Actually, we should only emit the error signal for the Account case as well if we didn't successfully receive a Channel yet.

+    Q_ASSERT(channels.size() == 1);
Asserting is too strict here as well. Enables crashing us from bogus calls received from the bus.

As a general resilience paradigm, asserts should be only used as sanity checks for conditions which only depend on tp-qt4 internals: never on anything affected by other D-Bus users or the tp-qt4 API user.

The channel handled again notifier
==================================

+    void finished();
What's this signal used for? It seems to be emitted when the channel is invalidated. Is that really "finishing"? I'd perhaps rather just document handledAgain() such that it may be emitted again and again as long as the Channel is valid, and won't be afterwards, with no separate destruction signal. Note that there's already 2 signals happening at that point: Channel invalidated() and HandledChannelNotifier::destroyed() from the QObject base class.

+    void handledAgain();
+

This signal should relay the user action time and request hints, as described earlier.

+            SIGNAL(hanldedAgain()));

This is not right :)

Btw, wouldn't currently a successfully finishing request and handle, but not recovering (and eventually closing or otherwise invalidating) the resulting Channel leak the ClientRegistrar, the Handler and most importantly the Channel itself? (Because the HandledChannelNotifier holds refs to them). This isn't cool. Make only the accessor on PendingChannel construct the notifier? If the accessor isn't used, nobody can't be listening to the handledAgain() signal on it anyway, and hence even if we continue being the Handler etc, nobody is going to get our notifications.

This arises another point: the application can only connect to handledAgain() once it has received the signal about the PendingChannel being finished (which is at least one mainloop iteration away from our Handler getting the channel for the first time due to PendingOperation emitting finished() through the mainloop). So, if we add useful information passed in the channel request hints and expect to get that in the (enhanced) handledAgain signal, there's a window during which we can lose it. Hence, I think we have to queue any re-handlings in the Handler initially (simple to do: the queue can be just of pairs of <user action time, hints> I think), and only emit them when the application has had the chance to connect to the Notifier. Do you see any other way to avoid this race?

The class needs documentation as to why it's needed, pointing out to the fact that anybody using the request and handle API to get a longer-lived interactive channel (vs a temporary internal one) should use the notifier as well.

PendingChannel additions
========================

+#include <TelepathyQt4/ChannelClassSpecList>

why?

+    AccountPtr construct(const QString &busName, const QString &objectPath,
+            const ConnectionFactoryConstPtr &connFactory,
+            const ChannelFactoryConstPtr &chanFactory,
+            const ContactFactoryConstPtr &contactFactory) const
+    {
+        return mAccount;
+    }

Warning please if the object path is wrong. Note that with this AccountFactory, your check for the wrong account in the Handler can never be hit: your factory always gives a pointer to that one account proxy, and the Handler compares the account pointers for inequality.

+    QString handlerName = QString(QLatin1String("x%1.%2"))
+        .arg((intptr_t) mPriv->handler.data(), 0, 16)

This is only ever unique in a single process. If we expect to be run in a system which has virtual memory, at least :) And using numHandlers already accomplishes that. So you must use the PID here, or your unique name on the bus, or something like that. Also please prefix it with TpQt4RaH or something to not conflict with others doing similar things - and to identify us better in debug logs.

 ConnectionPtr PendingChannel::connection() const
 {
-    return ConnectionPtr(qobject_cast<Connection*>((Connection*) object().data()));
+    return mPriv->connection;
 }

Please either make connection() return the current Connection of the Account for a request made on an account (consistent, through a bit useless), or document that it always returns NULL for requests made on accounts.

+    if (isFinished()) {
+        warning() << "Handler received the channel but this operation already finished due "
+            "to failure in the channel request";
+        return;
+    }
+

With the current Handler implementation, if somebody issues a request to our channel immediately following us, we are likely to get re-invoked in the handler while the PendingChannel still exists, resulting in this warning. This would be solved by queuing the reinvocations until they can be processed by the application as I mentioned earlier.

+    mPriv->yours = channel->isRequested();

Any channel we're supposed to get is ours - we requested it. If it isn't ours, ie. somebody else is handling it, we shouldn't be handling it.

+    Q_ASSERT(mPriv->channelType == channel->channelType());

Again asserting on data received from D-Bus.

+    if (isFinished()) {
+        // ignore possible errors when calling create/ensureChannel if we already finished
+        return;
+    }

So just silently munch the error? In particular, if we have finished successfully, this is a situation where sanity has been lost already: I'd emit a warning like "create/ensureChannel finished with a failure after the internal handler already got a channel".

Error-after-error (e.g. first error from the handler, then here) can be silent here though, as there is debug output for those failures already.

Account additions
=================

First of all, this is intended to be an API to make things EASIER. I expect this is in your plans too, and you just haven't got around to it yet, but we really need to have all of the convenience wrappers for requesting text, call, file transfer etc channels for the request and handle API too. Suggested plan of action: rather than copy-pasting the code building the request variant map from the current overloads, add static QVariantMap textChannelRequest(const QString &targetID) etc accessors (these could actually semi-usefully be public, but perhaps in a low-level section?), and make both the current request methods and the new request and handle ones use them.

+ * or to cancel it.
How? I don't see any way to do this from the PendingChannel.

+ * \param hints Arbitrary metadata which will be relayed to the handler if supported,
+ *              as indicated by supportsRequestHints().
...
+        const ChannelRequestHints &hints)

As mentioned earlier, passing hints to yourself is just an expensive way to pass internal context through the CD. Also, we wouldn't want any other handler getting those hints, even if our request and handle failed because somebody else was already handling the channel. So better to just leave hints out from these.

+ * \return A PendingChannel which will emit PendingChannel::finished
+ *         when the call has finished and can be used to get the channel to handle.

We discussed this for your other branch already, but something like "successfully, when the Channel is available for handling using PendingChannel::channel(), or with an error if one has been encountered" would be more descriptive here too, right?

You should also document how create and ensure can fail: in particular, that createAndHandle will fail with NOT_AVAILABLE if a conflicting channel already exists, and ensureAndHandle will fail with NOT_YOURS if somebody else is already handling the channel. (And of course, this is not to dispute the fact that both can fail due to a myriad other reasons - just to be clearer about what happens in these cases, as especially for ensureAndHandle that is a very likely failure mode).

(In reply to comment #5)
> The internal handler class
> ==========================
> 
> SimpleHandler is a rather generic name. We might even want to use it in the
> future as a common base class name for a family of Handler classes for easy
> handling of text, voice and especially tube channels I have some ideas for. So
> let's try to be a bit more specific and indicate its particular use:
> RequestTemporaryHandler?
Good point, I was not sure how to name it, so came up with the easiest one :). Will change.
 
> +
> +    bool bypassApproval() const { return true; }
> +
> 
> Handlers we request ourselves never go through the approvers anyway. And this
> handler shouldn't get any channels we didn't request - hence I'd rather make
> this always false to leave slightly less room for the CD to get confused and
> give some channel we didn't request to us, without even asking an approver
> first. Though if the CD isn't confused it shouldn't really matter - our filter
> is empty anyway.
Makes sense, will do it.

> +    void channelReceived(const Tp::ChannelPtr &channel);
> +
> 
> You need to relay at least the user action time so that whoever's using the API
> can decide whether to display their window/tab for it when it's re-requested
> and given to it again. Also, at least the hints from the ChannelRequest should
> be given (a ready ChannelRequest will be given in the requestsSatisfied param
> for every new request) so that they don't get lost.
> 
> wjt has proposed an interesting usecase for hints for requested text channels
> in particular: you could use them to invoke the text chat UI, or whoever is
> currently handling a text chat to a particular contact, for filling a
> particular message to be sent but still leaving the user a way to edit it
> before sending in the text UI. This could be used for e.g. sharing links: you'd
> press "share link to X" from a context menu in your browser, and a text chat
> window would pop up OR a previously open text chat window to that contact would
> be focused, allowing you to describe the link you're sharing.
> 
> Note that while notifications on somebody else requesting the channel you're
> currently handling through the Request and Handle API should give the hints to
> you, it's fine for us to disallow passing any hints to the original request
> made to the Request and Handle API: any successful request and handle will
> result in the handling being done by the requester by definition, so the hints
> would be just passing data uselessly from the requester back to it through the
> CD.
I am confused here, you first said "the hints of ChannelRequest should be given in channelReceived", but then you say that it does not make sense to pass hints as we are the caller anyway. I agree that hints should not be exposed, either in Account::create/ensureAndHandle and channelReceived. Is there something I missed here?
 
> +    if (account != mAccount) {
> +        emit error(TP_QT4_ERROR_SERVICE_CONFUSED,
> +                QLatin1String("Account received is not the same as the account
> which made the request"));
> +        context->setFinishedWithError(TP_QT4_ERROR_SERVICE_CONFUSED,
> +                QLatin1String("Account received is not the same as the account
> which made the request"));
> +        return;
> +    }
> +
> 
> I think you should set the context finished with error in this case. You're not
> handling (incl. closing) the channel, although the CD assumes you are because
> you return successfully.
Well, check the code again, it's calling setFinishedWithError, so I don't get this.

> +    if (mChannel) {
> +        Q_ASSERT(mChannel == channels.first());
> +        mChannel = channels.first();
> +    }
> +
> 
> An assert here is too strict. It could be triggered by a race where our
> previous channel gets invalidated for some reason and then we yet get it passed
> to us as a Handler (this can happen because the invalidation events come from
> the CM, but the handling is invoked from the CD, which are different processes
> and can run in an arbitrary order). The ChannelFactory would construct a new
> proxy in this case, because the old one was invalidated.
> 
> Even more so, having such an assert enables anybody to crash any application
> using the TpQt4 request and handle API by calling HandleChannels on it with a
> different channel object path. Not very cool.
> 
> So I'd make this similar to the "wrong account" case as well: finish the
> handling context unsuccessfully and log a warning. We've succesfully got the
> first channel already and given it to the API user though so it's not an error
> from the request and handle API user's perspective, hence we shouldn't
> propagate the error here in that direction. Actually, we should only emit the
> error signal for the Account case as well if we didn't successfully receive a
> Channel yet.
Ok, I will remove the assert and warn there.

> +    Q_ASSERT(channels.size() == 1);
> Asserting is too strict here as well. Enables crashing us from bogus calls
> received from the bus.
Same.
 
> As a general resilience paradigm, asserts should be only used as sanity checks
> for conditions which only depend on tp-qt4 internals: never on anything
> affected by other D-Bus users or the tp-qt4 API user.
> 
> The channel handled again notifier
> ==================================
> 
> +    void finished();
> What's this signal used for? It seems to be emitted when the channel is
> invalidated. Is that really "finishing"? I'd perhaps rather just document
> handledAgain() such that it may be emitted again and again as long as the
> Channel is valid, and won't be afterwards, with no separate destruction signal.
> Note that there's already 2 signals happening at that point: Channel
> invalidated() and HandledChannelNotifier::destroyed() from the QObject base
> class.
I did this similar to PendingOperation, where it emits finished and then deleteLater, but in this case we could rely on the channel being invalidated as you pointed.

> +    void handledAgain();
> +
> 
> This signal should relay the user action time and request hints, as described
> earlier.
As as I said earlier, should we pass the hints or just the action time here?
 
> +            SIGNAL(hanldedAgain()));
> 
> This is not right :)
heh, I didn't get to tests yet :).
 
> Btw, wouldn't currently a successfully finishing request and handle, but not
> recovering (and eventually closing or otherwise invalidating) the resulting
> Channel leak the ClientRegistrar, the Handler and most importantly the Channel
> itself? (Because the HandledChannelNotifier holds refs to them). This isn't
> cool. Make only the accessor on PendingChannel construct the notifier? If the
> accessor isn't used, nobody can't be listening to the handledAgain() signal on
> it anyway, and hence even if we continue being the Handler etc, nobody is going
> to get our notifications.
Ok, I will change it to only construct the object on accessor.

> This arises another point: the application can only connect to handledAgain()
> once it has received the signal about the PendingChannel being finished (which
> is at least one mainloop iteration away from our Handler getting the channel
> for the first time due to PendingOperation emitting finished() through the
> mainloop). So, if we add useful information passed in the channel request hints
> and expect to get that in the (enhanced) handledAgain signal, there's a window
> during which we can lose it. Hence, I think we have to queue any re-handlings
> in the Handler initially (simple to do: the queue can be just of pairs of <user
> action time, hints> I think), and only emit them when the application has had
> the chance to connect to the Notifier. Do you see any other way to avoid this
> race?
Will check.

> The class needs documentation as to why it's needed, pointing out to the fact
> that anybody using the request and handle API to get a longer-lived interactive
> channel (vs a temporary internal one) should use the notifier as well.
Docs was in the plans.

> PendingChannel additions
> ========================
> 
> +#include <TelepathyQt4/ChannelClassSpecList>
> 
> why?
Don't remember. Will check if it is needed.
 
> +    AccountPtr construct(const QString &busName, const QString &objectPath,
> +            const ConnectionFactoryConstPtr &connFactory,
> +            const ChannelFactoryConstPtr &chanFactory,
> +            const ContactFactoryConstPtr &contactFactory) const
> +    {
> +        return mAccount;
> +    }
> 
> Warning please if the object path is wrong. Note that with this AccountFactory,
> your check for the wrong account in the Handler can never be hit: your factory
> always gives a pointer to that one account proxy, and the Handler compares the
> account pointers for inequality.
Warn will be it.

> 
> +    QString handlerName = QString(QLatin1String("x%1.%2"))
> +        .arg((intptr_t) mPriv->handler.data(), 0, 16)
> 
> This is only ever unique in a single process. If we expect to be run in a
> system which has virtual memory, at least :) And using numHandlers already
> accomplishes that. So you must use the PID here, or your unique name on the
> bus, or something like that. Also please prefix it with TpQt4RaH or something
> to not conflict with others doing similar things - and to identify us better in
> debug logs.
Good point, will change.

>  ConnectionPtr PendingChannel::connection() const
>  {
> -    return ConnectionPtr(qobject_cast<Connection*>((Connection*)
> object().data()));
> +    return mPriv->connection;
>  }
> 
> Please either make connection() return the current Connection of the Account
> for a request made on an account (consistent, through a bit useless), or
> document that it always returns NULL for requests made on accounts.
We are always setting mPriv->connection = channel->connection() when we get the channel. It will work if either the channel was requested using Account or Connection.
 
> +    if (isFinished()) {
> +        warning() << "Handler received the channel but this operation already
> finished due "
> +            "to failure in the channel request";
> +        return;
> +    }
> +
> 
> With the current Handler implementation, if somebody issues a request to our
> channel immediately following us, we are likely to get re-invoked in the
> handler while the PendingChannel still exists, resulting in this warning. This
> would be solved by queuing the reinvocations until they can be processed by the
> application as I mentioned earlier.
Will check.

> +    mPriv->yours = channel->isRequested();
> 
> Any channel we're supposed to get is ours - we requested it. If it isn't ours,
> ie. somebody else is handling it, we shouldn't be handling it.
:)
 
> +    Q_ASSERT(mPriv->channelType == channel->channelType());
> 
> Again asserting on data received from D-Bus.
Will remove.
 
> +    if (isFinished()) {
> +        // ignore possible errors when calling create/ensureChannel if we
> already finished
> +        return;
> +    }
> 
> So just silently munch the error? In particular, if we have finished
> successfully, this is a situation where sanity has been lost already: I'd emit
> a warning like "create/ensureChannel finished with a failure after the internal
> handler already got a channel".
> 
> Error-after-error (e.g. first error from the handler, then here) can be silent
> here though, as there is debug output for those failures already.
Ok, will change.
 
> Account additions
> =================
> 
> First of all, this is intended to be an API to make things EASIER. I expect
> this is in your plans too, and you just haven't got around to it yet, but we
> really need to have all of the convenience wrappers for requesting text, call,
> file transfer etc channels for the request and handle API too. Suggested plan
> of action: rather than copy-pasting the code building the request variant map
> from the current overloads, add static QVariantMap textChannelRequest(const
> QString &targetID) etc accessors (these could actually semi-usefully be public,
> but perhaps in a low-level section?), and make both the current request methods
> and the new request and handle ones use them.
I plan to add helper methods, yes, just didn't have time yet.
 
> + * or to cancel it.
> How? I don't see any way to do this from the PendingChannel.
Copy/paste.
 
> + * \param hints Arbitrary metadata which will be relayed to the handler if
> supported,
> + *              as indicated by supportsRequestHints().
> ...
> +        const ChannelRequestHints &hints)
> 
> As mentioned earlier, passing hints to yourself is just an expensive way to
> pass internal context through the CD. Also, we wouldn't want any other handler
> getting those hints, even if our request and handle failed because somebody
> else was already handling the channel. So better to just leave hints out from
> these.
Ok, check questions above about hints.

> + * \return A PendingChannel which will emit PendingChannel::finished
> + *         when the call has finished and can be used to get the channel to
> handle.
> 
> We discussed this for your other branch already, but something like
> "successfully, when the Channel is available for handling using
> PendingChannel::channel(), or with an error if one has been encountered" would
> be more descriptive here too, right?
> 
> You should also document how create and ensure can fail: in particular, that
> createAndHandle will fail with NOT_AVAILABLE if a conflicting channel already
> exists, and ensureAndHandle will fail with NOT_YOURS if somebody else is
> already handling the channel. (And of course, this is not to dispute the fact
> that both can fail due to a myriad other reasons - just to be clearer about
> what happens in these cases, as especially for ensureAndHandle that is a very
> likely failure mode).
Ok, good points, will change.

Tnx for the review, will update here when I am done with the changes

(In reply to comment #6)
> (In reply to comment #5)
> 
> > +    void channelReceived(const Tp::ChannelPtr &channel);
> > +
> > 
> > You need to relay at least the user action time so that whoever's using the API
> > can decide whether to display their window/tab for it when it's re-requested
> > and given to it again. Also, at least the hints from the ChannelRequest should
> > be given (a ready ChannelRequest will be given in the requestsSatisfied param
> > for every new request) so that they don't get lost.
> > 
> > wjt has proposed an interesting usecase for hints for requested text channels
> > in particular: you could use them to invoke the text chat UI, or whoever is
> > currently handling a text chat to a particular contact, for filling a
> > particular message to be sent but still leaving the user a way to edit it
> > before sending in the text UI. This could be used for e.g. sharing links: you'd
> > press "share link to X" from a context menu in your browser, and a text chat
> > window would pop up OR a previously open text chat window to that contact would
> > be focused, allowing you to describe the link you're sharing.
> > 
> > Note that while notifications on somebody else requesting the channel you're
> > currently handling through the Request and Handle API should give the hints to
> > you, it's fine for us to disallow passing any hints to the original request
> > made to the Request and Handle API: any successful request and handle will
> > result in the handling being done by the requester by definition, so the hints
> > would be just passing data uselessly from the requester back to it through the
> > CD.
> I am confused here, you first said "the hints of ChannelRequest should be given
> in channelReceived", but then you say that it does not make sense to pass hints
> as we are the caller anyway. I agree that hints should not be exposed, either
> in Account::create/ensureAndHandle and channelReceived. Is there something I
> missed here?

Read again with some thought please!

Why do we have HandledChannelNotifier in the first place? Because *somebody else* can ensure the same channel, resulting in us being re-invoked. In that case, we should receive the hints they passed in.

However, our *own* request shouldn't pass hints, as those would just at best come back to us (where we already have them). So, channelReceived() wouldn't have any hints for first emission, which should correspond to our request. Note that the application doesn't see that emission anyway: that happens before the PendingChannel is finished(). BUT it would have hints for any successive emissions, if hints were passed to the request leading to the channel being handled again.

Shortly put: We don't pass hints. But somebody else might, if they're using the "ensure and forget" API rather than this request and handle API. We should get their hints in that case.

> 
> > +    if (account != mAccount) {
> > +        emit error(TP_QT4_ERROR_SERVICE_CONFUSED,
> > +                QLatin1String("Account received is not the same as the account
> > which made the request"));
> > +        context->setFinishedWithError(TP_QT4_ERROR_SERVICE_CONFUSED,
> > +                QLatin1String("Account received is not the same as the account
> > which made the request"));
> > +        return;
> > +    }
> > +
> > 
> > I think you should set the context finished with error in this case. You're not
> > handling (incl. closing) the channel, although the CD assumes you are because
> > you return successfully.
> Well, check the code again, it's calling setFinishedWithError, so I don't get
> this.
> 

True, sorry about that - too quick reading :)

> >  ConnectionPtr PendingChannel::connection() const
> >  {
> > -    return ConnectionPtr(qobject_cast<Connection*>((Connection*)
> > object().data()));
> > +    return mPriv->connection;
> >  }
> > 
> > Please either make connection() return the current Connection of the Account
> > for a request made on an account (consistent, through a bit useless), or
> > document that it always returns NULL for requests made on accounts.
> We are always setting mPriv->connection = channel->connection() when we get the
> channel. It will work if either the channel was requested using Account or
> Connection.
> 

Well, that's strictly speaking true only once the operation is finished (when we get the channel). When a Channel is requested directly from the Connection, connection() will be non-NULL all the time - it'll be the Conn the request was made on.

Reviewed and merged to master. Will be in release 0.5.9 due today.