dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > iconapplet.cpp:242 > +{ > + new KRun(QUrl::fromLocalFile(m_localPath), nullptr); > +} Just wondering, why is the parent widget nullptr here, while it's QApplication::desktop() in other code further down? Not that it changes anything at runtime, I suppose. > iconapplet.cpp:268 > + > + const QString &stringUrl = m_url.toLocalFile(); > + This variable name confused me. It's called "something URL" but it's not a URL, it's a local path. > iconapplet.cpp:306 > + foreach (const QUrl &url, urls) { > + params.append(url.toString()); > + } This for loop can be written in one line with const QStringList params = QUrl::toStringList(urls); (I added that to QUrl for Qt 5.1) > iconapplet.cpp:309 > + > + QProcess::startDetached(m_url.path(), params); > + Surely this should be m_url.toLocalFile() ? (only makes a difference on Windows, but it's the right method for getting a local path out of a URL) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2687 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: broulik, #plasma, dfaure Cc: mart, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas