D26650: Use KService to look for Filelight

[Nathaniel Graham]

ngraham commandeered this revision.
ngraham edited reviewers, added: shubham; removed: ngraham.
ngraham added a comment.


  Aha!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: ngraham, broulik, shubham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Nathaniel Graham]

ngraham abandoned this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: ngraham, broulik, shubham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Kai Uwe Broulik]

broulik added a comment.


  Superseded by D28266 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Nathaniel Graham]

ngraham added a comment.


  Ping @shubham

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> sitter wrote in kpropertiesdialog.cpp:1114
> I'm pretty sure that isn't true.
> 
> KFilePropsPlugin are the tabs inside the properties dialog. They get 
> instantiated for each dialog and destroyed when the dialog is destroyed. They 
> are not persistent throughout the life time of the process. 
> The lamda in this case is scoped to the internal QFunctorSlotObject or 
> whatever it's called and that is held by the QObject. When the dialog gets 
> destroyed, it destroys the KFilePropsPlugin instance and that disconnects the 
> signal triggering the QFunctorSlotObject to get deleted, which in turn causes 
> the lambda to lose scope and clean up, destroying its pointer copies.
> TLDR: the lambda and all its copies do not outlive the property dialog it 
> belongs to.

@shubham reimplement slotSizeDetails with 
`KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));` + 
`Krun::runApplication`

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham added a comment.


  @broulik @ngraham Any updates on this, how to proceed?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kpropertiesdialog.cpp:1114
> OK, it can be a problem since we can have many objects of KFilePropsPlugin 
> thus lambda will extend service ptr life to the process end, which can result 
> in memory leak (it's not leak) But since it's a plugin we don't expect a tons 
> of objects, but i'm fine to make service a class scope var to not outlive the 
> plugin.

I'm pretty sure that isn't true.

KFilePropsPlugin are the tabs inside the properties dialog. They get 
instantiated for each dialog and destroyed when the dialog is destroyed. They 
are not persistent throughout the life time of the process. 
The lamda in this case is scoped to the internal QFunctorSlotObject or whatever 
it's called and that is held by the QObject. When the dialog gets destroyed, it 
destroys the KFilePropsPlugin instance and that disconnects the signal 
triggering the QFunctorSlotObject to get deleted, which in turn causes the 
lambda to lose scope and clean up, destroying its pointer copies.
TLDR: the lambda and all its copies do not outlive the property dialog it 
belongs to.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: sitter, meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> meven wrote in kpropertiesdialog.cpp:1114
> service is a QExplicitlySharedDataPointer in fact, I guess it 
> covers lambda use cases.

OK, it can be a problem since we can have many objects of KFilePropsPlugin thus 
lambda will extend service ptr life to the process end, which can result in 
memory leak (it's not leak) But since it's a plugin we don't expect a tons of 
objects, but i'm fine to make service a class scope var to not outlive the 
plugin.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26650: Use KService to look for Filelight

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> broulik wrote in kpropertiesdialog.cpp:1114
> Not sure copying that pointer into the lambda is a good idea?

service is a QExplicitlySharedDataPointer in fact, I guess it covers 
lambda use cases.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26650: Use KService to look for Filelight

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1114
> +
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon()));
> +connect(d->m_sizeDetailsButton, ::clicked, this, 
> [this, service]() {
> +KRun::runApplication(*service, { properties->url() }, 
> properties->window());});

Not sure copying that pointer into the lambda is a good idea?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham marked 2 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham updated this revision to Diff 73593.
shubham added a comment.


  Make requested changes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26650?vs=73556=73593

BRANCH
  filelight

REVISION DETAIL
  https://phabricator.kde.org/D26650

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp
  src/widgets/kpropertiesdialog_p.h

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1114
> +
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon()));
>  connect(d->m_sizeDetailsButton, ::clicked, this, 
> ::slotSizeDetails);
>  sizelay->addWidget(d->m_sizeDetailsButton, 0);

We don't need to reparse desktop file on every click just make it

  connect(d->m_sizeDetailsButton, ::clicked, this, [this, 
service]() {
  KRun::runApplication(*service, { properties->url() }, 
properties->window());
  });

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1452-1457
>  void KFilePropsPlugin::slotSizeDetails()
>  {
>  // Open the current folder in filelight
> -KRun::run((QStandardPaths::findExecutable(QStringLiteral("filelight"))), 
> { properties->url() }, properties->window(), QStringLiteral("Filelight"), 
> QStringLiteral("filelight"));
> +KService::Ptr service = 
> KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
> +KRun::runApplication(*service, { properties->url() }, 
> properties->window());
>  }

Remove

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Nathaniel Graham]

ngraham added a comment.


  Please mark the inline comments that are resolved as "Done"

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham updated this revision to Diff 73556.
shubham added a comment.


  Use desciptive variable name

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26650?vs=73535=73556

BRANCH
  filelight

REVISION DETAIL
  https://phabricator.kde.org/D26650

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Nathaniel Graham]

ngraham added a comment.


  Much better, thanks. Remember to mark inline comments as "Done" once you've 
addressed them.

INLINE COMMENTS

> kpropertiesdialog.cpp:1109
>  
> -if 
> (!QStandardPaths::findExecutable(QStringLiteral("filelight")).isEmpty()) {
> +KService::Ptr serv = 
> KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
> +

prefer descriptive variable names; "service" is better than "serv"

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham updated this revision to Diff 73535.
shubham added a comment.


  Use KService to get the application's icon

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26650?vs=73483=73535

BRANCH
  filelight

REVISION DETAIL
  https://phabricator.kde.org/D26650

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> shubham wrote in kpropertiesdialog.cpp:1113
> I tried removing it, but I couldn't see the icon on the button then.

Does this not work?

  d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon());

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham added inline comments.

INLINE COMMENTS

> broulik wrote in kpropertiesdialog.cpp:1113
> You can also use the icon from the service

I tried removing it, but I couldn't see the icon on the button then.

> anthonyfieroni wrote in kpropertiesdialog.cpp:1455
> service can be nullptr, add a check

This slot is called from a check at line , so not required

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1455
>  // Open the current folder in filelight
> -KRun::run((QStandardPaths::findExecutable(QStringLiteral("filelight"))), 
> { properties->url() }, properties->window(), QStringLiteral("Filelight"), 
> QStringLiteral("filelight"));
> +KService::Ptr serv = 
> KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
> +KRun::runApplication(*serv, { properties->url() }, properties->window());

service can be nullptr, add a check

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1113
>  d->m_sizeDetailsButton = new QPushButton(i18n("Explore in 
> Filelight"), d->m_frame);
>  
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(QStringLiteral("filelight")));
>  connect(d->m_sizeDetailsButton, ::clicked, this, 
> ::slotSizeDetails);

You can also use the icon from the service

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26650: Use KService to look for Filelight

[Shubham]

shubham created this revision.
shubham added reviewers: broulik, ngraham.
shubham added a project: Frameworks.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.
shubham requested review of this revision.

REVISION SUMMARY
  Related to D24932 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, broulik, ngraham
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns