Remove usage of QWeakPointer for QObject for thumbnails
ClosedPublic

Authored by gladhorn on Aug 10 2019, 10:04 AM.

Details

Summary

This usage of QWeakPointer has been deprecated since Qt 5.0, since it
leads to really confusing API - usually you must never dereference a
QWeakPointer directly, but always go through QSharedPointer, except in
this one case, where it's permissible.

The thumbnails are only referenced in one place, this change is straight-forward.

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D23070_1
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14948
Build 14966: arc lint + arc unit
gladhorn created this revision.Aug 10 2019, 10:04 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 10 2019, 10:04 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
gladhorn requested review of this revision.Aug 10 2019, 10:04 AM

Where is part 1? Please don't name the commits "<many words> part x", "<many words> part y". Name explicitly each of them what the commit does. This one could be for example: "Remove thumbnails QWeakPointers".

gladhorn updated this revision to Diff 63473.Aug 10 2019, 10:51 AM
gladhorn retitled this revision from Remove usage of QWeakPointer for QObject part 2 to Remove usage of QWeakPointer for QObject for thumbnails.

Fix commit message

Sorry, but although you said that the change would be straight-forward can you explain it to me some more. Weak pointers are normally used in conjunction with shared ones. Is there a shared one somewhere?

See https://phabricator.kde.org/D23065

Using a QWeakPointer for QObjects works, but has been deprecated for around seven years. QPointer does the same cleanly (track life-time of QObjects). If you use .toStrongRef() on a QWeakPointer to a QObject without going through QSharedPointer you are in UB land.
In other words: this API is bad and will most likely be removed from Qt in the future.

And that's exactly it: there is no QSharedPointer involved here at all.

gladhorn updated this revision to Diff 63476.Aug 10 2019, 11:32 AM
gladhorn edited the summary of this revision. (Show Details)

better commit message

See https://phabricator.kde.org/D23065

Using a QWeakPointer for QObjects works, but has been deprecated for around seven years. QPointer does the same cleanly (track life-time of QObjects). If you use .toStrongRef() on a QWeakPointer to a QObject without going through QSharedPointer you are in UB land.
In other words: this API is bad and will most likely be removed from Qt in the future.

Thanks for the explanation. With "except in this one case" in the description you mean which case? Using it on QObjects? The API will most likely be removed, this means the whole concept of QWeakPointer?

See https://phabricator.kde.org/D23065

Using a QWeakPointer for QObjects works, but has been deprecated for around seven years. QPointer does the same cleanly (track life-time of QObjects). If you use .toStrongRef() on a QWeakPointer to a QObject without going through QSharedPointer you are in UB land.
In other words: this API is bad and will most likely be removed from Qt in the future.

Thanks for the explanation. With "except in this one case" in the description you mean which case? Using it on QObjects? The API will most likely be removed, this means the whole concept of QWeakPointer?

QWeakPointer is just fine. QWeakPointer for QObjects is not, when not using QSharedPointer. So the constructor QWeakPointer::QWeakPointer<QObject>(object) will be removed.

Ok, I believe I got it now. The QWeakPointer constructer should get either passed a QSharedPointer or QWeakPointer as argument. But here we just pass the QObject EffectWindowImpl. Thanks for explaining.

romangg accepted this revision.Aug 10 2019, 12:32 PM
This revision is now accepted and ready to land.Aug 10 2019, 12:32 PM
This revision was automatically updated to reflect the committed changes.

Ok, I believe I got it now. The QWeakPointer constructer should get either passed a QSharedPointer or QWeakPointer as argument. But here we just pass the QObject EffectWindowImpl. Thanks for explaining.

Indeed, QWeakPointer should only come from QSharedPointer, nothing else.