D24932: Add Button to open the folder in filelight for more details

2019-12-27 Thread Shubham
shubham updated this revision to Diff 72261.
shubham edited the summary of this revision.
shubham added a comment.


  Fix whitespaces and crash condition

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24932?vs=72254=72261

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: Add Button to open the folder in filelight for more details

2019-12-27 Thread Shubham
shubham marked 9 inline comments 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: Add Button to open the folder in filelight for more details

2019-12-27 Thread Nathaniel Graham
ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  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).

INLINE COMMENTS

> kpropertiesdialog.cpp:782
>  QLabel *m_sizeLabel;
> +
>  QPushButton *m_sizeDetermineButton;

When you add a newline, make sure it doesn't have any spaces in it

> kpropertiesdialog.cpp:786
> +QPushButton *m_sizeDetailsButton;
> +
>  KLineEdit *m_linkTargetLineEdit;

ditto

> kpropertiesdialog.cpp:1099
>  d->m_sizeStopButton = new QPushButton(i18n("Stop"), d->m_frame);
> +
> +if 
> (!QStandardPaths::findExecutable(QStringLiteral("filelight")).isEmpty()) {

ditto

> kpropertiesdialog.cpp:1101
> +if 
> (!QStandardPaths::findExecutable(QStringLiteral("filelight")).isEmpty()) {
> +d->m_sizeDetailsButton = new QPushButton(i18n("Explore in 
> Filelight?"), d->m_frame);
> +
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(QStringLiteral("filelight")));

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.

> kpropertiesdialog.cpp:1105
> +}
> +
>  
> d->m_sizeDetermineButton->setIcon(QIcon::fromTheme(QStringLiteral("view-refresh")));

ditto

> kpropertiesdialog.cpp:1108
>  
> d->m_sizeStopButton->setIcon(QIcon::fromTheme(QStringLiteral("dialog-cancel")));
> +
>  connect(d->m_sizeDetermineButton, ::clicked, this, 
> ::slotSizeDetermine);

ditto

> kpropertiesdialog.cpp:1114
>  sizelay->addWidget(d->m_sizeStopButton, 0);
> +sizelay->addWidget(d->m_sizeDetailsButton, 0);
> +

This causes a crash when Filelight isn't installed, because in that case, 
`d->m_sizeDetailsButton` is a `nullptr`.

> kpropertiesdialog.cpp:1115
> +sizelay->addWidget(d->m_sizeDetailsButton, 0);
> +
>  sizelay->addStretch(10); // so that the buttons don't grow 
> horizontally

ditto

> kpropertiesdialog.cpp:1453
> +const QUrl url = properties->url();
> +
> +// Open the current folder in filelight

ditto

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: Add Button to open the folder in filelight for more details

2019-12-27 Thread Shubham
shubham retitled this revision from "[WIP]: Add Button to open the folder in 
filelight for more details" to "Add Button to open the folder in filelight for 
more details".

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