[Bug 34228] Implement API for requesting channels and handling them yourself

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=34228

--- Comment #5 from Olli Salli <ollisal at gmail.com> 2011-03-03 05:16:03 PST ---
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).

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.