Use KService to look for Filelight
Needs ReviewPublic

Authored by shubham on Tue, Jan 14, 5:19 AM.

Details

Reviewers
broulik
ngraham
Summary

Related to D24932

Diff Detail

Repository
R241 KIO
Branch
filelight
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21168
Build 21186: arc lint + arc unit
shubham created this revision.Tue, Jan 14, 5:19 AM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptTue, Jan 14, 5:19 AM
broulik added inline comments.Tue, Jan 14, 8:31 AM
src/widgets/kpropertiesdialog.cpp
1113

You can also use the icon from the service

anthonyfieroni added inline comments.
src/widgets/kpropertiesdialog.cpp
1455

service can be nullptr, add a check

shubham added inline comments.Tue, Jan 14, 9:26 AM
src/widgets/kpropertiesdialog.cpp
1113

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

1455

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

broulik added inline comments.Tue, Jan 14, 9:29 AM
src/widgets/kpropertiesdialog.cpp
1113

Does this not work?

d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon());
shubham updated this revision to Diff 73535.Tue, Jan 14, 3:17 PM

Use KService to get the application's icon

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

src/widgets/kpropertiesdialog.cpp
1109–1111

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

shubham updated this revision to Diff 73556.Tue, Jan 14, 6:51 PM

Use desciptive variable name

shubham marked 3 inline comments as done.Tue, Jan 14, 6:51 PM

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

shubham marked 3 inline comments as done.Tue, Jan 14, 6:57 PM
anthonyfieroni added inline comments.Tue, Jan 14, 7:21 PM
src/widgets/kpropertiesdialog.cpp
1113–1115

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

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

Remove

shubham updated this revision to Diff 73593.Wed, Jan 15, 7:56 AM

Make requested changes

shubham marked 2 inline comments as done.Wed, Jan 15, 7:57 AM
broulik added inline comments.Wed, Jan 15, 8:18 AM
src/widgets/kpropertiesdialog.cpp
1114

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

meven added a subscriber: meven.Wed, Jan 15, 9:08 AM
meven added inline comments.
src/widgets/kpropertiesdialog.cpp
1114

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

anthonyfieroni added inline comments.Wed, Jan 15, 9:35 AM
src/widgets/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.

sitter added a subscriber: sitter.Wed, Jan 15, 1:22 PM
sitter added inline comments.
src/widgets/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.