[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 #6 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-03-03 05:38:53 PST ---
(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

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