From 60d589ccddb036e84883a6c2ef63a5292c8ad022 Mon Sep 17 00:00:00 2001 From: Richard Moe Gustavsen Date: Mon, 6 Nov 2017 16:13:13 +0100 Subject: QQmlIntanceModel: use QQmlIncubator::IncubationMode instead of bool to specify incubation mode The current implementation would pass a boolean to signal if asynchronous or synchronous incubation should be used to create an item. The problem with this approach is that passing 'synchronous" would translate to QQmlIncubation::AsynchronousIfNested later down the chain. This meant that even if the caller requested synchronous incubation, it could end up with asynchronous incubation anyway, e.g if an async parent incubator was active at the time of the call. And this can easily come as an unhandled supprise for the caller, and as such, cause unforseen bugs. This patch is a first of a set of patches that is done to fix the bug reported in the task below. It will not change any behavior, it is written to preserve the logic exactly as it were, just as a preparation for subsequent patches. It makes it explicit at the call location what incubation mode will be used, and especially make it clear whenever the AsynchronousIfNested flag is in play. Task-number: QTBUG-61537 Change-Id: I8b3ba5438ebb2cd59983a098bd8ceeeb844da87b Reviewed-by: J-P Nurmi --- src/qml/types/qqmldelegatemodel.cpp | 21 +++++++++------------ src/qml/types/qqmldelegatemodel_p.h | 3 ++- src/qml/types/qqmldelegatemodel_p_p.h | 4 ++-- src/qml/types/qqmlinstantiator.cpp | 4 ++-- src/qml/types/qqmlobjectmodel.cpp | 2 +- src/qml/types/qqmlobjectmodel_p.h | 5 +++-- src/quick/items/qquickgridview.cpp | 6 ++++-- src/quick/items/qquickitemview.cpp | 10 +++++----- src/quick/items/qquickitemview_p_p.h | 2 +- src/quick/items/qquicklistview.cpp | 6 ++++-- src/quick/items/qquickpathview.cpp | 2 +- src/quick/items/qquickrepeater.cpp | 6 +++--- .../tst_qquickvisualdatamodel.cpp | 18 +++++++++--------- 13 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/qml/types/qqmldelegatemodel.cpp b/src/qml/types/qqmldelegatemodel.cpp index e3906f2a7e..4da7199d1e 100644 --- a/src/qml/types/qqmldelegatemodel.cpp +++ b/src/qml/types/qqmldelegatemodel.cpp @@ -931,7 +931,7 @@ void QQmlDelegateModelPrivate::setInitialState(QQDMIncubationTask *incubationTas emitInitItem(incubationTask, cacheItem->object); } -QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, bool asynchronous) +QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, QQmlIncubator::IncubationMode incubationMode) { if (!m_delegate || index < 0 || index >= m_compositor.count(group)) { qWarning() << "DelegateModel::item: index out range" << index << m_compositor.count(group); @@ -962,7 +962,8 @@ QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, bo cacheItem->referenceObject(); if (cacheItem->incubationTask) { - if (!asynchronous && cacheItem->incubationTask->incubationMode() == QQmlIncubator::Asynchronous) { + bool sync = (incubationMode == QQmlIncubator::Synchronous || incubationMode == QQmlIncubator::AsynchronousIfNested); + if (sync && cacheItem->incubationTask->incubationMode() == QQmlIncubator::Asynchronous) { // previously requested async - now needed immediately cacheItem->incubationTask->forceCompletion(); } @@ -971,7 +972,7 @@ QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, bo cacheItem->scriptRef += 1; - cacheItem->incubationTask = new QQDMIncubationTask(this, asynchronous ? QQmlIncubator::Asynchronous : QQmlIncubator::AsynchronousIfNested); + cacheItem->incubationTask = new QQDMIncubationTask(this, incubationMode); cacheItem->incubationTask->incubating = cacheItem; cacheItem->incubationTask->clear(); @@ -1024,7 +1025,7 @@ QObject *QQmlDelegateModelPrivate::object(Compositor::Group group, int index, bo to ensure a reference is held. Any call to item() which returns a valid item must be matched by a call to release() in order to destroy the item. */ -QObject *QQmlDelegateModel::object(int index, bool asynchronous) +QObject *QQmlDelegateModel::object(int index, QQmlIncubator::IncubationMode incubationMode) { Q_D(QQmlDelegateModel); if (!d->m_delegate || index < 0 || index >= d->m_compositor.count(d->m_compositorGroup)) { @@ -1032,11 +1033,7 @@ QObject *QQmlDelegateModel::object(int index, bool asynchronous) return 0; } - QObject *object = d->object(d->m_compositorGroup, index, asynchronous); - if (!object) - return 0; - - return object; + return d->object(d->m_compositorGroup, index, incubationMode); } QString QQmlDelegateModelPrivate::stringValue(Compositor::Group group, int index, const QString &name) @@ -2659,7 +2656,7 @@ void QQmlDelegateModelGroup::create(QQmlV4Function *args) return; } - QObject *object = model->object(group, index, false); + QObject *object = model->object(group, index, QQmlIncubator::AsynchronousIfNested); if (object) { QVector inserts; Compositor::iterator it = model->m_compositor.find(group, index); @@ -3147,7 +3144,7 @@ bool QQmlPartsModel::isValid() const return m_model->isValid(); } -QObject *QQmlPartsModel::object(int index, bool asynchronous) +QObject *QQmlPartsModel::object(int index, QQmlIncubator::IncubationMode incubationMode) { QQmlDelegateModelPrivate *model = QQmlDelegateModelPrivate::get(m_model); @@ -3156,7 +3153,7 @@ QObject *QQmlPartsModel::object(int index, bool asynchronous) return 0; } - QObject *object = model->object(m_compositorGroup, index, asynchronous); + QObject *object = model->object(m_compositorGroup, index, incubationMode); if (QQuickPackage *package = qmlobject_cast(object)) { QObject *part = package->part(m_part); diff --git a/src/qml/types/qqmldelegatemodel_p.h b/src/qml/types/qqmldelegatemodel_p.h index 186144d107..5ae2d48969 100644 --- a/src/qml/types/qqmldelegatemodel_p.h +++ b/src/qml/types/qqmldelegatemodel_p.h @@ -54,6 +54,7 @@ #include #include #include +#include #include #include @@ -109,7 +110,7 @@ public: int count() const override; bool isValid() const override { return delegate() != 0; } - QObject *object(int index, bool asynchronous = false) override; + QObject *object(int index, QQmlIncubator::IncubationMode incubationMode = QQmlIncubator::AsynchronousIfNested) override; ReleaseFlags release(QObject *object) override; void cancel(int index) override; QString stringValue(int index, const QString &role) override; diff --git a/src/qml/types/qqmldelegatemodel_p_p.h b/src/qml/types/qqmldelegatemodel_p_p.h index 0f8438870a..a6a7853116 100644 --- a/src/qml/types/qqmldelegatemodel_p_p.h +++ b/src/qml/types/qqmldelegatemodel_p_p.h @@ -261,7 +261,7 @@ public: void connectModel(QQmlAdaptorModel *model); void requestMoreIfNecessary(); - QObject *object(Compositor::Group group, int index, bool asynchronous); + QObject *object(Compositor::Group group, int index, QQmlIncubator::IncubationMode incubationMode); QQmlDelegateModel::ReleaseFlags release(QObject *object); QString stringValue(Compositor::Group group, int index, const QString &name); void emitCreatedPackage(QQDMIncubationTask *incubationTask, QQuickPackage *package); @@ -357,7 +357,7 @@ public: int count() const override; bool isValid() const override; - QObject *object(int index, bool asynchronous = false) override; + QObject *object(int index, QQmlIncubator::IncubationMode incubationMode = QQmlIncubator::AsynchronousIfNested) override; ReleaseFlags release(QObject *item) override; QString stringValue(int index, const QString &role) override; QList watchedRoles() const { return m_watchedRoles; } diff --git a/src/qml/types/qqmlinstantiator.cpp b/src/qml/types/qqmlinstantiator.cpp index 2de5875deb..6e2b66aea7 100644 --- a/src/qml/types/qqmlinstantiator.cpp +++ b/src/qml/types/qqmlinstantiator.cpp @@ -85,7 +85,7 @@ void QQmlInstantiatorPrivate::clear() QObject *QQmlInstantiatorPrivate::modelObject(int index, bool async) { requestedIndex = index; - QObject *o = instanceModel->object(index, async); + QObject *o = instanceModel->object(index, async ? QQmlIncubator::Asynchronous : QQmlIncubator::AsynchronousIfNested); requestedIndex = -1; return o; } @@ -123,7 +123,7 @@ void QQmlInstantiatorPrivate::_q_createdItem(int idx, QObject* item) if (objects.contains(item)) //Case when it was created synchronously in regenerate return; if (requestedIndex != idx) // Asynchronous creation, reference the object - (void)instanceModel->object(idx, false); + (void)instanceModel->object(idx); item->setParent(q); if (objects.size() < idx + 1) { int modelCount = instanceModel->count(); diff --git a/src/qml/types/qqmlobjectmodel.cpp b/src/qml/types/qqmlobjectmodel.cpp index dcd0360199..dcb41469ba 100644 --- a/src/qml/types/qqmlobjectmodel.cpp +++ b/src/qml/types/qqmlobjectmodel.cpp @@ -267,7 +267,7 @@ bool QQmlObjectModel::isValid() const return true; } -QObject *QQmlObjectModel::object(int index, bool) +QObject *QQmlObjectModel::object(int index, QQmlIncubator::IncubationMode) { Q_D(QQmlObjectModel); QQmlObjectModelPrivate::Item &item = d->children[index]; diff --git a/src/qml/types/qqmlobjectmodel_p.h b/src/qml/types/qqmlobjectmodel_p.h index fc4c03874f..5e4a9c686f 100644 --- a/src/qml/types/qqmlobjectmodel_p.h +++ b/src/qml/types/qqmlobjectmodel_p.h @@ -52,6 +52,7 @@ // #include +#include #include #include @@ -74,7 +75,7 @@ public: virtual int count() const = 0; virtual bool isValid() const = 0; - virtual QObject *object(int index, bool asynchronous=false) = 0; + virtual QObject *object(int index, QQmlIncubator::IncubationMode incubationMode = QQmlIncubator::AsynchronousIfNested) = 0; virtual ReleaseFlags release(QObject *object) = 0; virtual void cancel(int) {} virtual QString stringValue(int, const QString &) = 0; @@ -113,7 +114,7 @@ public: int count() const override; bool isValid() const override; - QObject *object(int index, bool asynchronous = false) override; + QObject *object(int index, QQmlIncubator::IncubationMode incubationMode = QQmlIncubator::AsynchronousIfNested) override; ReleaseFlags release(QObject *object) override; QString stringValue(int index, const QString &role) override; void setWatchedRoles(const QList &) override {} diff --git a/src/quick/items/qquickgridview.cpp b/src/quick/items/qquickgridview.cpp index c570b95a21..e59c579790 100644 --- a/src/quick/items/qquickgridview.cpp +++ b/src/quick/items/qquickgridview.cpp @@ -511,9 +511,11 @@ bool QQuickGridViewPrivate::addVisibleItems(qreal fillFrom, qreal fillTo, qreal FxGridItemSG *item = 0; bool changed = false; + QQmlIncubator::IncubationMode incubationMode = doBuffer ? QQmlIncubator::Asynchronous : QQmlIncubator::AsynchronousIfNested; + while (modelIndex < model->count() && rowPos <= fillTo + rowSize()*(columns - colNum)/(columns+1)) { qCDebug(lcItemViewDelegateLifecycle) << "refill: append item" << modelIndex << colPos << rowPos; - if (!(item = static_cast(createItem(modelIndex, doBuffer)))) + if (!(item = static_cast(createItem(modelIndex, incubationMode)))) break; if (!transitioner || !transitioner->canTransition(QQuickItemViewTransitioner::PopulateTransition, true)) // pos will be set by layoutVisibleItems() item->setPosition(colPos, rowPos, true); @@ -548,7 +550,7 @@ bool QQuickGridViewPrivate::addVisibleItems(qreal fillFrom, qreal fillTo, qreal colPos = colNum * colSize(); while (visibleIndex > 0 && rowPos + rowSize() - 1 >= fillFrom - rowSize()*(colNum+1)/(columns+1)){ qCDebug(lcItemViewDelegateLifecycle) << "refill: prepend item" << visibleIndex-1 << "top pos" << rowPos << colPos; - if (!(item = static_cast(createItem(visibleIndex-1, doBuffer)))) + if (!(item = static_cast(createItem(visibleIndex-1, incubationMode)))) break; --visibleIndex; if (!transitioner || !transitioner->canTransition(QQuickItemViewTransitioner::PopulateTransition, true)) // pos will be set by layoutVisibleItems() diff --git a/src/quick/items/qquickitemview.cpp b/src/quick/items/qquickitemview.cpp index c203f389ae..82dbf04090 100644 --- a/src/quick/items/qquickitemview.cpp +++ b/src/quick/items/qquickitemview.cpp @@ -1729,7 +1729,7 @@ void QQuickItemViewPrivate::updateCurrent(int modelIndex) FxViewItem *oldCurrentItem = currentItem; int oldCurrentIndex = currentIndex; currentIndex = modelIndex; - currentItem = createItem(modelIndex, false); + currentItem = createItem(modelIndex, QQmlIncubator::AsynchronousIfNested); if (oldCurrentItem && oldCurrentItem->attached && (!currentItem || oldCurrentItem->item != currentItem->item)) oldCurrentItem->attached->setIsCurrentItem(false); if (currentItem) { @@ -2325,11 +2325,11 @@ void QQuickItemViewPrivate::viewItemTransitionFinished(QQuickItemViewTransitiona When the item becomes available, refill() will be called and the item will be returned on the next call to createItem(). */ -FxViewItem *QQuickItemViewPrivate::createItem(int modelIndex, bool asynchronous) +FxViewItem *QQuickItemViewPrivate::createItem(int modelIndex, QQmlIncubator::IncubationMode incubationMode) { Q_Q(QQuickItemView); - if (requestedIndex == modelIndex && asynchronous) + if (requestedIndex == modelIndex && incubationMode == QQmlIncubator::Asynchronous) return 0; for (int i=0; iobject(modelIndex, asynchronous); + QObject* object = model->object(modelIndex, incubationMode); QQuickItem *item = qmlobject_cast(object); if (!item) { if (object) { diff --git a/src/quick/items/qquickitemview_p_p.h b/src/quick/items/qquickitemview_p_p.h index 2c04022cde..622ebb430f 100644 --- a/src/quick/items/qquickitemview_p_p.h +++ b/src/quick/items/qquickitemview_p_p.h @@ -206,7 +206,7 @@ public: void refill(qreal from, qreal to); void mirrorChange() override; - FxViewItem *createItem(int modelIndex, bool asynchronous = false); + FxViewItem *createItem(int modelIndex,QQmlIncubator::IncubationMode incubationMode = QQmlIncubator::AsynchronousIfNested); virtual bool releaseItem(FxViewItem *item); QQuickItem *createHighlightItem() const; diff --git a/src/quick/items/qquicklistview.cpp b/src/quick/items/qquicklistview.cpp index a6236d9801..d924bc4460 100644 --- a/src/quick/items/qquicklistview.cpp +++ b/src/quick/items/qquicklistview.cpp @@ -669,11 +669,13 @@ bool QQuickListViewPrivate::addVisibleItems(qreal fillFrom, qreal fillTo, qreal } } + QQmlIncubator::IncubationMode incubationMode = doBuffer ? QQmlIncubator::Asynchronous : QQmlIncubator::AsynchronousIfNested; + bool changed = false; FxListItemSG *item = 0; qreal pos = itemEnd; while (modelIndex < model->count() && pos <= fillTo) { - if (!(item = static_cast(createItem(modelIndex, doBuffer)))) + if (!(item = static_cast(createItem(modelIndex, incubationMode)))) break; qCDebug(lcItemViewDelegateLifecycle) << "refill: append item" << modelIndex << "pos" << pos << "buffer" << doBuffer << "item" << (QObject *)(item->item); if (!transitioner || !transitioner->canTransition(QQuickItemViewTransitioner::PopulateTransition, true)) // pos will be set by layoutVisibleItems() @@ -690,7 +692,7 @@ bool QQuickListViewPrivate::addVisibleItems(qreal fillFrom, qreal fillTo, qreal return changed; while (visibleIndex > 0 && visibleIndex <= model->count() && visiblePos > fillFrom) { - if (!(item = static_cast(createItem(visibleIndex-1, doBuffer)))) + if (!(item = static_cast(createItem(visibleIndex-1, incubationMode)))) break; qCDebug(lcItemViewDelegateLifecycle) << "refill: prepend item" << visibleIndex-1 << "current top pos" << visiblePos << "buffer" << doBuffer << "item" << (QObject *)(item->item); --visibleIndex; diff --git a/src/quick/items/qquickpathview.cpp b/src/quick/items/qquickpathview.cpp index aac2b0296a..b9fea974ce 100644 --- a/src/quick/items/qquickpathview.cpp +++ b/src/quick/items/qquickpathview.cpp @@ -129,7 +129,7 @@ QQuickItem *QQuickPathViewPrivate::getItem(int modelIndex, qreal z, bool async) requestedIndex = modelIndex; requestedZ = z; inRequest = true; - QObject *object = model->object(modelIndex, async); + QObject *object = model->object(modelIndex, async ? QQmlIncubator::Asynchronous : QQmlIncubator::AsynchronousIfNested); QQuickItem *item = qmlobject_cast(object); if (!item) { if (object) { diff --git a/src/quick/items/qquickrepeater.cpp b/src/quick/items/qquickrepeater.cpp index ebf6e9c5cb..658a7de3d4 100644 --- a/src/quick/items/qquickrepeater.cpp +++ b/src/quick/items/qquickrepeater.cpp @@ -401,7 +401,7 @@ void QQuickRepeater::regenerate() void QQuickRepeaterPrivate::requestItems() { for (int i = 0; i < itemCount; i++) { - QObject *object = model->object(i, false); + QObject *object = model->object(i, QQmlIncubator::AsynchronousIfNested); if (object) model->release(object); } @@ -410,7 +410,7 @@ void QQuickRepeaterPrivate::requestItems() void QQuickRepeater::createdItem(int index, QObject *) { Q_D(QQuickRepeater); - QObject *object = d->model->object(index, false); + QObject *object = d->model->object(index, QQmlIncubator::AsynchronousIfNested); QQuickItem *item = qmlobject_cast(object); emit itemAdded(index, item); } @@ -508,7 +508,7 @@ void QQuickRepeater::modelUpdated(const QQmlChangeSet &changeSet, bool reset) int modelIndex = index + i; ++d->itemCount; d->deletables.insert(modelIndex, 0); - QObject *object = d->model->object(modelIndex, false); + QObject *object = d->model->object(modelIndex, QQmlIncubator::AsynchronousIfNested); if (object) d->model->release(object); } diff --git a/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp b/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp index cabfb97914..2280f75518 100644 --- a/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp +++ b/tests/auto/quick/qquickvisualdatamodel/tst_qquickvisualdatamodel.cpp @@ -4013,7 +4013,7 @@ void tst_qquickvisualdatamodel::asynchronousInsert() connect(visualModel, SIGNAL(createdItem(int,QObject*)), &requester, SLOT(createdItem(int,QObject*))); connect(visualModel, SIGNAL(destroyingItem(QObject*)), &requester, SLOT(destroyingItem(QObject*))); - QQuickItem *item = qobject_cast(visualModel->object(requestIndex, true)); + QQuickItem *item = qobject_cast(visualModel->object(requestIndex, QQmlIncubator::Asynchronous)); QVERIFY(!item); QVERIFY(!requester.itemInitialized); @@ -4025,7 +4025,7 @@ void tst_qquickvisualdatamodel::asynchronousInsert() newItems.append(qMakePair(QLatin1String("New item") + QString::number(i), QString(QLatin1String("")))); model.insertItems(insertIndex, newItems); - item = qobject_cast(visualModel->object(completeIndex, false)); + item = qobject_cast(visualModel->object(completeIndex)); QVERIFY(item); QCOMPARE(requester.itemInitialized, item); @@ -4078,7 +4078,7 @@ void tst_qquickvisualdatamodel::asynchronousRemove() connect(visualModel, SIGNAL(createdItem(int,QObject*)), &requester, SLOT(createdItem(int,QObject*))); connect(visualModel, SIGNAL(destroyingItem(QObject*)), &requester, SLOT(destroyingItem(QObject*))); - QQuickItem *item = qobject_cast(visualModel->object(requestIndex, true)); + QQuickItem *item = qobject_cast(visualModel->object(requestIndex, QQmlIncubator::Asynchronous)); QVERIFY(!item); QVERIFY(!requester.itemInitialized); @@ -4099,7 +4099,7 @@ void tst_qquickvisualdatamodel::asynchronousRemove() QVERIFY(!requester.itemCreated); QVERIFY(!requester.itemDestroyed); } else { - item = qobject_cast(visualModel->object(completeIndex, false)); + item = qobject_cast(visualModel->object(completeIndex)); QVERIFY(item); QCOMPARE(requester.itemInitialized, item); @@ -4157,7 +4157,7 @@ void tst_qquickvisualdatamodel::asynchronousMove() connect(visualModel, SIGNAL(createdItem(int,QObject*)), &requester, SLOT(createdItem(int,QObject*))); connect(visualModel, SIGNAL(destroyingItem(QObject*)), &requester, SLOT(destroyingItem(QObject*))); - QQuickItem *item = qobject_cast(visualModel->object(requestIndex, true)); + QQuickItem *item = qobject_cast(visualModel->object(requestIndex, QQmlIncubator::Asynchronous)); QVERIFY(!item); QVERIFY(!requester.itemInitialized); @@ -4166,7 +4166,7 @@ void tst_qquickvisualdatamodel::asynchronousMove() model.moveItems(from, to, count); - item = qobject_cast(visualModel->object(completeIndex, false)); + item = qobject_cast(visualModel->object(completeIndex)); QVERIFY(item); @@ -4200,7 +4200,7 @@ void tst_qquickvisualdatamodel::asynchronousCancel() QQmlDelegateModel *visualModel = qobject_cast(c.create()); QVERIFY(visualModel); - QQuickItem *item = qobject_cast(visualModel->object(requestIndex, true)); + QQuickItem *item = qobject_cast(visualModel->object(requestIndex, QQmlIncubator::Asynchronous)); QVERIFY(!item); QCOMPARE(controller.incubatingObjectCount(), 1); @@ -4225,7 +4225,7 @@ void tst_qquickvisualdatamodel::invalidContext() QQmlDelegateModel *visualModel = qobject_cast(c.create(context.data())); QVERIFY(visualModel); - QQuickItem *item = qobject_cast(visualModel->object(4, false)); + QQuickItem *item = qobject_cast(visualModel->object(4)); QVERIFY(item); visualModel->release(item); @@ -4233,7 +4233,7 @@ void tst_qquickvisualdatamodel::invalidContext() model.insertItem(4, "new item", ""); - item = qobject_cast(visualModel->object(4, false)); + item = qobject_cast(visualModel->object(4)); QVERIFY(!item); } -- cgit v1.2.3