2

I need to create dynamic list of QObject* (representin a custom model) and expose them to QML. The problem is that QML tries to re-use previously deleted QObject* which ends up with errors at runtime:

qrc:/MyWidget.qml:6: TypeError: Cannot read property 'value' of null

Here is my model:

#include <QObject>

class SubModel : public QObject
{
    Q_OBJECT
public:
    SubModel(int value) : m_value(value) {}

    Q_PROPERTY(int value READ value WRITE setValue NOTIFY valueChanged)

    int value() { return m_value; }

    void setValue(int value)
    {
        m_value = value;
        emit valueChanged();
    }

signals:
    void valueChanged();

private:
    int m_value;
};

Here is the QAbstractListModel containing the SubModel list (note the createSubModels() method):

#include <QAbstractListModel>
#include <QObject>
#include <QVariant>
#include <memory>
#include <vector>

class ModelList : public QAbstractListModel
{
    Q_OBJECT

public:
    enum ModelRole
    {
        SubModelRole = Qt::UserRole
    };
    Q_ENUM(ModelRole)

    void setSubModels(std::vector<std::unique_ptr<SubModel>> subModels)
    {
        beginResetModel();
        m_subModels = std::move(subModels);
        endResetModel();
    }

    int rowCount(const QModelIndex& parent = QModelIndex()) const override
    {
        return m_subModels.size();
    }

    QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override
    {
        if (!index.isValid()) {
            return {};
        }

        switch (role) {
        case ModelRole::SubModelRole:
            return QVariant::fromValue<SubModel*>(m_subModels[index.row()].get());
        }

        return {};
    }

    bool setData(const QModelIndex& index, const QVariant& value, int role) override
    {
        Q_UNUSED(index);
        Q_UNUSED(value);
        Q_UNUSED(role);
        return false;
    }

    QHash<int, QByteArray> roleNames() const override
    {
        QHash<int, QByteArray> roles;
        roles[ModelRole::SubModelRole] = "submodel";
        return roles;
    }

    Q_INVOKABLE void createSubModels()
    {
        std::vector<std::unique_ptr<SubModel>> subModels;
        for (int i = 0; i < rand() % 5 + 1; i++) {
            subModels.push_back(std::make_unique<SubModel>(rand() % 100));
        }
        setSubModels(std::move(subModels));
    }

private:
    std::vector<std::unique_ptr<SubModel>> m_subModels;
};

Here is my QML widget using the SubModel instance (note the typed property):

import QtQuick 2.12
import MyLib.SubModel 1.0

Text {
    property SubModel subModel;
    text: subModel.value
}

Here is the main.qml file:

import QtQuick 2.5
import QtQuick.Window 2.2
import QtQuick.Controls 2.15

Window { 
    visible: true
    
    Column {
        Button {
            text: "Create list"
            onClicked: modelList.createSubModels();
        }

        Column {
            Repeater {
                model: modelList

                //// This does generate errors
                MyWidget {
                    subModel: model.modelData
                }
          
                //// This does generate errors
                // Text {
                //    property var data: modelData
                //    text: data.value
                // }
      
                //// This does NOT generate errors!
                // Text {
                //    text: modelData.value;
                // }
            }
        }
    }
}

And finally, here is the main.cpp file:

#include <QGuiApplication>
#include <QQmlApplicationEngine>
#include <QQmlContext>
#include <iostream>

#include "models.h"

int main(int argc, char* argv[])
{
    qmlRegisterUncreatableType<SubModel>(
        "MyLib.SubModel", 1, 0, "SubModel", "This type can't be created in QML");

    auto modelList = std::make_unique<ModelList>();

    QGuiApplication app(argc, argv);

    QQmlApplicationEngine engine;

    QQmlContext* rootContext = engine.rootContext();
    rootContext->setContextProperty("modelList", modelList.get());

    engine.load(QUrl(QStringLiteral("qrc:/main.qml")));

    return app.exec();
}

The firs time the "Create list" button is clicked it works fine. But the second time, the initial SubModule pointers are deleted and QML tries to access value attribute of nullptr, resulting in warning. Then m_subModels is replaced and the GUI is effectively updated. I don't understand why QML tries to access model while I emitted the beginResetModel() signal.

I know that I could change ModelList and make data() returns the actual int value instead of a SubModel pointer but this is not acceptable because I actually have several submodels in real code (separation of concerns).

Solutions I thought of:

  • Checking for if submodel !== null in QML but this doesn't look good
  • Replacing std::unique_ptr with new but this creates a memory leak
  • Using raw pointer with a Qt parent but memory will never be freed until exit
  • Using QSharedPointer with deleteLater() in destructor but this does not work all the time
  • Using QSharedPointer with setObjectOwnership(JavaScriptOwnership) in destructor but this seems hacky (not tested)
  • Using a QList<QObject*> instead of QAbstractListModel but this generated the same errors
  • Using a QQmlListProperty instead of QAbstractListModel but this generated the same errors
  • Re-using existing SubModel* instead of re-creating the list but this seems very convoluted

I have spent several days on this problem and I can't find a suitable solution. Would you have an idea please to keep my "SubModel" as it is and avoid QML warnings when the list is re-created?


I uploaded all project files here to easily reproduce the problem (using cmake for building).


Edit: The problem seems related to my usage of property because if I do not store the modelData as a property then there is no error.

Delgan
  • 18,571
  • 11
  • 90
  • 141
  • Have you though about putting printouts in the C++ constructor and destructor to be able to watch object lifetime? – Ross Rogers Mar 02 '22 at 15:36
  • @RossRogers I just did. What happens is: 1. New submodule is constructed 2. Old submodule is destroyed 3. QML error is triggered. I think QML is connected to "destroyed" QObject signal, it generates a UI refresh, but then the `property` of `MyWidget` is null. – Delgan Mar 02 '22 at 16:01
  • If the object is destroyed, then the QObject signal won't be transmitted to that object. Qt cleans up the signals on QObject destruction. If it did go to the destroyed QObject, it would be a use-after-free bug. – Ross Rogers Mar 02 '22 at 16:18
  • I think you have a bug in your [`Repeater`](https://doc.qt.io/qt-5/qml-qtquick-repeater.html). `subModel: model.modelData` should just be `subModel: modelData`. I think that's why `subModel` is `null` in your `MyWidget`. – Ross Rogers Mar 02 '22 at 16:21
  • @RossRogers Same error visibly using just `modelData`. – Delgan Mar 02 '22 at 16:25
  • I believe that `model.modelData` is accessing an invalid field on field `model` of your instance of `Repeater`. Whatever else is wrong, I believe you need to reference `modelData` without the `model` suffix. – Ross Rogers Mar 02 '22 at 16:26
  • @RossRogers I tried different ways to reference the model data, also trying by remote-access with `id` instead of using a `property`, but didn't have any luck. :( – Delgan Mar 02 '22 at 16:35
  • Do you want to wrap up your [S.S.C.C.E](http://sscce.org/) as a zip file and post it? When the @eyllanesc sees this problem I'm sure he'll know the solution :-D He's a machine. – Ross Rogers Mar 02 '22 at 16:43
  • 1
    @RossRogers Haha, thanks for your time in any case. :D I added the `.zip` to the question. The problem seems related to the `property`, it seems the `modelData` should not be stored this way. – Delgan Mar 02 '22 at 16:55
  • I'm flummoxed. Tried using `beginRemoveRows` and `beginInsertRows`. Have you tried [`qmlRegisterSingletonType`](https://doc.qt.io/qt-5/qqmlengine.html#qmlRegisterSingletonType) instead of `qmlRegisterUncreatableType`. Not sure it would change anything.. – Ross Rogers Mar 02 '22 at 17:52
  • 1
    Have you tried unreferencing the existing vector _after_ `endResetModel`? That way the objects get deleted when they are not referenced anymore in QML (note, that plus converting to QVector worked for me, not sure how to do it with std::vector) – Amfasis Mar 02 '22 at 19:06
  • @Delgan, when you really have to debug a problem, you can go nuclear by catching the stdout printout and looking up the stack: https://stackoverflow.com/a/8235612/20712 . The answer to that question has served me over the years. – Ross Rogers Mar 03 '22 at 04:11
  • @RossRogers Oh, I never though this kind of debugging solution was possible! Looks neat, thanks for sharing. I didn't manage to make it work properly, though, but I'll keep it in my toolbox! – Delgan Mar 03 '22 at 10:10
  • 2
    @Amfasis Unreferencing the vector after `endResetModel()` worked, thanks! I didn't even have to use `QVector`, I simply replaced `m_subModels = std::move(subModels);` with `m_subModels.swap(subModels);`. Would you mind posting your solution as an answer? I don't know how robust this solution is, though, but it's the closest to what I'm looking for. – Delgan Mar 03 '22 at 10:25

2 Answers2

1

Your code seems to be working fine. That's how Repeater works.

When your model is reset, Repeater starts to remove its QQuickItems one by one in reverse order.

// qtdeclarative/src/quick/items/qquickrepeater.cpp
if (d->model) {
    // We remove in reverse order deliberately; so that signals are emitted
    // with sensible indices.
    for (int i = d->deletables.count() - 1; i >= 0; --i) {
        if (QQuickItem *item = d->deletables.at(i)) {
            if (complete)
                emit itemRemoved(i, item);
            d->model->release(item);
        }
    }
    for (QQuickItem *item : qAsConst(d->deletables)) {
        if (item)
            item->setParentItem(nullptr);
    }
}

Depending on Repeater's strategy items may not be removed completely, but pooled for the future use.

At that very moment Item itself still exists, but you model data is not.

 Text {
     property var data: modelData // modelData is undefined at the moment
     text: data.value // "data" is undefined. You see the warning message 
 }

So, best you can do to fix warning messages is to check if modelData is defined.

 Text {
     property var data: modelData
     text: data ? data.value : ""
 }
samdavydov
  • 564
  • 3
  • 22
  • Why would the item fetch the data fields then? It shouldn't be doing a render then, should it? – Ross Rogers Mar 03 '22 at 03:56
  • Model notifies Repeater that the corresponding index (i.e. modelData in terms of Repeater) has changed. I agree that the overall behavior looks weird and it's not documented either. – samdavydov Mar 03 '22 at 06:24
  • Thanks for the answer. It helps understanding the idfference between "item" and "modelData". However, it's still not clear to me why QML would complain. If I remove the `property` and directly use `text: modelData.value` then no errors are generated, yet the `modelData` should be invalid according to your answer, right? – Delgan Mar 03 '22 at 10:17
  • The reason of no warnings in your case may be because "property var data: modelData" and "text: data.value" are handled in different part of the Qml engine code. Say, after beginReset and endReset correspondingly. I added this line inside Text element "onDataChanged: console.log(data, modelData, modelData.value)" and now I can see warning messages when trying to access invalid modelData. – samdavydov Mar 03 '22 at 16:23
1

For reasons explained by @samdavydov the Items in the Repeater stay during a model reset, but by unreferencing m_subModels the SubModels are deleted (because of the unique_ptr) and the views are invalidated because of the destroyed signal as you mention.

By swapping the two vectors during the reset, the old SubModels will stay a bit in memory, until the function exits, at which point the reset has already been completed and the new SubModels are being used:

void setSubModels(std::vector<std::unique_ptr<SubModel>> subModels)
{
   beginResetModel();
   m_subModels.swap(subModels);
   endResetModel();
} //RAII deletes old SubModel at this point

With credit to yourself for finding the swap after my mention of keeping them in memory ;-)

Amfasis
  • 3,932
  • 2
  • 18
  • 27