Remove unused signal we can use directly "signal(const QUrl&)
ClosedPublic

Authored by mlaurent on Nov 27 2019, 6:31 AM.

Details

Summary

We create overload signal but we can use it with argument direcly

Test Plan

build

Diff Detail

Repository
R236 KWidgetsAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mlaurent created this revision.Nov 27 2019, 6:31 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 27 2019, 6:31 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mlaurent requested review of this revision.Nov 27 2019, 6:31 AM
cfeck added a subscriber: cfeck.Nov 27 2019, 10:15 AM
cfeck added inline comments.
src/kurllabel.h
284

Wrong comment?

Looks correct from deprecation-markup POV. Did you do a test build with EXCLUDE_DEPRECATED_BEFORE_AND_AT=5.65.0, to check if any tests or examples still use the deprecated signals and need some additions of #if KWIDGETSADDONS_ENABLE_DEPRECATED_SINCE(5, 65) in places?

When it comes to deprecated signals, seems those overloads have been present since introduction of KUrlLabel. Not sure about the usefulness of passing the url as argument with any signals, given that the url is a static property of the label, only set via the setter setUrl().
Have you checked how the respective signals are used? Are there similar classes, what signals do they have?

mlaurent marked an inline comment as done.Nov 27 2019, 11:12 AM

There is not autotests/test which use kurllabel.
we have some apps as connect(urlLabel, qOverload<const QString &>(&KUrlLabel::leftClickedUrl), this, &SummaryWidget::selectFolder); in kmail.

mlaurent updated this revision to Diff 70406.Nov 27 2019, 11:12 AM

Fix comment

Yeah, but these days with lambdas, we can find out who emitted the signal, without the need for an identifier as signal argument...

connect(urlLabel, qOverload<>(&KUrlLabel::leftClickedUrl), this, [this, urlLabel]() { selectFolder(urlLabel->url()); } );

should work too.
This way the application can choose whether it wants the URL or the KUrlLabel pointer in the slot, or whatever else.

I'm not 100% sure what's best -- e.g. QPushButton has clicked() without arguments, while KJob as result(KJob*) because it's convenient for the slot. With lambdas we don't really need the KJob* anymore. Capturing the local job variable instead would even remove some static_cast to subclasses in a number of cases. But the porting effort would be huge, so I'm reluctant :-)

I guess this is most closely related to QPushButton though. In the simple case (different slot for each button or URLLabel) the signal with 0 arguments was always enough, and for the dynamic case (N buttons or N URLLabels) we have lambdas nowadays. Seems true for both, so I vote for deprecating the signals with an argument, and for porting KMail to my lambda above right away.

Opinions?

I don't prefer a signal with argument or not.
Indeed I can adapt kmail for it.

So I will change deprecated signal.

mlaurent updated this revision to Diff 70477.Nov 28 2019, 6:55 AM

Deprecated signal with argument

dfaure accepted this revision.Nov 30 2019, 11:51 AM
This revision is now accepted and ready to land.Nov 30 2019, 11:51 AM
This revision was automatically updated to reflect the committed changes.