summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJan Arve Sæther <jan-arve.saether@qt.io>2021-03-09 10:13:57 +0100
committerQt Cherry-pick Bot <cherrypick_bot@qt-project.org>2021-03-29 17:42:22 +0000
commit8c0e1b73ae651f3a9f06b151d8f6372818b974f9 (patch)
tree899c0014461c2966ab9240fd90ec99adff183714
parentfa54c390626dd684ba3478769923fe4b73aba09e (diff)
DelegateModelGroup: Fix bug where item could be removed from the model
If an item was removed from the DelegateModelGroup before it was completed it caused subsequent items in the model to be missing in some cases. The reason was that while populating the ListView, it iterated with an index for each item to call createItem() on. However, createItem() might call onCompleted (which in the case of QTBUG-86708 removed the item from the DelegateModel), which caused the next index we called createItem() with to be wrong (it became one step ahead). We therefore add a helper class MutableModelIterator, which keeps track of if a index in the model got removed (and if the iterator index needs to be adjusted because of that).... Task-number: QTBUG-86708 Change-Id: I33537b43727aed4f2b9bdda794b011b6684c44b4 Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io> (cherry picked from commit 0ff9db566c48172c688bf9327fe6a781dc4a1c34) Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
-rw-r--r--src/quick/items/qquicklistview.cpp102
-rw-r--r--tests/auto/qml/qqmldelegatemodel/CMakeLists.txt1
-rw-r--r--tests/auto/qml/qqmldelegatemodel/data/removeFromGroup.qml45
-rw-r--r--tests/auto/qml/qqmldelegatemodel/qqmldelegatemodel.pro13
-rw-r--r--tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp15
5 files changed, 167 insertions, 9 deletions
diff --git a/src/quick/items/qquicklistview.cpp b/src/quick/items/qquicklistview.cpp
index 0c924c0e0e..b954f1b107 100644
--- a/src/quick/items/qquicklistview.cpp
+++ b/src/quick/items/qquicklistview.cpp
@@ -382,6 +382,83 @@ private:
}
};
+/*! \internal
+ \brief A helper class for iterating over a model that might change
+
+ When populating the ListView from a model under normal
+ circumstances, we would iterate over the range of model indices
+ correspondning to the visual range, and basically call
+ createItem(index++) in order to create each item.
+
+ This will also emit Component.onCompleted() for each item, which
+ might do some weird things... For instance, it might remove itself
+ from the model, and this might change model count and the indices
+ of the other subsequent entries in the model.
+
+ This class takes such changes to the model into consideration while
+ iterating, and will adjust the iterator index and keep track of
+ whether the iterator has reached the end of the range.
+
+ It keeps track of changes to the model by connecting to
+ QQmlInstanceModel::modelUpdated() from its constructor.
+ When destroyed, it will automatically disconnect. You can
+ explicitly disconnect earlier by calling \fn disconnect().
+*/
+class MutableModelIterator {
+public:
+ MutableModelIterator(QQmlInstanceModel *model, int iBegin, int iEnd)
+ : removedAtIndex(false)
+ , backwards(iEnd < iBegin)
+ {
+ conn = QObject::connect(model, &QQmlInstanceModel::modelUpdated,
+ [&] (const QQmlChangeSet &changeSet, bool /*reset*/)
+ {
+ for (const QQmlChangeSet::Change &rem : changeSet.removes()) {
+ idxEnd -= rem.count;
+ if (rem.start() <= index) {
+ index -= rem.count;
+ if (index < rem.start() + rem.count)
+ removedAtIndex = true; // model index was removed
+ }
+ }
+ for (const QQmlChangeSet::Change &ins : changeSet.inserts()) {
+ idxEnd += ins.count;
+ if (ins.start() <= index)
+ index += ins.count;
+ }
+ }
+ );
+ index = iBegin;
+ idxEnd = iEnd;
+ }
+
+ bool hasNext() const {
+ return backwards ? index > idxEnd : index < idxEnd;
+ }
+
+ void next() { index += (backwards ? -1 : +1); }
+
+ ~MutableModelIterator()
+ {
+ disconnect();
+ }
+
+ void disconnect()
+ {
+ if (conn) {
+ QObject::disconnect(conn);
+ conn = QMetaObject::Connection(); // set to nullptr
+ }
+ }
+ int index = 0;
+ int idxEnd;
+ unsigned removedAtIndex : 1;
+ unsigned backwards : 1;
+private:
+ QMetaObject::Connection conn;
+};
+
+
//----------------------------------------------------------------------------
bool QQuickListViewPrivate::isContentFlowReversed() const
@@ -3602,7 +3679,6 @@ bool QQuickListViewPrivate::applyInsertionChange(const QQmlChangeSet::Change &ch
if (insertResult->visiblePos.isValid() && pos < insertResult->visiblePos) {
// Insert items before the visible item.
int insertionIdx = index;
- int i = 0;
qreal from = tempPos - displayMarginBeginning - buffer;
if (insertionIdx < visibleIndex) {
@@ -3611,15 +3687,18 @@ bool QQuickListViewPrivate::applyInsertionChange(const QQmlChangeSet::Change &ch
insertResult->sizeChangesBeforeVisiblePos += count * (averageSize + spacing);
}
} else {
- for (i = count-1; i >= 0 && pos >= from; --i) {
+ MutableModelIterator it(model, modelIndex + count - 1, modelIndex -1);
+ for (; it.hasNext() && pos >= from; it.next()) {
// item is before first visible e.g. in cache buffer
FxViewItem *item = nullptr;
- if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i))))
- item->index = modelIndex + i;
+ if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(it.index))))
+ item->index = it.index;
if (!item)
- item = createItem(modelIndex + i, QQmlIncubator::Synchronous);
+ item = createItem(it.index, QQmlIncubator::Synchronous);
if (!item)
return false;
+ if (it.removedAtIndex)
+ continue;
visibleAffected = true;
visibleItems.insert(insertionIdx, item);
@@ -3652,16 +3731,20 @@ bool QQuickListViewPrivate::applyInsertionChange(const QQmlChangeSet::Change &ch
}
} else {
- for (int i = 0; i < count && pos <= lastVisiblePos; ++i) {
+ MutableModelIterator it(model, modelIndex, modelIndex + count);
+ for (; it.hasNext() && pos <= lastVisiblePos; it.next()) {
visibleAffected = true;
FxViewItem *item = nullptr;
- if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i))))
- item->index = modelIndex + i;
+ if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(it.index))))
+ item->index = it.index;
bool newItem = !item;
+ it.removedAtIndex = false;
if (!item)
- item = createItem(modelIndex + i, QQmlIncubator::Synchronous);
+ item = createItem(it.index, QQmlIncubator::Synchronous);
if (!item)
return false;
+ if (it.removedAtIndex)
+ continue;
visibleItems.insert(index, item);
if (index == 0)
@@ -3682,6 +3765,7 @@ bool QQuickListViewPrivate::applyInsertionChange(const QQmlChangeSet::Change &ch
pos += item->size() + spacing;
++index;
}
+ it.disconnect();
if (0 < index && index < visibleItems.count()) {
FxViewItem *prevItem = visibleItems.at(index - 1);
diff --git a/tests/auto/qml/qqmldelegatemodel/CMakeLists.txt b/tests/auto/qml/qqmldelegatemodel/CMakeLists.txt
index ef31fa3db3..9020d9e892 100644
--- a/tests/auto/qml/qqmldelegatemodel/CMakeLists.txt
+++ b/tests/auto/qml/qqmldelegatemodel/CMakeLists.txt
@@ -22,6 +22,7 @@ qt_internal_add_test(tst_qqmldelegatemodel
Qt::Qml
Qt::QmlModelsPrivate
Qt::QmlPrivate
+ Qt::Quick
TESTDATA ${test_data}
)
diff --git a/tests/auto/qml/qqmldelegatemodel/data/removeFromGroup.qml b/tests/auto/qml/qqmldelegatemodel/data/removeFromGroup.qml
new file mode 100644
index 0000000000..4ae1a8aacc
--- /dev/null
+++ b/tests/auto/qml/qqmldelegatemodel/data/removeFromGroup.qml
@@ -0,0 +1,45 @@
+import QtQuick 2.8
+import QtQml.Models 2.1
+
+Item {
+ id: root
+ width: 200
+ height: 200
+
+ DelegateModel {
+ id: visualModel
+ model: ListModel {
+ id: myLM
+ ListElement {
+ name: "Apple"
+ }
+ ListElement {
+ name: "Banana"
+ }
+ ListElement {
+ name: "Orange"
+ }
+ }
+ filterOnGroup: "selected"
+ groups: [
+ DelegateModelGroup {
+ name: "selected"
+ includeByDefault: true
+ }
+ ]
+ delegate: Text {
+ Component.onCompleted: {
+ if (index === 1) {
+ DelegateModel.inSelected = false
+ }
+ }
+ text: index + ": " + model.name
+ }
+ }
+
+ // Needs an actual ListView in order for the DelegateModel to instantiate all items
+ ListView {
+ model: visualModel
+ anchors.fill: parent
+ }
+}
diff --git a/tests/auto/qml/qqmldelegatemodel/qqmldelegatemodel.pro b/tests/auto/qml/qqmldelegatemodel/qqmldelegatemodel.pro
new file mode 100644
index 0000000000..fbd72f6a44
--- /dev/null
+++ b/tests/auto/qml/qqmldelegatemodel/qqmldelegatemodel.pro
@@ -0,0 +1,13 @@
+CONFIG += testcase
+TARGET = tst_qqmldelegatemodel
+macos:CONFIG -= app_bundle
+
+QT += qml quick testlib core-private qml-private qmlmodels-private
+
+SOURCES += tst_qqmldelegatemodel.cpp
+
+include (../../shared/util.pri)
+
+TESTDATA = data/*
+
+OTHER_FILES += data/*
diff --git a/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp b/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp
index 8e6d147028..1a772b8a1e 100644
--- a/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp
+++ b/tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp
@@ -29,6 +29,8 @@
#include <QtTest/qtest.h>
#include <QtQml/qqmlcomponent.h>
#include <QtQmlModels/private/qqmldelegatemodel_p.h>
+#include <QtQuick/qquickview.h>
+#include <QtQuick/qquickitem.h>
#include "../../shared/util.h"
@@ -43,6 +45,7 @@ private slots:
void valueWithoutCallingObjectFirst_data();
void valueWithoutCallingObjectFirst();
void qtbug_86017();
+ void filterOnGroup_removeWhenCompleted();
};
class AbstractItemModel : public QAbstractItemModel
@@ -150,6 +153,18 @@ void tst_QQmlDelegateModel::qtbug_86017()
QCOMPARE(model->filterGroup(), "selected");
}
+void tst_QQmlDelegateModel::filterOnGroup_removeWhenCompleted()
+{
+ QQuickView view(testFileUrl("removeFromGroup.qml"));
+ QCOMPARE(view.status(), QQuickView::Ready);
+ view.show();
+ QQuickItem *root = view.rootObject();
+ QVERIFY(root);
+ QQmlDelegateModel *model = root->findChild<QQmlDelegateModel*>();
+ QVERIFY(model);
+ QTest::qWaitFor([=]{ return model->count() == 2; } );
+}
+
QTEST_MAIN(tst_QQmlDelegateModel)
#include "tst_qqmldelegatemodel.moc"