another Service draft

[Kevin Ottens]

Le Thursday 15 May 2008, Aaron J. Seigo a écrit :
> attached is another draft of the Service framework; still doesn't have much
> docu about creating subclasses of Service yet, but things should be getting
> closer here.

Here we go again, same method.

First: I guess my comments for the class apidox still stand. Also there's 
probably a typo in the example (s/jobCompeted/jobCompleted/).

------------------
> class Service : public QObject
> {
>     Q_OBJECT
> public:
>    explicit Service(QObject *parent = 0);
>    Service(QObject *parent, const QVariantList &args);

protected? (see, I did it only once this time!) ;-)

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

Why not pass the destination on the call to load? I'm not sure the semantic 
of "setDestination" makes sense here, it's as if I'd be able to move a 
service.

s/destination/address/i ;-)

>    QString destination() const;

s/destination/address/ again.

>    QStringList operationList() const;
>    KConfigGroup operationParameters(const QString &operation);
>
>    ServiceJob* startOperation(const KConfigGroup &parameters);

Still my "why KConfigGroup" question.

>    QString name() const;
>
> protected:
>    virtual ServiceJob* createJob(const QString &operation,
>                                  QMap<QString, QVariant> &parameters) = 0;

Hmmm, or QMap<QString, QVariant> everywhere, or KConfigGroup everywhere. I'd 
be relunctant to mix both. Hmmmm... actually I'm now wondering, what about:
KConfigGroup operationDescription(const QString &operationName);

And then using QMap<QString, QVariant> everywhere else?

The missing bit would be the conversion from KConfigGroup to the QMap (too 
bad, KConfigGroup::entryMap() returns a QMap<QString, QString>).

>    virtual void registerOperationScheme();

I still don't like the name... but still no idea.

>    void setOperationsScheme(QIODevice *xml);

If it's "Operations" here, it should probably be "registerOperationsScheme()" 
above.

>    void setName(const QString &name);
>
> private:
>     class Private;
>     Private * const d;
> };
------------------
> class ServiceJob : public KJob
> {
>     Q_OBJECT
>
> public:
>     ServiceJob(const QString &destination, const QString &operation,
>                QMap<QString, QVariant> &parameters, QObject *parent = 0);
>     ~ServiceJob();
>
>     QString destination() const;

You probably already know my proposed change for this one. :o)

>     QString operation() const;

s/operation/operationName/?

>     QMap<QString, QVariant> parameters() const;
>     QVariant result() const;
>
> Q_SIGNALS:
>     void result(Plasma::ServiceJob *service, QVariant result);
>
> protected:
>     void setResult(const QVariant &result);
>
> private:
>     class Private;
>     Private * const d;
> };
------------------

IMO, the split between Service and ServiceJob makes it much better already.

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/20080517/928b6b77/attachment-0001.pgp