D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham edited the summary of this revision. shubham edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham updated this revision to Diff 72254. shubham marked 2 inline comments as done. shubham edited the summary of this revision. shubham added a comment. Fix above mentioned issues, now works perfectly fine REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24932?vs=68759=72254 BRANCH file REVISION DETAIL https://phabricator.kde.org/D24932 AFFECTED FILES src/widgets/kpropertiesdialog.cpp src/widgets/kpropertiesdialog_p.h To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
ngraham added a comment. 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 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham added a comment. @ngraham How should I create a KService for runApplication? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.
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.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D24932
To: shubham, ngraham, #frameworks
Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham added inline comments. INLINE COMMENTS > ngraham wrote in kpropertiesdialog.cpp:1100 > This is a `QPushButton`, so connect to `::clicked` It is done this way a bit below in line 1106 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
ngraham added a comment.
Much better, thanks.
INLINE COMMENTS
> kpropertiesdialog.cpp:1097
> +
> +if (QStandardPaths::findExecutable(QStringLiteral("filelight")) !=
> QString()) {
> +d->m_sizeDetailsButton = new QPushButton(i18n("Explore in
> Filelight?"), d->m_frame);
`isEmpty()` is preferred over comparing to `QString()`
> kpropertiesdialog.cpp:1100
> +
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(QStringLiteral("filelight")));
> +connect(d->m_sizeDetailsButton, ::clicked, this,
> ::slotSizeDetails);
> +}
This is a `QPushButton`, so connect to `::clicked`
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D24932
To: shubham, ngraham, #frameworks
Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham updated this revision to Diff 68759. shubham marked 3 inline comments as done. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24932?vs=68710=68759 REVISION DETAIL https://phabricator.kde.org/D24932 AFFECTED FILES src/widgets/kpropertiesdialog.cpp src/widgets/kpropertiesdialog_p.h To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
ngraham requested changes to this revision.
ngraham added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kpropertiesdialog.cpp:444
> {
> -for (KPropertiesDialogPlugin *it : qAsConst(d->m_pageList)) {
> +foreach (KPropertiesDialogPlugin *it, d->m_pageList) {
> if (auto *filePropsPlugin = qobject_cast(it)) {
We're trying to port our software //away// from using `foreach` and
`Q_FOREACH`; not back to them! :) Don't change these.
> kpropertiesdialog.cpp:745
>
> +static bool isFilelightInstalled()
> +{
All of this logic is unnecessary; instead use
https://doc.qt.io/qt-5/qstandardpaths.html#findExecutable
> kpropertiesdialog.cpp:1118
> +if (isFilelightInstalled()) {
> +d->m_sizeDetailsButton = new QPushButton(i18n("Open in
> Filelight"), d->m_frame);
> +
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(QStringLiteral("filelight")));
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.
> kpropertiesdialog.cpp:1409
> +QProcess *m_filelight = nullptr;
> +m_filelight->start(QStringLiteral("filelight"), QStringList() <<
> directory);
> +m_filelight->waitForFinished();
Use `KRun` to launch it, not `QProcess`
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D24932
To: shubham, ngraham, #frameworks
Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
D24932: [WIP]: Add Button to open the folder in filelight for more details
shubham created this revision. shubham added reviewers: ngraham, Frameworks. shubham added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. shubham requested review of this revision. REVISION SUMMARY BUG: 408962 Note: Does not compile on my machine due to system-specific issues (Should work ; ) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24932 AFFECTED FILES src/widgets/kpropertiesdialog.cpp src/widgets/kpropertiesdialog_p.h To: shubham, ngraham, #frameworks Cc: kde-frameworks-devel, #frameworks, LeGast00n, GB_2, michaelh, ngraham, bruns
