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 ¶meters 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 _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel