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

[Andre Moreira Magalhaes]

Simon McVittie wrote:
> 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 don't see the need to that, we can typedef OperationHandle uint if 
that's more pleasant.

> 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.
>
>   
The base class is useful in the sense that we don't replicated the 
filters methods in all subclasses, it's just a thin class and does not 
hurt anyone.

Let me elaborate on asynchronous and finished/finishedWithError usage:

All D-Bus methods can fail, some race condition for example, so enabling 
them to fail is not something we should avoid IMHO.
Also the spec for Observer/Approver/Handler is still DRAFT, with some 
FIXME/TODO's items, and the idea is that it can change at any time, so I 
decided to make all methods able to fail, in case we add error support 
in the future.

Regarding async usage:
Some methods probably don't need to be async, but we never know what the 
app will do in order to implement the method, so allowing the app to 
implement the method async is OK. Also it's better to make all methods 
call finished/finishedWithError when finished then doing it case by 
case, so we have a pattern.
This will be documented, and I don't see the reason not to do it.

>>             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.
>   
ObserverInfo class?

> 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.
>
>   
Explained above.

>>     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).
>
>   
DispatchOperation class is on the TODO list.

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

>>     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.
>
>   
Need to check.

> 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.
>
>   
Explained above.

>>     virtual void addRequest(const QString &requestObjectPath,
>>             const QVariantMap &requestProperties) = 0;
>>     
>
> This should probably take a ChannelRequestPtr that's guaranteed to be ready?
>
>   
ChannelRequest class is on the TODO list.

> 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?
>   
I will add enableListenRequests(bool)

> 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.
>
>   
Explained above.
>>     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.
>
>   
Yep, when ChannelRequest appear, a cache will be useful.

>> 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.
>
>   
I don't see the need to make registerClients async, but if you prefer it 
can return a PendingOperation.

>> 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?
>
>   
Yeah, we can, But I was thinking if we will be able to add filters at 
runtime.

We have 2 solutions:
- Receive the filters in the constructor
- Have a addFilters instead of setFilters

Which one do you prefer?

>> 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.
>
>   
Nice :)

BR
Andrunko