https://bugreports.qt.io/browse/QTBUG-125053 https://bugreports.qt.io/browse/QTBUG-127340 https://codereview.qt-project.org/c/qt/qtdeclarative/+/593122 --- a/src/qmlmodels/qqmldelegatemodel.cpp +++ b/src/qmlmodels/qqmldelegatemodel.cpp @@ -4,4 +4,6 @@ #include "qqmldelegatemodel_p_p.h" +#include + #include @@ -168,5 +170,4 @@ , m_incubatorCleanupScheduled(false) , m_waitingToFetchMore(false) - , m_maybeResetRoleNames(false) , m_cacheItems(nullptr) , m_items(nullptr) @@ -373,6 +374,4 @@ qmlobject_connect(aim, QAbstractItemModel, SIGNAL(layoutChanged(QList,QAbstractItemModel::LayoutChangeHint)), q, QQmlDelegateModel, SLOT(_q_layoutChanged(QList,QAbstractItemModel::LayoutChangeHint))); - QObject::connect(aim, &QAbstractItemModel::modelReset, q, &QQmlDelegateModel::handleModelReset); - QObject::connect(aim, &QAbstractItemModel::layoutChanged, q, &QQmlDelegateModel::_q_layoutChanged); } @@ -404,6 +403,4 @@ QObject::disconnect(aim, SIGNAL(layoutChanged(QList,QAbstractItemModel::LayoutChangeHint)), q, SLOT(_q_layoutChanged(QList,QAbstractItemModel::LayoutChangeHint))); - QObject::disconnect(aim, &QAbstractItemModel::modelReset, q, &QQmlDelegateModel::handleModelReset); - QObject::disconnect(aim, &QAbstractItemModel::layoutChanged, q, &QQmlDelegateModel::_q_layoutChanged); } @@ -429,4 +426,19 @@ d->requestMoreIfNecessary(); } + + // Since 837c2f18cd223707e7cedb213257b0158ea07146, we connect to modelAboutToBeReset + // rather than modelReset so that we can handle role name changes. _q_modelAboutToBeReset + // now connects modelReset to handleModelReset with a single shot connection instead. + // However, it's possible for user code to begin the reset before connectToAbstractItemModel is called + // (QTBUG-125053), in which case we connect to modelReset too late and handleModelReset is never called, + // resulting in delegates not being created in certain cases. + // So, we check at the earliest point we can if the model is in the process of being reset, + // and if so, connect modelReset to handleModelReset. + if (d->m_adaptorModel.adaptsAim()) { + auto *aim = d->m_adaptorModel.aim(); + auto *aimPrivate = QAbstractItemModelPrivate::get(aim); + if (aimPrivate->resetting) + QObject::connect(aim, &QAbstractItemModel::modelReset, this, &QQmlDelegateModel::handleModelReset, Qt::SingleShotConnection); + } } @@ -1915,26 +1927,23 @@ if (!d->m_adaptorModel.adaptsAim()) return; - - /* - roleNames are generally guaranteed to be stable (given that QAIM has no - change signal for them), except that resetting the model is allowed to - invalidate them (QTBUG-32132). DelegateModel must take this into account by - snapshotting the current roleNames before the model is reset. - Afterwards, if we detect that roleNames has changed, we throw the - current model set up away and rebuild everything from scratch – it is - unlikely that a more efficient implementation would be worth it. - - If we detect no changes, we simply use the existing logic to handle the - model reset. - - This (role name resetting) logic relies on the fact that - modelAboutToBeReset must be followed by a modelReset signal before any - further modelAboutToBeReset can occur. However, it's possible for user - code to begin the reset before connectToAbstractItemModel is called - (QTBUG-125053), in which case we don't attempt to reset the role names. - */ - Q_ASSERT(!d->m_maybeResetRoleNames); - d->m_maybeResetRoleNames = true; - d->m_roleNamesBeforeReset = d->m_adaptorModel.aim()->roleNames(); + auto aim = d->m_adaptorModel.aim(); + auto oldRoleNames = aim->roleNames(); + // this relies on the fact that modelAboutToBeReset must be followed + // by a modelReset signal before any further modelAboutToBeReset can occur + QObject::connect(aim, &QAbstractItemModel::modelReset, this, [this, d, oldRoleNames, aim](){ + if (!d->m_adaptorModel.adaptsAim() || d->m_adaptorModel.aim() != aim) + return; + if (oldRoleNames == aim->roleNames()) { + // if the rolenames stayed the same (most common case), then we don't have + // to throw away all the setup that we did + handleModelReset(); + } else { + // If they did change, we give up and just start from scratch via setMode + setModel(QVariant::fromValue(model())); + // but we still have to call handleModelReset, otherwise views will + // not refresh + handleModelReset(); + } + }, Qt::SingleShotConnection); } @@ -1946,21 +1955,4 @@ int oldCount = d->m_count; - - if (d->m_maybeResetRoleNames) { - auto aim = d->m_adaptorModel.aim(); - if (!d->m_adaptorModel.adaptsAim() || d->m_adaptorModel.aim() != aim) - return; - - // If the role names stayed the same (most common case), then we don't have - // to throw away all the setup that we did. - // If they did change, we give up and just start from scratch via setModel. - // We do this before handling the reset to ensure that views refresh. - if (aim->roleNames() != d->m_roleNamesBeforeReset) - setModel(QVariant::fromValue(model())); - - d->m_maybeResetRoleNames = false; - d->m_roleNamesBeforeReset.clear(); - } - d->m_adaptorModel.rootIndex = QModelIndex(); --- a/src/qmlmodels/qqmldelegatemodel_p_p.h +++ b/src/qmlmodels/qqmldelegatemodel_p_p.h @@ -335,5 +335,4 @@ QList m_finishedIncubating; QList m_watchedRoles; - QHash m_roleNamesBeforeReset; QString m_filterGroup; @@ -349,5 +348,4 @@ bool m_incubatorCleanupScheduled : 1; bool m_waitingToFetchMore : 1; - bool m_maybeResetRoleNames : 1; union { --- a/tests/auto/qml/qqmldelegatemodel/data/proxyModelWithDelayedSourceModelInListView.qml +++ b/tests/auto/qml/qqmldelegatemodel/data/proxyModelWithDelayedSourceModelInListView.qml @@ -0,0 +1,30 @@ +import QtQuick +import Test + +Window { + id: root + title: listView.count + + property alias listView: listView + property ProxySourceModel connectionModel: null + + Component { + id: modelComponent + ProxySourceModel {} + } + + ListView { + id: listView + anchors.fill: parent + + delegate: Text { + text: model.Name + } + + model: ProxyModel { + sourceModel: root.connectionModel + } + } + + Component.onCompleted: root.connectionModel = modelComponent.createObject(root) +} --- a/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp +++ b/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp @@ -4,4 +4,5 @@ #include #include +#include #include #include @@ -52,4 +53,5 @@ void clearCacheDuringInsertion(); void viewUpdatedOnDelegateChoiceAffectingRoleChange(); + void proxyModelWithDelayedSourceModelInListView(); }; @@ -732,4 +734,77 @@ } +class ProxySourceModel : public QAbstractListModel +{ + Q_OBJECT + QML_ELEMENT +public: + explicit ProxySourceModel(QObject *parent = nullptr) + : QAbstractListModel(parent) + { + for (int i = 0; i < rows; ++i) { + beginInsertRows(QModelIndex(), i, i); + endInsertRows(); + } + } + + ~ProxySourceModel() override = default; + + int rowCount(const QModelIndex &) const override + { + return rows; + } + + QVariant data(const QModelIndex &, int ) const override + { + return "Hello"; + } + + QHash roleNames() const override + { + QHash roles = QAbstractListModel::roleNames(); + roles[Qt::UserRole + 1] = "Name"; + + return roles; + } + + static const int rows = 1; +}; + +class ProxyModel : public QSortFilterProxyModel +{ + Q_OBJECT + QML_ELEMENT + Q_PROPERTY(QAbstractItemModel *sourceModel READ sourceModel WRITE setSourceModel) + +public: + explicit ProxyModel(QObject *parent = nullptr) + : QSortFilterProxyModel(parent) + { + } + + ~ProxyModel() override = default; +}; + +// Checks that the correct amount of delegates are created when using a proxy +// model whose source model is set after a delay. +void tst_QQmlDelegateModel::proxyModelWithDelayedSourceModelInListView() +{ + QTest::failOnWarning(); + + qmlRegisterTypesAndRevisions("Test", 1); + qmlRegisterTypesAndRevisions("Test", 1); + + QQuickApplicationHelper helper(this, "proxyModelWithDelayedSourceModelInListView.qml"); + QVERIFY2(helper.ready, helper.failureMessage()); + QQuickWindow *window = helper.window; + window->show(); + QVERIFY(QTest::qWaitForWindowExposed(window)); + + auto *listView = window->property("listView").value(); + QVERIFY(listView); + const auto delegateModel = QQuickItemViewPrivate::get(listView)->model; + QTRY_COMPARE(listView->count(), 1); +} + QTEST_MAIN(tst_QQmlDelegateModel)