[Differential] [Requested Changes To] D2687: [Icon Widget] Bring back properties dialog

[dfaure (David Faure)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161214/771f2366/attachment-0001.html>