Plasma::Service API review

[Aaron J. Seigo]

On Thursday 15 May 2008, Kevin Ottens wrote:
> > @brief This class provides a generic API for write access to settings
> > or services.
>
> I find it kind of odd that "settings" appear here,

of course, there are many things that plasmoids access in a service-y way that 
are actually system settings.

> itself is "Plasma::Service", that said that probably explains why
> KConfigGroup got through. :-)

KConfigGroup simply provides an easy way to do a number of interesting things 
related to key/value pairs.

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

think of twitter. the destination is the username.
think of networking. one sets up profiles for commonly used settings.

for many traditional web services, you are quite right, however. plasma isn't 
just about web services of course. 

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

we'd end up with a set of QMaps, we'd want to know what the default values 
are, etc. i originally started out doing a QMap based implementation and 
discovered i was adding all sorts of API that actually mapped 1:1 with 
KConfigGroup.

it really ends up being pretty much exactly the same thing, just with a lot 
less API in Plasma::Service when KConfigGroup is used.

it also lets Services use config xml to define things.

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

so instead of "destination" you find "address" more natural?

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

see the new draft of the API which adresses this by providing a ServiceJob 
class.

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

> > {
> > 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).

it's interesting to DataEngine writers. it can probably be made protected, 
however.

> >     Service(QObject *parent, const QVariantList &args);
>
> I guess this one is because of K_PLUGIN_FACTORY...

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

personally i prefer to only mark newly virtual methods.

> >     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?

load, obviously. three times to say "make the constructors protected"? =P

> >     void setDestination(const QString &profile);
>
> setDestination() should probably not take a "profile" as argument. I'd

already fixed in the next draft.

> propose a setAddress(address)

'address' is very webby and doesn't make a lot of sense for non-webby things 
like "my Home network profile"

> >     QStringList operations() const;
> >     KConfigGroup parameters(const QString &operation);
>
> That's really the part where we mix services and calls to services.

no, this is all about getting information on the service. =) calling doesn' 
thappen till much later.

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

you get the object in the finished signals from KJob and can query there.

> > protected:
> >     void setResult(const QVariant &result);
> >     void setOperationsScheme(QIODevice *xml);
>
> Cries "service writers" not "applet writers" IMO. OK, it's protected, so

well .. yes. this class is written by service writers and used by applet 
writers.

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

that would save a couple of lines of code perhaps, yes.

> >     virtual void registerOperationScheme();
>
> Really unclear to me.

this is called when the service should register the operation scheme, e.g. 
call setOperationsScheme(QIODevice *xml). the reason it's done this way is 
that QIODevice is not copyable, so i couldn't just have a QIODevice 
*operationsScheme() without forcing some ugly new/delete semantics on 
subclasses. i could force it to return an xml document or a qstring instead, 
but then we can't read out of a file or other source.

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

exactly.

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

see the second draft.

> >     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;

and then add methods for getting what parameters are expected for a service, 
and what types and defaults and runtime documentation for them and ... you 
end up with KConfigSkeleton.

> Plasma::ServiceCall *call = service->createCall("update", args);
> call->start();

already done in the next draft that i sent yesterday.

> As a last (maybe crazy) remark, we might want to make it look even closer
> to something like QtDBus on the applet writers side 

in which way exactly? setting up a connection, connecting to a named service, 
etc? 

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech
-------------- 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/20080516/6202532c/attachment-0001.pgp