KDEREVIEW: share like connect and plasmate

[David Edmundson]

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.

--

virtual QVariant executeAction(SLC::Provider::Action action, const
QVariantHash &content, const QVariantHash &parameters

This implies the execute action is synchronous. 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. Again why is it a QVariant? Everything returns a bool.

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?

Returning a KJob* would fix both of the above.

--

    virtual Actions actionsFor(const QVariantHash &content) const;

Completely undocumented QVariant maps do not make for a good API. A
good API should be usable without the documentation, this is not. To
me this looks like poor code to pander to QML's limitation. At the
very least it needs documentation.

--
    virtual QString actionName(const QVariantHash &content, Action action);

This can be const?


--

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)

--

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


--

RatingProvider::exectuteAction

if (content.value("Mime Type").toString() == "text/x-html")

should be
== QLatin1String("text/x-html");

(this is in many places)

--

David Edmundson