D26650: Use KService to look for Filelight

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,

D26650: Use KService to look for Filelight

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

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

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

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

D26650: Use KService to look for Filelight

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

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

D26650: Use KService to look for Filelight

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

D26650: Use KService to look for Filelight

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

D26650: Use KService to look for Filelight

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, {

D26650: Use KService to look for Filelight

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

D26650: Use KService to look for Filelight

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1114 > + > d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon())); > connect(d->m_sizeDetailsButton, ::clicked, this, > ::slotSizeDetails); >

D26650: Use KService to look for Filelight

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() },

D26650: Use KService to look for Filelight

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

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

D26650: Use KService to look for Filelight

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 =

D26650: Use KService to look for Filelight

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

D26650: Use KService to look for Filelight

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

D26650: Use KService to look for Filelight

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

D26650: Use KService to look for Filelight

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"), >

D26650: Use KService to look for Filelight

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"))); >

D26650: Use KService to look for Filelight

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