Differential D26650 Diff 73483 src/widgets/kpropertiesdialog.cpp

Changeset View

Standalone View

View Options

src/widgets/kpropertiesdialog.cpp

Context not available.
1106 sizelay->addWidget(d->m_sizeDetermineButton, 0); 1106​ sizelay->addWidget(d->m_sizeDetermineButton, 0);
1107 sizelay->addWidget(d->m_sizeStopButton, 0); 1107​ sizelay->addWidget(d->m_sizeStopButton, 0);
1108 1108​
1109 if (!QStandardPaths::findExecutable(QStringLiteral("filelight")).isEmpty()) { 1109​ KService::Ptr serv = KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
ngrahamAuthorUnsubmitted
Done

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

ngraham: prefer descriptive variable names; "service" is better than "serv"
1110​
1111​ if (serv) {
1110 d->m_sizeDetailsButton = new QPushButton(i18n("Explore in Filelight"), d->m_frame); 1112​ d->m_sizeDetailsButton = new QPushButton(i18n("Explore in Filelight"), d->m_frame);
1111 d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(QStringLiteral("filelight"))); 1113​ d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(QStringLiteral("filelight")));
broulikUnsubmitted
Done

You can also use the icon from the service

broulik: You can also use the icon from the service
shubhamUnsubmitted
Done

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

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

Does this not work?

d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon());
broulik: Does this not work? ``` d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon()); ```
1112 connect(d->m_sizeDetailsButton, &QPushButton::clicked, this, &KFilePropsPlugin::slotSizeDetails); 1114​ connect(d->m_sizeDetailsButton, &QPushButton::clicked, this, &KFilePropsPlugin::slotSizeDetails);
anthonyfieroniUnsubmitted
Done

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());
});
anthonyfieroni: We don't need to reparse desktop file on every click just make it ``` connect(d…
broulikUnsubmitted
Not Done

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

broulik: Not sure copying that pointer into the lambda is a good idea?
mevenUnsubmitted
Not Done

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

meven: service is a QExplicitlySharedDataPointer<KService> in fact, I guess it covers lambda use cases.
anthonyfieroniUnsubmitted
Not Done

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.

anthonyfieroni: OK, it can be a problem since we can have many objects of KFilePropsPlugin thus lambda will…
sitterUnsubmitted
Not Done

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.

sitter: I'm pretty sure that isn't true. KFilePropsPlugin are the tabs inside the properties dialog.
mevenUnsubmitted
Not Done

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

meven: @shubham reimplement slotSizeDetails with `KService::serviceByDesktopName(QStringLiteral("org.
Context not available.
1450void KFilePropsPlugin::slotSizeDetails() 1452​void KFilePropsPlugin::slotSizeDetails()
1451{ 1453​{
1452 // Open the current folder in filelight 1454​ // Open the current folder in filelight
1453 KRun::run((QStandardPaths::findExecutable(QStringLiteral("filelight"))), { properties->url() }, properties->window(), QStringLiteral("Filelight"), QStringLiteral("filelight")); 1455​ KService::Ptr serv = KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
anthonyfieroniUnsubmitted
Done

service can be nullptr, add a check

anthonyfieroni: service can be nullptr, add a check
shubhamUnsubmitted
Done

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

shubham: This slot is called from a check at line 1111, so not required
1456​ KRun::runApplication(*serv, { properties->url() }, properties->window());
1454} 1457​}
anthonyfieroniUnsubmitted
Done

Remove

anthonyfieroni: Remove
1455 1458​
1456KFilePropsPlugin::~KFilePropsPlugin() 1459​KFilePropsPlugin::~KFilePropsPlugin()
Context not available.