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<KFilePropsPlugin *>(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
