FEATURE: 408962
Details
Details
- Reviewers
ngraham broulik - Group Reviewers
Frameworks - Commits
- R241:e348cd80d35f: Add button to open the folder in filelight to view more details
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- file
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 20319 Build 20337: arc lint + arc unit
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 446 | We're trying to port our software away from using foreach and Q_FOREACH; not back to them! :) Don't change these. | |
| 745 | All of this logic is unnecessary; instead use https://doc.qt.io/qt-5/qstandardpaths.html#findExecutable | |
| 1101 | Maybe "Explore in Filelight?" or "See usage with Filelight" If the user isn't familiar with what Filelight is, it might be unclear what you would want to open a folder in it. | |
| 1456 | Use KRun to launch it, not QProcess | |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 1100 ↗ | (On Diff #68759) | It is done this way a bit below in line 1106 |
Comment Actions
It doesn't compile:
/home/nate/kde/src/kio/src/widgets/kpropertiesdialog.cpp:1389:72: error: ‘QString::QString(const char*)’ is private within this context
1389 | KRun::runCommand(QStringLiteral("filelight"), directory, nullptr, QStringLiteral("/"));
| ^Anyway, should use KRun::runApplication() for this since that's made for launching GUI apps and handling arguments passed to the binary.
Comment Actions
Copy some other code that's already doing it this way I guess. For example, in plasma: https://cgit.kde.org/plasma-workspace.git/tree/libtaskmanager/tasktools.cpp#n767
If you don't already know about it, let me introduce you to the magic of LXR: https://lxr.kde.org/ident?_i=runApplication
Comment Actions
We're getting there, but this code now causes a crash that must be fixed. Please make sure you are testing your changes for all conditions (i.e. don't just make sure that it works when Filelight is installed, also test the case where it's not installed).
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 782 | When you add a newline, make sure it doesn't have any spaces in it | |
| 786 | ditto | |
| 1099 | ditto | |
| 1101 | Heh, I think I confused you, my bad. I wasn't trying to imply that the label should actually have a question mark in it. Button labels don't typically end with punctuation like that. Just "Explore in Filelight" is enough. | |
| 1105 | ditto | |
| 1108 | ditto | |
| 1114 | This causes a crash when Filelight isn't installed, because in that case, d->m_sizeDetailsButton is a nullptr. | |
| 1115 | ditto | |
| 1453 | ditto | |
Comment Actions
Much better now, thanks. There's just one more thing I only noticed now, sorry. Then I think it'll be good to go.
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 1386 ↗ | (On Diff #68759) | Instead of defining this as a variable that's used only once, you can just directly access the value from properties->url() in the KRun::run() function |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 1387 ↗ | (On Diff #68759) | please do not hardcode the path to filelight... other than breaking on systems where filelight is installed in a prefix different than /usr, this breaks on Mac and Windows (and not only). |
Comment Actions
As a reference for the future: this change broke the two weeks string freeze - which we haven't enforced so strongly in the past months, but please keep an eye on it. Expecially this time of the year with many countries on vacation.
Comment Actions
Can you please look for filelight using KService::serviceByDesktopName("org.kde.filelight"), then you don't have hardcode the executable path, name, icon, etc.
Comment Actions
It would make more sense to submit a new patch. This one wasn't reverted, so it's not clear what would happen if we tried to land it.