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
