Repeater: Don't rely on the createFrom variable

since this breaks for asynchronous models, because item creation order
is not guaranteed. We always have the index
for what item to create, so we do not need it either.

Change-Id: Ib8ce25ac342f5cce4784c56e6a91cf70136566b3
Task-number: QTBUG-38879
Task-number: QTBUG-39001
Task-number: QTBUG-44250
Reviewed-by: Ulf Hermann <ulf.hermann@theqtcompany.com>

diff --git a/src/quick/items/qquickrepeater.cpp b/src/quick/items/qquickrepeater.cpp
index c5fea34..0168d73 100644 (file)
--- a/src/quick/items/qquickrepeater.cpp
+++ b/src/quick/items/qquickrepeater.cpp
@@ -44,7 +44,11 @@
 QT_BEGIN_NAMESPACE
 
 QQuickRepeaterPrivate::QQuickRepeaterPrivate()
-    : model(0), ownModel(false), inRequest(false), dataSourceIsObject(false), delegateValidated(false), itemCount(0), createFrom(-1)
+    : model(0)
+    , ownModel(false)
+    , dataSourceIsObject(false)
+    , delegateValidated(false)
+    , itemCount(0)
 {
 }
 
@@ -378,57 +382,51 @@ void QQuickRepeater::regenerate()
 
     d->itemCount = count();
     d->deletables.resize(d->itemCount);
-    d->createFrom = 0;
-    d->createItems();
+    d->requestItems();
 }
 
-void QQuickRepeaterPrivate::createItems()
+void QQuickRepeaterPrivate::requestItems()
 {
-    Q_Q(QQuickRepeater);
-    if (createFrom == -1)
-        return;
-    inRequest = true;
-    for (int ii = createFrom; ii < itemCount; ++ii) {
-        if (!deletables.at(ii)) {
-            QObject *object = model->object(ii, false);
-            QQuickItem *item = qmlobject_cast<QQuickItem*>(object);
-            if (!item) {
-                if (object) {
-                    model->release(object);
-                    if (!delegateValidated) {
-                        delegateValidated = true;
-                        QObject* delegate = q->delegate();
-                        qmlInfo(delegate ? delegate : q) << QQuickRepeater::tr("Delegate must be of Item type");
-                    }
+    for (int i = 0; i < itemCount; i++) {
+        QObject *object = model->object(i, false);
+        if (object)
+            model->release(object);
+    }
+}
+
+void QQuickRepeater::createdItem(int index, QObject *)
+{
+    Q_D(QQuickRepeater);
+    if (!d->deletables.at(index)) {
+        QObject *object = d->model->object(index, false);
+        QQuickItem *item = qmlobject_cast<QQuickItem*>(object);
+        if (!item) {
+            if (object) {
+                d->model->release(object);
+                if (!d->delegateValidated) {
+                    d->delegateValidated = true;
+                    QObject* delegate = this->delegate();
+                    qmlInfo(delegate ? delegate : this) << QQuickRepeater::tr("Delegate must be of Item type");
                 }
-                createFrom = ii;
-                break;
             }
-            deletables[ii] = item;
-            item->setParentItem(q->parentItem());
-            if (ii > 0 && deletables.at(ii-1)) {
-                item->stackAfter(deletables.at(ii-1));
-            } else {
-                QQuickItem *after = q;
-                for (int si = ii+1; si < itemCount; ++si) {
-                    if (deletables.at(si)) {
-                        after = deletables.at(si);
-                        break;
-                    }
+            return;
+        }
+        d->deletables[index] = item;
+        item->setParentItem(parentItem());
+        if (index > 0 && d->deletables.at(index-1)) {
+            item->stackAfter(d->deletables.at(index-1));
+        } else {
+            QQuickItem *after = this;
+            for (int si = index+1; si < d->itemCount; ++si) {
+                if (d->deletables.at(si)) {
+                    after = d->deletables.at(si);
+                    break;
                 }
-                item->stackBefore(after);
             }
-            emit q->itemAdded(ii, item);
+            item->stackBefore(after);
         }
+        emit itemAdded(index, item);
     }
-    inRequest = false;
-}
-
-void QQuickRepeater::createdItem(int, QObject *)
-{
-    Q_D(QQuickRepeater);
-    if (!d->inRequest)
-        d->createItems();
 }
 
 void QQuickRepeater::initItem(int, QObject *object)
@@ -476,7 +474,6 @@ void QQuickRepeater::modelUpdated(const QQmlChangeSet &changeSet, bool reset)
         difference -= remove.count;
     }
 
-    d->createFrom = -1;
     foreach (const QQmlChangeSet::Change &insert, changeSet.inserts()) {
         int index = qMin(insert.index, d->deletables.count());
         if (insert.isMove()) {
@@ -491,14 +488,13 @@ void QQuickRepeater::modelUpdated(const QQmlChangeSet &changeSet, bool reset)
             int modelIndex = index + i;
             ++d->itemCount;
             d->deletables.insert(modelIndex, 0);
-            if (d->createFrom == -1)
-                d->createFrom = modelIndex;
+            QObject *object = d->model->object(modelIndex, false);
+            if (object)
+                d->model->release(object);
         }
         difference += insert.count;
     }
 
-    d->createItems();
-
     if (difference != 0)
         emit countChanged();
 }

diff --git a/src/quick/items/qquickrepeater_p_p.h b/src/quick/items/qquickrepeater_p_p.h
index 026633f..732ba4d 100644 (file)
--- a/src/quick/items/qquickrepeater_p_p.h
+++ b/src/quick/items/qquickrepeater_p_p.h
@@ -63,17 +63,15 @@ public:
     ~QQuickRepeaterPrivate();
 
 private:
-    void createItems();
+    void requestItems();
 
     QPointer<QQmlInstanceModel> model;
     QVariant dataSource;
     QPointer<QObject> dataSourceAsObject;
     bool ownModel : 1;
-    bool inRequest : 1;
     bool dataSourceIsObject : 1;
     bool delegateValidated : 1;
     int itemCount;
-    int createFrom;
 
     QVector<QPointer<QQuickItem> > deletables;
 };

diff --git a/tests/auto/quick/qquickrepeater/data/destroycount.qml b/tests/auto/quick/qquickrepeater/data/destroycount.qml
new file mode 100644 (file)
index 0000000..6aaed85
--- /dev/null
+++ b/tests/auto/quick/qquickrepeater/data/destroycount.qml
@@ -0,0 +1,22 @@
+import QtQuick 2.0
+
+Item {
+    width: 360
+    height: 480
+
+    Repeater {
+        id: repeater
+        objectName: "repeater"
+        anchors.fill: parent
+        property var componentCount: 0
+        Component {
+            Text {
+                y: index*20
+                text: index
+                Component.onCompleted: repeater.componentCount++;
+                Component.onDestruction: repeater.componentCount--;
+            }
+        }
+    }
+
+}

diff --git a/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp b/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp
index 81cd871..b2039bd 100644 (file)
--- a/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp
+++ b/tests/auto/quick/qquickrepeater/tst_qquickrepeater.cpp
@@ -75,6 +75,7 @@ private slots:
     void invalidContextCrash();
     void jsArrayChange();
     void clearRemovalOrder();
+    void destroyCount();
 };
 
 class TestObject : public QObject
@@ -293,6 +294,18 @@ void tst_QQuickRepeater::dataModel_adding()
     QCOMPARE(addedSpy.at(0).at(1).value<QQuickItem*>(), container->childItems().at(2));
     addedSpy.clear();
 
+    //insert in middle multiple
+    int childItemsSize = container->childItems().size();
+    QList<QPair<QString, QString> > multiData;
+    multiData << qMakePair(QStringLiteral("five"), QStringLiteral("5")) << qMakePair(QStringLiteral("six"), QStringLiteral("6")) << qMakePair(QStringLiteral("seven"), QStringLiteral("7"));
+    testModel.insertItems(1, multiData);
+    QCOMPARE(countSpy.count(), 1);
+    QCOMPARE(addedSpy.count(), 3);
+    QCOMPARE(container->childItems().size(), childItemsSize + 3);
+    QCOMPARE(repeater->itemAt(2), container->childItems().at(2));
+    addedSpy.clear();
+    countSpy.clear();
+
     delete testObject;
     addedSpy.clear();
     countSpy.clear();
@@ -676,7 +689,8 @@ void tst_QQuickRepeater::asynchronous()
     }
 
     // items will be created one at a time
-    for (int i = 0; i < 10; ++i) {
+    // the order is incubator/model specific
+    for (int i = 9; i >= 0; --i) {
         QString name("delegate");
         name += QString::number(i);
         QVERIFY(findItem<QQuickItem>(container, name) == 0);
@@ -845,6 +859,41 @@ void tst_QQuickRepeater::clearRemovalOrder()
     delete rootObject;
 }
 
+void tst_QQuickRepeater::destroyCount()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("destroycount.qml"));
+
+    QQuickItem *rootObject = qobject_cast<QQuickItem*>(component.create());
+    QVERIFY(rootObject);
+
+    QQuickRepeater *repeater = findItem<QQuickRepeater>(rootObject, "repeater");
+    QVERIFY(repeater);
+
+    repeater->setProperty("model", qVariantFromValue<int>(3));
+    QCOMPARE(repeater->property("componentCount").toInt(), 3);
+    repeater->setProperty("model", qVariantFromValue<int>(0));
+    QCOMPARE(repeater->property("componentCount").toInt(), 0);
+    repeater->setProperty("model", qVariantFromValue<int>(4));
+    QCOMPARE(repeater->property("componentCount").toInt(), 4);
+
+    QStringListModel model;
+    repeater->setProperty("model", qVariantFromValue<QStringListModel *>(&model));
+    QCOMPARE(repeater->property("componentCount").toInt(), 0);
+    QStringList list;
+    list << "1" << "2" << "3" << "4";
+    model.setStringList(list);
+    QCOMPARE(repeater->property("componentCount").toInt(), 4);
+    model.insertRows(2,1);
+    QModelIndex index = model.index(2);
+    model.setData(index, qVariantFromValue<QString>(QStringLiteral("foobar")));
+    QCOMPARE(repeater->property("componentCount").toInt(), 5);
+
+    model.removeRows(2,1);
+    QCOMPARE(model.rowCount(), 4);
+    QCOMPARE(repeater->property("componentCount").toInt(), 4);
+}
+
 QTEST_MAIN(tst_QQuickRepeater)
 
 #include "tst_qquickrepeater.moc"