[Telepathy] tp-qt4 Observer, Approver, Handler high-level API proposal

Simon McVittie simon.mcvittie at collabora.co.uk
Fri Apr 3 10:13:28 PDT 2009


On Fri, 03 Apr 2009 at 13:00:55 -0300, Andre Moreira Magalhaes wrote:
>     void setOperationFinished(uint operationHandle);
>     void setOperationFinishedWithError(uint operationHandle,
>             const QString &errorName, const QString &errorMessage);

Ewwwwww. We have opaque typed pointers. Can we please use them? Even if they're
secretly just pointer-sized integers that have been cast to Something*.

I'm not convinced that the base class is hugely useful - the meaning of
returning from the initial D-Bus method varies, and we should have a method
name that indicates what it's doing.

>     virtual void observeChannels(uint operationHandle,
>             const AccountPtr &account,
>             const ConnectionPtr &connection,
>             const QList<ChannelPtr> &channels,
>             const QString &dispatchOperationObjectPath,

I think you're right to just give the library user the object path here -
at this point in the process it'll mostly be used as an identifying token.

In the final version of Observer we expect to have a list of requests being
satisfied by these channels, too (so, a QStringList).

>             const QVariantMap &observerInfo) = 0;

It'd be good if there was some way to access the contents of this dict in a
better way, as we define meanings for bits of it; maybe the virtual method
could call back into the base class somehow.

When observing channels, returning from ObserveChannels indicates that you
have done all your initial setup and are now ready for those channels to be
handled by a Handler (e.g. a logger would connect to MessageReceived and
download the message backlog before saying "yes"). As a result, it needs to
be able to happen asynchronously.

>     virtual void addDispatchOperation(uint operationHandle,
>             const QString &dispatchOperationObjectPath,
>             const QVariantMap &dispatchOperationProperties) = 0;

This should take a DispatchOperationPtr to a dispatch operation that is
guaranteed to already be ready (the QVariantMap should be enough to ensure
that).

It's not yet clear to me what returning from AddDispatchOperation signifies,
or whether it needs to be asynchronous.

>     virtual void handleChannels(uint operationHandle,
>             const AccountPtr &account,
>             const ConnectionPtr &connection,
>             const QList<ChannelPtr> &channels,
>             const QStringList &requestsSatisfiedObjectPaths,
>             const QDateTime &userActionTime) = 0;

What does Qt use to do focus-stealing prevention in its equivalent of
gtk_window_present_with_time()? That's the purpose of UserActionTime, so
whatever representation Qt wants, that's what userActionTime should be.

Returning successfully from HandleChannels means you have accepted your
responsibility to handle the channels. Returning an error means you are unable
to handle them, and the ChannelDispatcher should close them or fail over to
another Handler or something. I'm not yet sure whether we need to allow
the library user to make HandleChannels fail, and whether this needs to be
asynchronous.

>     virtual void addRequest(const QString &requestObjectPath,
>             const QVariantMap &requestProperties) = 0;

This should probably take a ChannelRequestPtr that's guaranteed to be ready?

Perhaps we could have some sort of opt-in mechanism so only clients that want
to care about requests will get this method called, so they can avoid the
overhead of creating the ChannelRequest?

Returning successfully or unsuccessfully from AddRequest is basically
irrelevant, if I remember the spec correctly: the ChannelDispatcher will
probably just emit debug output if you failed. Accordingly, I think we can
return from the D-Bus method as soon as the virtual method returns, and not
give the user a way to make it fail.

>     virtual void removeFailedRequest(const QString &requestObjectPath,
>             const QString &error, const QString &message) = 0;

Returning successfully or unsuccessfully is like AddRequest, IIRC.

Probably the BaseHandler should cache a ChannelRequestPtr for each request that
we've been told about and that has not either been satisfied or removed,
so we could get a ChannelRequestPtr here that can be compared for equality
with the one from addRequest.

> public:
>     // clientName is the name to be used
>     ClientRegister(const QString &clientName, bool unique = false,
>             QObject *parent = 0);
>     ~ClientRegister();
> 
>     QString clientName() const;
> 
>     bool addObserverClient(ObserverClient *client);
>     bool addApproverClient(ApproverClient *client);
>     bool addHandlerClient(HandlerClient *client);
> 
>     bool registerClients();

I'm not sure whether we should block on RequestName like this. Probably we
should, since it's only a round trip to the dbus-daemon.

> map.insert("org.freedesktop.Telepathy.Channel.ChannelType",
>            "org.freedesktop.Telepathy.Channel.Type.Text");
> map.insert("org.freedesktop.Telepathy.Channel.TargetHandleType",
>            Telepathy::HandleTypeContact);
> h->setFilters(map);

Can't we have this happen at construct time?

> TODO:
> - Write wrappers for ChannelRequest.DRAFT and ChannelDispatchOperation.DRAFT

Yes. My current work on Mission Control should get these D-Bus APIs closer to
being final.

Regards,
    Simon


More information about the telepathy mailing list