From d7ee6d125c8eadf9b18c93dbc9bff16e6c47563d Mon Sep 17 00:00:00 2001 From: outfoxxed Date: Fri, 16 Oct 2026 17:43:57 -0700 Subject: [PATCH] core/scriptmodel: emit row updates for rows changed by only identity Fixes updates not being emitted when using objectProp. Changes that compared structurally equal are also reported, which might cover some additional JS equality cases. --- changelog/next.md | 3 + src/core/scriptmodel.cpp | 41 +++++++------- src/core/test/scriptmodel.cpp | 103 ++++++++++++++++++++++++++++++++++ src/core/test/scriptmodel.hpp | 13 ++++- 4 files changed, 140 insertions(+), 20 deletions(-) create mode 100644 changelog/next.md 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(); };