[Differential] [Commented On] D2687: WIP: [Icon Widget] Bring back properties dialog

[dfaure (David Faure)]

dfaure added inline comments.

INLINE COMMENTS

> iconapplet.cpp:57
> +    if (destroyed()) {
> +        QFile(m_localPath).remove();
> +    }

QFile::remove is enough, no need for a QFile instance.

> iconapplet.cpp:78
> +    if (!m_url.isValid()) {
> +        // invalid url, use dummy data ("nk
> +        populateFromDesktopFile(QString());

the end of the comment is weird ...  ("nk ?

> iconapplet.cpp:90
> +    // now create a desktop file
> +    const QUrl backingDesktopFile = QUrl::fromLocalFile(plasmaIconsFolderPath + QLatin1Char('/')
> +                                                        + KIO::suggestName(QUrl::fromLocalFile(plasmaIconsFolderPath), m_url.fileName()));

Extract QUrl::fromLocalFile(plasmaIconsFolderPath) so you can use it twice?
Like this:

  const QUrl dirUrl = QUrl::fromLocalFile(plasmaIconsFolderPath);
  QUrl backingDesktopFile(dirUrl);
  backingDesktopFile.setPath(dirUrl.path() + QLatin1Char('/') + KIO::suggestName(dirUrl, m_url.fileName());

> iconapplet.cpp:108
> +            // set no display so it does not mess with our system applications
> +            // TODO shouldn't be neccessary as we don't have it in applications
> +            KDesktopFile(localBackingDesktopFile).desktopGroup().writeEntry(QStringLiteral("Hidden"), true);

yep so remove it :-)

> iconapplet.cpp:235
> +{
> +    //new KRun(QUrl::fromLocalFile(m_localPath));
> +

So m_localPath is the full path to a Type=Link desktop file?
Then this first line is the one that should work, creating a KRun with the desktop file as argument.

For proof:

  wget http://www.davidfaure.fr/2016/testlink.desktop
  kioclient5 exec $PWD/testlink.desktop

(which does exactly this, new KRun(url))

> iconapplet.cpp:237
> +
> +    // TODO doesnt find it usually
> +    KService service(m_localPath);

KService is for Type=Application (or Type=Service) desktop files.

> iconapplet.cpp:258
> +    foreach (const QJsonValue &droppedUrl, droppedUrls) {
> +        const QUrl url = QUrl::fromUserInput(droppedUrl.toString(), QString(), QUrl::AssumeLocalFile);
> +        if (url.isValid()) {

Why the convoluted line here? Isn't droppedUrl.toString() always a full URL?

> iconapplet.cpp:304
> +        foreach (const QUrl &url, urls) {
> +            // TODO toEncoded?
> +            params += QLatin1Char(' ') + KShell::quoteArg(url.isLocalFile() ? url.toLocalFile() : url.toEncoded());

toString should be enough.

> iconapplet.cpp:308
> +
> +        KRun::runCommand(KShell::quoteArg(m_url.path()) + QLatin1Char(' ') + params, nullptr);
> +        return true;

This one could be a case for QProcess::startDetached, no need for shell-quoting then.

REPOSITORY
  rPLASMAWORKSPACE 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/20161104/e4851bf8/attachment-0001.html>