diff --git a/changelog/next.md b/changelog/next.md new file mode 100644 index 0000000..d02bf86 --- /dev/null +++ b/changelog/next.md @@ -0,0 +1,3 @@ +## Bug Fixes + +- Fixed ScriptModel not sending role updates on changed data that compares equal by key or structure. diff --git a/src/core/scriptmodel.cpp b/src/core/scriptmodel.cpp index 94fb788..f71a805 100644 --- a/src/core/scriptmodel.cpp +++ b/src/core/scriptmodel.cpp @@ -199,31 +199,34 @@ bool ScriptModel::updateValuesUnique(const QList& newValues) { this->endInsertRows(); anyChanges = true; } - } else if (!newIter->strictlyEquals(*iter)) { - auto first = static_cast(std::distance(this->mValues.begin(), iter)); - auto index = first; - - do { - this->mValues.replace(index, *newIter); - ++iter; - ++newIter; - ++index; - } while (iter != this->mValues.end() && newIter != newValues.end() - && !newIter->strictlyEquals(*iter)); - - this->dataChanged( - this->index(first, 0, QModelIndex()), - this->index(index - 1, 0, QModelIndex()), - {Qt::UserRole} - ); - - anyChanges = true; } else { ++iter; ++newIter; } } + for (auto i = 0; i != this->mValues.length();) { + if (this->mValues.at(i).strictlyEquals(newValues.at(i))) { + ++i; + continue; + } + + auto first = i; + + do { + this->mValues.replace(i, newValues.at(i)); + ++i; + } while (i != this->mValues.length() && !this->mValues.at(i).strictlyEquals(newValues.at(i))); + + this->dataChanged( + this->index(first, 0, QModelIndex()), + this->index(i - 1, 0, QModelIndex()), + {Qt::UserRole} + ); + + anyChanges = true; + } + this->isModifying = false; if (this->stagedValues.has_value()) { diff --git a/src/core/test/scriptmodel.cpp b/src/core/test/scriptmodel.cpp index 7b23610..809cc99 100644 --- a/src/core/test/scriptmodel.cpp +++ b/src/core/test/scriptmodel.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -47,6 +48,7 @@ ChangeObserver::ChangeObserver(QAbstractItemModel* model) { QObject::connect(model, &QAbstractItemModel::rowsInserted, this, &ChangeObserver::onInsert); QObject::connect(model, &QAbstractItemModel::rowsRemoved, this, &ChangeObserver::onRemove); QObject::connect(model, &QAbstractItemModel::rowsMoved, this, &ChangeObserver::onMove); + QObject::connect(model, &QAbstractItemModel::dataChanged, this, &ChangeObserver::onDataChanged); } void ChangeObserver::onInsert(const QModelIndex& parent, int first, int last) { @@ -72,6 +74,10 @@ void ChangeObserver::onMove( .emplaceBack(ModelOperation::Move, sourceStart, sourceEnd - sourceStart + 1, destStart); } +void ChangeObserver::onDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight) { + this->mChanges.emplaceBack(topLeft.row(), bottomRight.row()); +} + // NOLINTNEXTLINE(misc-use-internal-linkage) QDebug& operator<<(QDebug& debug, const QJSValueList& list) { QString str; @@ -186,6 +192,7 @@ void TestScriptModel::unique() { model.setValues(newList); QVERIFY(qjsValueListsEqual(model.values(), newList)); QCOMPARE_EQ(observer.operations(), operations); + QCOMPARE_EQ(observer.changes(), ChangeList()); } void TestScriptModel::structuralEquality() { @@ -280,4 +287,100 @@ void TestScriptModel::comparisonModes() { QCOMPARE_EQ(observer.operations(), OpList()); } +void TestScriptModel::dataUpdated_data() { + QTest::addColumn("oldstr"); + QTest::addColumn("newstr"); + QTest::addColumn("operations"); + QTest::addColumn("changes"); + + // Uppercase = same data, Lowercase = data updated + QTest::addRow("insert") << "ABC" << "AXBC" << OpList({{ModelOperation::Insert, 1, 1}}) + << ChangeList(); + QTest::addRow("remove") << "AXBC" << "ABC" << OpList({{ModelOperation::Remove, 1, 1}}) + << ChangeList(); + QTest::addRow("move") << "ABCD" << "ACBD" << OpList({{ModelOperation::Move, 2, 1, 1}}) + << ChangeList(); + QTest::addRow("move_range") << "ABCDEF" << "ADEBCF" << OpList({{ModelOperation::Move, 3, 2, 1}}) + << ChangeList(); + QTest::addRow("state_ok") << "ABCDEFG" << "ABEFXYCDG" + << OpList({ + {ModelOperation::Move, 4, 2, 2}, // ABEFCDG + {ModelOperation::Insert, 4, 2}, // ABEFXYCDG + }) + << ChangeList(); + + QTest::addRow("insert_all_updated") << "ABC" << "aXbc" << OpList({{ModelOperation::Insert, 1, 1}}) + << ChangeList({{0, 0}, {2, 3}}); + QTest::addRow("remove_all_updated") + << "AXBC" << "abc" << OpList({{ModelOperation::Remove, 1, 1}}) << ChangeList({{0, 2}}); + QTest::addRow("move_all_updated") + << "ABCD" << "acbd" << OpList({{ModelOperation::Move, 2, 1, 1}}) << ChangeList({{0, 3}}); + QTest::addRow("move_range_all_updated") + << "ABCDEF" << "adebcf" << OpList({{ModelOperation::Move, 3, 2, 1}}) << ChangeList({{0, 5}}); + QTest::addRow("state_ok_all_updated") << "ABCDEFG" << "abefXYcdg" + << OpList({ + {ModelOperation::Move, 4, 2, 2}, // abefCDG + {ModelOperation::Insert, 4, 2}, // abefXYCDG + }) + << ChangeList({{0, 3}, {6, 8}}); + + QTest::addRow("update_in_place") << "ABC" << "aBc" << OpList() << ChangeList({{0, 0}, {2, 2}}); + QTest::addRow("move_updated") << "ABCDEF" << "AdeBCF" << OpList({{ModelOperation::Move, 3, 2, 1}}) + << ChangeList({{1, 2}}); + QTest::addRow("move_neighbours_updated") + << "ABCD" << "aCbd" << OpList({{ModelOperation::Move, 2, 1, 1}}) + << ChangeList({{0, 0}, {2, 3}}); + QTest::addRow("move_partially_updated") + << "ABCDEF" << "aCdEbf" << OpList({{ModelOperation::Move, 2, 3, 1}}) + << ChangeList({{0, 0}, {2, 2}, {4, 5}}); +} + +void TestScriptModel::dataUpdated() { + QFETCH(const QString, oldstr); + QFETCH(const QString, newstr); + QFETCH(const OpList, operations); + QFETCH(const ChangeList, changes); + + QJSEngine engine; + auto version = 0; + QHash oldValues; + + auto strToJsValueList = [&](const QString& str) -> QJSValueList { + QJSValueList list; + + for (auto c: str) { + auto key = QString(c.toUpper()); + + if (c.isUpper() && oldValues.contains(key)) { + list.emplace_back(oldValues.value(key)); + continue; + } + + auto value = engine.newObject(); + value.setProperty("key", key); + value.setProperty("version", version); + list.emplace_back(value); + } + + version++; + return list; + }; + + auto oldList = strToJsValueList(oldstr); + for (const auto& value: oldList) oldValues.insert(value.property("key").toString(), value); + auto newList = strToJsValueList(newstr); + + ScriptModel model; + model.setObjectProp("key"); + auto modelTester = QAbstractItemModelTester(&model); + + model.setValues(oldList); + auto observer = ChangeObserver(&model); + + model.setValues(newList); + QVERIFY(qjsValueListsEqual(model.values(), newList)); + QCOMPARE_EQ(observer.operations(), operations); + QCOMPARE_EQ(observer.changes(), changes); +} + QTEST_MAIN(TestScriptModel); diff --git a/src/core/test/scriptmodel.hpp b/src/core/test/scriptmodel.hpp index 27f4a24..23eb1f3 100644 --- a/src/core/test/scriptmodel.hpp +++ b/src/core/test/scriptmodel.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -29,13 +30,19 @@ struct ModelOperation { }; using OpList = QList; +// First and last row of each dataChanged, in emission order. +using ChangeList = QList>; class ChangeObserver: public QObject { public: explicit ChangeObserver(QAbstractItemModel* model); [[nodiscard]] const OpList& operations() const { return this->mOperations; } - void clear() { this->mOperations.clear(); } + [[nodiscard]] const ChangeList& changes() const { return this->mChanges; } + void clear() { + this->mOperations.clear(); + this->mChanges.clear(); + } private: void onInsert(const QModelIndex& parent, int first, int last); @@ -47,8 +54,10 @@ private: const QModelIndex& destParent, int destStart ); + void onDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight); OpList mOperations; + ChangeList mChanges; }; QDebug& operator<<(QDebug& debug, const ModelOperation& op); @@ -61,4 +70,6 @@ private slots: static void unique(); static void structuralEquality(); static void comparisonModes(); + static void dataUpdated_data(); // NOLINT + static void dataUpdated(); };