KDEREVIEW: share like connect and plasmate

[Aaron J. Seigo]

On Tuesday, November 6, 2012 15:03:05 David Edmundson wrote:
> Initial code review of Share-Like-Connect:
> 
> from provider.h
> 
>     ~Provider();
> 
> This class is designed to be subclassed, yet the destructor is public
> and non virtual. The derived classes will leak everything.

it is a QObject subclass :)

> --
> 
> virtual QVariant executeAction(SLC::Provider::Action action, const
> QVariantHash &content, const QVariantHash &parameters
> 
> This implies the execute action is synchronous. 

it isn't; it can be, but it doesn't have to be.

> Given this is meant to
> (as I understand it) include uploading to the web in later releases
> this will cause all of plasma-desktop to lock up whilst the upload
> takes place.

the action can be processed later and executeAction can return success 
immediately. no blocking is required. executeAction simply lets the UI know 
that its job is complete.

> Again why is it a QVariant? Everything returns a bool.

this is a bit of a strange API, i agree. 

if it returns a bool, then it is done. 

but it can request a confirmation:

            QVariantHash result;
            result["Confirmation"] = i18n("Are you sure to delete this 
bookmark? \nAll of its associations with activities will be removed as 
well.");
            return result;

if it returns a string, then it's shown to the user as a yes/no question. then 
executeAction may get called again

what this allows is a stateless two-way process without requiring either side 
of the interaction to support any specific features. i'm not 100% happy with it 
... but it works well even from Javascript.

> In the case of the identica plugin, you start calling a webpage then
> return true immediately... but then who is responsible for showing if
> opening the website failed? Currently it will just fail silently and
> the user will get false positive feedback?

yes; this is not yet implemented. what we need is a way for plugins to emit a 
failure message at a random point.

> Returning a KJob* would fix both of the above.

not really. ignoring the unique challenges of using KJob from QML/JS (which 
can be worked around in the JS; we do this in various places already), it does 
not provide any mechanism for the back-and-forth communication between 
executeAction and the UI.

such API could be added using a KJob subclass; note, however, that we already 
do use a KJob in the service (SlcJob) which is a Plasma::ServiceJob. that 
service job simply wraps the provider's functionality.

what would be nice to avoid is a KJob from the provider inside a KJob from the 
service. (with the "real" KJob that does the work probably happening inside 
the provider for uploads)

what could be added is a return from executeAction that puts SlcJob into a 
waiting state so that it does not always setResult in start(), but may wait 
for the provider to finish up whatever it was doing.
 
but that means introducing statefulness into Provider and that would be nice 
to avoid.

>     virtual Actions actionsFor(const QVariantHash &content) const;
> 
> Completely undocumented QVariant maps do not make for a good API. A

apidox are missing, yes.

> good API should be usable without the documentation, this is not. To

that's utopian view of the world that should be aspired to. reality should not 
be twisted too far in the pursuit of it, however.

> me this looks like poor code to pander to QML's limitation.

not really. a full function would look like this:

actionsFor(const QUrl &uri, const QString &mimetype, const QString &title, 
const QString &source, const QString &title, const QPixmap &thumbnail, int 
windowId)

and some requests would have some of these items set, other requests wouldn't 
.. in future more items may be added ... 

what could be done is to create a C++ class that encapsulates these into 
getters and pas that through:

actionsFor(const Resource &resource)

and this would look a bit nicer in the C++, but only look uglier in the JS 
which we want to encourage the use of.

also keep in mind that this lives behind a Plasma::Service which also uses a 
QVariantHash to pass parameters as it is designed to be used as, well, a 
service with no assumptions as to function, target or locality.

> At the very least it needs documentation.

agreed.

>     virtual QString actionName(const QVariantHash &content, Action action);
> 
> This can be const?

yes it can; good catch .. Marco has fixed that now.

> --
> 
> in
> RatingProvider::executeAction
> 
>     int rating = parameters["Targets"].toStringList().first().toInt();
> 
> calling .first() on a stringlist is liable to crash if the stringlist is
> empty.
> 
> (in fairness, there's a FIXME next to it too)

fixed.
 
> --
> 
> rating/tags
> 
> you should probably check nepomuk is enabled in the actionsFor()
> method. Especially as you always return true regardless of whether
> setTags failed or not

indeed; legacy from usage in Active where nepomuk is always on. will fix next.

-- 
Aaron J. Seigo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20121106/55c2eef0/attachment.sig>