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

@@ -0,0 +1,3 @@
+## Bug Fixes
+
+- Fixed ScriptModel not sending role updates on changed data that compares equal by key or structure.

src/core/scriptmodel.cpp

@@ -199,31 +199,34 @@ bool ScriptModel::updateValuesUnique(const QList<QJSValue>& newValues) {
 				this->endInsertRows();
 				anyChanges = true;
 			}
-		} else if (!newIter->strictlyEquals(*iter)) {
-			auto first = static_cast<qint32>(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()) {

src/core/test/scriptmodel.cpp

@@ -4,6 +4,7 @@
 #include <qabstractitemmodeltester.h>
 #include <qcontainerfwd.h>
 #include <qdebug.h>
+#include <qhash.h>
 #include <qjsengine.h>
 #include <qjsvalue.h>
 #include <qlist.h>
@@ -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<QString>("oldstr");
+	QTest::addColumn<QString>("newstr");
+	QTest::addColumn<OpList>("operations");
+	QTest::addColumn<ChangeList>("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<QString, QJSValue> 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);

src/core/test/scriptmodel.hpp

@@ -4,6 +4,7 @@
 #include <qdebug.h>
 #include <qlist.h>
 #include <qobject.h>
+#include <qpair.h>
 #include <qtmetamacros.h>
 #include <qtypes.h>
 
@@ -29,13 +30,19 @@ struct ModelOperation {
 };
 
 using OpList = QList<ModelOperation>;
+// First and last row of each dataChanged, in emission order.
+using ChangeList = QList<QPair<qint32, qint32>>;
 
 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();
 };