Plasma::Service API review

[Kevin Ottens]

Le Thursday 15 May 2008, Aaron J. Seigo a écrit :
> attached is an API draft for Plasma::Service. i'll let the apidox do the
> talking. questions and criticisms desired.

OK, I'll just paste the content of the file in my reply after removing all the 
apidox from there (I basically kept only the class definition itself), and 
add my comments inline. As usual, I'll assume that I should be able to 
understand the class interface without the apidox, and that the apidox is 
only here to provide extra information or confirm what I'm looking at.

As a preliminary note, I'll just examine the class description itself from the 
apidox (and on purpose that's the only piece of apidox I'll detail):
> @brief This class provides a generic API for write access to settings
> or services.

I find it kind of odd that "settings" appear here, since the class name itself 
is "Plasma::Service", that said that probably explains why KConfigGroup got 
through. :-)

> Plasma::Service allows interaction with a "destination", the definition
> of which depends on the Service itself. For a network settings Service
> this might be a profile name ("Home", "Office", "Road Warrior") while a
> web based Service this might be a username ("aseigo", "stranger65").

A "profile" or a "username" are "destinations"? Sounds odd to me. In 
particular the "username" as destination example for a web service doesn't 
feel natural at all to me (you'd definitely expect a url for this one).

> A Service provides one or more operations, each of which provides some
> sort of interaction with the destination. Operations are described using
> config XML which is used to create a KConfig object with one group per
> operation. The group names are used as the operation names, and the defined
> items in the group are the parameters available to be set when using that
> operation. 

Reading this, I'm kind of wondering if the API shouldn't be splitted. It looks 
like it's talking both to applet writers (who use Service) and service 
writers. They're probably not the same audience, and as an applet writer I'd 
run away from this class after reading this (from the applet writer point of 
view it looks too much like implementation details... he just want to call a 
service operation). To me it looks like KConfigGroup is overkill there (again 
for applet writers) I'd expect at most a QMap<QString, QVariant> for the 
parameters.

> A service is started with a KConfigGroup (representing a ready to be
> serviced operation) and automatically deletes itself after completion
> and signaling success or failure. See KJob for more information on this
> part of the process.
>
> Services may either be loaded "stand alone" from plugins, or from a
> DataEngine by passing in a source name to be used as the destination.

I think that the vocabulary might need to be reevaluated. To me service would 
relate to Service Oriented Architecture (SOA) and in such a case a service is 
a collection of operations. Of course the same service can be running at 
several places, and then you identify which one you're calling using an 
address.

That's why I think that "destination" should be changed to "address". Also 
sometimes you're talking about "profiles" which seems kind of confusing to me 
(at least it's confusing me).

> Sample use might look like:
>
> Plasma::DataEngine *twitter = dataEngine("twitter");
> Plasma::Service *service = twitter.serviceForSource("aseigo");
> KConfigGroup op = service->operation("update");
> op.writeEntry("tweet", "Hacking on plasma!");
> service->start(op);

Just to place a call that looks weird to me. I mean, once the call is done, my 
Plasma::Service instance is deleted. It effectly only did _one_ call to a 
service. I'd be tempted to say that this class is Plasma::ServiceCall then 
and not Plasma::Service.

Without looking at the class interface (yet) I'd think this naming issue 
probably relates to one of my remarks above: it's trying to be both an api 
for the applet writers (the onesplacing calls on services) and for the applet 
writers (who need to have the necessary to describe what a service exposes).
--------------------

That's it for the class description, I guess that explains where I'm coming 
from when I take a closer look at the class interface itself.

--------------------
> class Service : public KJob

Well, it's now obvious that I'd propose to rename to ServiceCall or ServiceJob 
(I probably favor ServiceCall though, since it's placing a call on a service 
operation).

> {
> public:
>     explicit Service(QObject *parent = 0);

What is this one doing? Do I get an "invalid" Service? What would be the point 
of such a thing? (it's public, I'm assuming it's interesting to applet 
writers then, it seems it shouldn't be).

>     Service(QObject *parent, const QVariantList &args);

I guess this one is because of K_PLUGIN_FACTORY... I'm always wondering if 
there's a way to make those ones protected or hidden better. Might be another 
indication for splitting the class though.

>     ~Service();

Fine with me. I'd tend to repeat the "virtual" here (that's really a matter of 
giving more information to the reader).

>     static Service* load(const QString &name, QObject *parent = 0);

Now confusing, as an applet writer which one am I supposed to use? This one, 
or the constructors?

>     void setDestination(const QString &profile);

setDestination() should probably not take a "profile" as argument. I'd propose 
a setAddress(address) instead. Although if we split Service such a setter 
might become useless.

>     QString destination() const;

s/destination/address/

>     QStringList operations() const;
>     KConfigGroup parameters(const QString &operation);

That's really the part where we mix services and calls to services.

s/operations/operationsName/? (you're returning only the names, not the 
operations themselves).
s/parameters/operationParameters/?

>     void start(const KConfigGroup &parameters);
> 
>     QString name() const;
>     QVariant result() const;

This one made me wonder if that'd make sense in KJob directly... On the other 
hand not all the classes inheriting from KJob would need it.

Now are we sure we want that at all? That kind of encourage people to use the 
job synchronously (using exec(), which means blocking) without further 
thinking.

> protected:
>     void setResult(const QVariant &result);
>     void setOperationsScheme(QIODevice *xml);

Cries "service writers" not "applet writers" IMO. OK, it's protected, so 
applet writers can't mess with it, but that's also what forces to have the 
operations() and parameters() here.

For very simple services another method taking an xml document or a string 
could be handy.

s/setOperationsScheme/setScheme/?
s/setOperationsScheme/setServiceScheme/?

>     virtual void registerOperationScheme();

Really unclear to me. Actually, I guess I'm almost figuring out what it does 
after two reads, and I'm still unsure. Maybe I'm just too dumb I don't know.

That somehow looks like a hook called at initialization time so that I have 
the opportunity to call setOperationScheme()?

>     virtual void start();

This one sounds like your own reimplementation from KJob, hidden as protected. 
Kind of dangerous since service writers could screw this one... Making we 
should come up with a way to decouple this.

>     void setName(const QString &name);
> 
> private:
>     class Private;
>     Private * const d;
> };
--------------------

OK, now my proposal for the apidox example if Service and ServiceCall were 
splitted:
Plasma::DataEngine *twitter = dataEngine("twitter");
Plasma::Service *service = twitter.serviceForSource("aseigo");
QMap<QString, QVariant> args;
args["tweet"] = "Hacking on plasma!";
Plasma::ServiceCall *call = service->createCall("update", args);
call->start();

Slightly more verbose but I see three major benefits:
 - better separation of concerns
 - a service object is actually reusable
 - tied to the previous one somehow, it makes the api looks a bit more like an 
API to deal with SOA

As a last (maybe crazy) remark, we might want to make it look even closer to 
something like QtDBus on the applet writers side since that somehow shares 
some concept. The major differences would be the use of KJob to make it 
asynchronous.

Regards.
-- 
Kévin 'ervin' Ottens, http://ervin.ipsquad.net
"Ni le maître sans disciple, Ni le disciple sans maître,
Ne font reculer l'ignorance."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080515/20654442/attachment.pgp