KModifierKeyInfo: we are sharing the internal implementation
ClosedPublic

Authored by apol on Jul 3 2019, 3:14 PM.

Details

Summary

Use data types that will only destroy it when necessary.

Diff Detail

Repository
R273 KGuiAddons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13568
Build 13586: arc lint + arc unit
apol created this revision.Jul 3 2019, 3:14 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 3 2019, 3:14 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Jul 3 2019, 3:14 PM

I don't think any of these changes are ABI-compatible?

I don't understand the bug being fixed.

We have:

Q_DISABLE_COPY(KModifierKeyInfo)

So why does KModifierKeyInfo::Private need to be shared?

So why does KModifierKeyInfo::Private need to be shared?

It needs to be refcounted. In 0fe2990dbad992a4925a7b7bee09b1cdfbe5a7a7 a plugin infrastructure was introduced. Unbeknownst to us the instances are shared internally by Qt.

KModifierKeyInfoProvider* createProvider()
{
    QPluginLoader loader(QStringLiteral("kf5/kguiaddons/kmodifierkey/kmodifierkey_")+qGuiApp->platformName());
    auto instance = dynamic_cast<KModifierKeyInfoProvider*>(loader.instance());
    if (instance)
        return instance;
...
 
 KModifierKeyInfo::KModifierKeyInfo(QObject *parent)
    : QObject(parent), p(createProvider())

The instance returned is the same every time despite the QPluginLoader being on the stack.
Now the destructor of KModifierKeyInfo cleans up p and if you have two instances of KModifierKeyInfo, which Kile does for instance, it crashes on double delete.

apol added a comment.Jul 4 2019, 9:57 AM

I don't think any of these changes are ABI-compatible?

Yes it is, we're just modifying the size of a private class.

Also this was never released yet.

apol added a comment.Jul 5 2019, 5:45 PM

Landing because it works and we want it in before the freeze.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 5 2019, 5:46 PM
This revision was automatically updated to reflect the committed changes.