This brings back the properties dialog allowing to manipulate the icon and label of a widget.
Details
Details
- Reviewers
mart dfaure - Group Reviewers
Plasma - Commits
- R120:caea0aa4ee22: [Icon Widget] Bring back properties dialog
R871:caea0aa4ee22: [Icon Widget] Bring back properties dialog
Basically it makes sure we always have a desktop file which we can edit (either copying the one of the app or making a Link one in case of files). (the plasma 4 applet did some mental stuff with creating a desktop file only if you edited it and it wasnt writable etc etc).
- Launching apps and files works
- Dropping files on apps (launches app with as arg) and folders works (offers copy/move/link here)
- Jump list actions work
- Creating multiple icons for the same app works and they can be edited separately
- The desktop files do not screw up existing apps or krunner/kickoff results (they're not in ~/.local/share/applications)
- Editing app info (e.g. icon, name, "run in terminal" setting) works and the applet reflects the change immediately
- Removing the applet deletes the backing file in ~/.local/share/plasma_icons (and vice-versa, if you start plasmashell and the file is missing, it will be recreated)
Only issue I found after I fixed all of them™ was that when you have a link to two files of the same name (ie. one will have (1) suffix) and you edit some property (e.g. the icon) the name of the icon widget suddenly changes to reflect the actual file name with (1) suffix, ie. KIO updates the Name in the desktop file to contain the file name of the desktop file, but whatever.
Didn't test the migration path for existing icons again, so please add some various icon widgets to your desktop (e.g. some files, some apps, also an app multiple times) and see how it goes :) It should™ work, though.
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
i kindof feel that we shouldn't use neither kio nor krun in this case (maybe kio yes, but krun::run* doesn't seem particularly appropriate in this case)
| applets/icon/iconapplet.cpp | ||
|---|---|---|
| 58 | QFile::remove is enough, no need for a QFile instance. | |
| 79 | the end of the comment is weird ... ("nk ? | |
| 91 | Extract QUrl::fromLocalFile(plasmaIconsFolderPath) so you can use it twice? const QUrl dirUrl = QUrl::fromLocalFile(plasmaIconsFolderPath);
QUrl backingDesktopFile(dirUrl);
backingDesktopFile.setPath(dirUrl.path() + QLatin1Char('/') + KIO::suggestName(dirUrl, m_url.fileName()); | |
| 109 | yep so remove it :-) | |
| 236 | So m_localPath is the full path to a Type=Link desktop file? For proof: wget http://www.davidfaure.fr/2016/testlink.desktop kioclient5 exec $PWD/testlink.desktop (which does exactly this, new KRun(url)) | |
| 238 | KService is for Type=Application (or Type=Service) desktop files. | |
| 259 | Why the convoluted line here? Isn't droppedUrl.toString() always a full URL? | |
| 305 | toString should be enough. | |
| 309 | This one could be a case for QProcess::startDetached, no need for shell-quoting then. | |
Comment Actions
- Ensure Link desktop file is actually .desktop so the properties dialog offers to change the URL
- Emit jump list actions changed when they do (fixes jump list actions not working when applet is created for the first time)
- Call sync() on the KDesktopFile we created for the file Links so the applet works when it is created for the first time for a file
- KIO::suggestName always appends suffix, so check whether the file exists before
- use new KRun to run the apps and QProcess::startDetached as suggested by David F
- Cleanup a bit, at least try to ;)
Comment Actions
- Create context menu actions on demand, saves some cycles on startup and more importantly allows us to follow SystemImmutable
| applets/icon/iconapplet.cpp | ||
|---|---|---|
| 242 | 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. | |
| 268 | This variable name confused me. It's called "something URL" but it's not a URL, it's a local path. | |
| 306 | 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) | |
| 309 | 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) | |
Comment Actions
- update actions also in Component.onCompleted so the "configure" button in applet handle works correctly the first time
- Set icon for properties dialog ("document-properties", same as the context menu) instead of generic Plasma icon
- Let processDrop return void, nobody used the return value
- Use QUrl::toStringList
- Parent KRun to QApplication::desktop()
- Some minor cleanup
| applets/icon/iconapplet.cpp | ||
|---|---|---|
| 286 ↗ | (On Diff #8967) | So if the application forgot to claim that it supports this mimetype, dropping a file onto it will do nothing? I'm not sure this code is necessary. It's not very consistent either (an application which doesn't list any mimetype will be run without any restriction, while an application that lists a few mimetypes will not be started for other mimetypes). Since the user is explicitly dropping a file onto an application, I would just start the application with the file as argument. MimeType= is about associating apps with files for the "click on a file in the filemanager" use case, where we have to be smart about selecting the right app. But here the user did select the app, we can obey the user's request, no? |
Comment Actions
- Remove now unused icon qml plugin
- Drop cumbersome drop handling – if the user explicitly dropped some files somewhere the application should just launch with them, shouting at the user if unsupported
Comment Actions
Shouting or just coping with the file (e.g. displaying raw data, for a text editor) ;)
Looks good, thanks.