[Differential] [Accepted] D4690: Import remote ioslave from plasma-workspace

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  I know this isn't your code and you're just moving it to solve a dependency issue. No problem, go ahead.
  
  But while at it I took at it, I look at the code and found a few issues, feel free to fix them ;)
  
  Unittests would be good, too .... OK ok I'm pushing it :)

INLINE COMMENTS

> remoteimpl.cpp:42
> +    if (!dir.exists()) {
> +        dir.cdUp();
> +        dir.mkdir(QStringLiteral("remoteview"));

This should probably be QDir().mkpath(path) instead of the whole if exists + cdUp + mkdir dance, which would fail if the parent doesn't exist either (ok, unlikely, but anyway, mkpath is shorter and safer)

> remoteimpl.cpp:96
> +
> +        QStringList filenames = dir.entryList(QDir::Files | QDir::Readable);
> +

Wow this is convoluted, a simple QFile::exists(*dirpath + '/' + filename) should be enough rather than a full directory listing.

> remoteimpl.cpp:155
> +    if (service && service->isValid()) {
> +        url.setPath(QStandardPaths::locate(QStandardPaths::ApplicationsLocation,
> +                                           QStringLiteral("%1.desktop").arg(WIZARD_SERVICE)));

wrong port to QUrl, should be url = QUrl::fromLocalFile(...)

REPOSITORY
  R241 KIO

BRANCH
  import-remote

REVISION DETAIL
  https://phabricator.kde.org/D4690

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: elvisangelaccio, dfaure, davidedmundson
Cc: #plasma, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170221/9a7b9cb1/attachment-0001.html>