Differential D26650 Diff 73535 src/widgets/kpropertiesdialog.cpp

Changeset View

Standalone View

View Options

src/widgets/kpropertiesdialog.cpp

Show First 20 Lines • Show All 1100 Lines • ▼ Show 20 Line(s)1092 } else { // Directory
1101 d->m_sizeStopButton->setIcon(QIcon::fromTheme(QStringLiteral("dialog-cancel"))); 1101​ d->m_sizeStopButton->setIcon(QIcon::fromTheme(QStringLiteral("dialog-cancel")));
1102 1102​
1103 connect(d->m_sizeDetermineButton, &QAbstractButton::clicked, this, &KFilePropsPlugin::slotSizeDetermine); 1103​ connect(d->m_sizeDetermineButton, &QAbstractButton::clicked, this, &KFilePropsPlugin::slotSizeDetermine);
1104 connect(d->m_sizeStopButton, &QAbstractButton::clicked, this, &KFilePropsPlugin::slotSizeStop); 1104​ connect(d->m_sizeStopButton, &QAbstractButton::clicked, this, &KFilePropsPlugin::slotSizeStop);
1105 1105​
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(serv->icon()));
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…
1113 sizelay->addWidget(d->m_sizeDetailsButton, 0); 1115​ sizelay->addWidget(d->m_sizeDetailsButton, 0);
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.
1114 } 1116​ }
1115 1117​
1116 sizelay->addStretch(10); // so that the buttons don't grow horizontally 1118​ sizelay->addStretch(10); // so that the buttons don't grow horizontally
1117 1119​
1118 // auto-launch for local dirs only, and not for '/' 1120​ // auto-launch for local dirs only, and not for '/'
1119 if (isLocal && !hasRoot) { 1121​ if (isLocal && !hasRoot) {
1120 d->m_sizeDetermineButton->setText(i18n("Refresh")); 1122​ d->m_sizeDetermineButton->setText(i18n("Refresh"));
1121 slotSizeDetermine(); 1123​ slotSizeDetermine();
▲ Show 20 Lines • Show All 320 Lines • ▼ Show 20 Line(s)1436{
1442 if (d->dirSizeUpdateTimer) { 1444​ if (d->dirSizeUpdateTimer) {
1443 d->dirSizeUpdateTimer->stop(); 1445​ d->dirSizeUpdateTimer->stop();
1444 } 1446​ }
1445 1447​
1446 d->m_sizeStopButton->setEnabled(false); 1448​ d->m_sizeStopButton->setEnabled(false);
1447 d->m_sizeDetermineButton->setEnabled(true); 1449​ d->m_sizeDetermineButton->setEnabled(true);
1448} 1450​}
1449 1451​
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()
1457{ 1460​{
1458 delete d; 1461​ delete d;
1459} 1462​}
1460 1463​
1461bool KFilePropsPlugin::supports(const KFileItemList & /*_items*/) 1464​bool KFilePropsPlugin::supports(const KFileItemList & /*_items*/)
1462{ 1465​{
▲ Show 20 Lines • Show All 2549 Lines • Show Last 20 Lines