From aca9a8d95f1fa9c29a7650d528616a0962732db3 Mon Sep 17 00:00:00 2001 From: Bea Lam Date: Mon, 10 Jan 2011 14:08:44 +1000 Subject: set() and setProperty() should not always trigger change signals Fix set() and setProperty() to only trigger itemsChanged(), and the the Changed signals for items returned by get(), if the new value is different from the old one. The exception to this are list values, as it is inefficient to check the sublists and also the model classes are due to be revised. This also fixes models used with a WorkerScript to emit itemsChanged() with the correct roles. Task-number: QTBUG-14620 Reviewed-by: Michael Brasser --- src/declarative/util/qdeclarativelistmodel.cpp | 83 +++++++---- src/declarative/util/qdeclarativelistmodel_p.h | 3 + src/declarative/util/qdeclarativelistmodel_p_p.h | 6 +- .../util/qdeclarativelistmodelworkeragent.cpp | 28 ++-- .../util/qdeclarativelistmodelworkeragent_p.h | 5 +- .../tst_qdeclarativelistmodel.cpp | 159 ++++++++++++++++++++- 6 files changed, 238 insertions(+), 46 deletions(-) diff --git a/src/declarative/util/qdeclarativelistmodel.cpp b/src/declarative/util/qdeclarativelistmodel.cpp index 538e8af..8290665 100644 --- a/src/declarative/util/qdeclarativelistmodel.cpp +++ b/src/declarative/util/qdeclarativelistmodel.cpp @@ -559,6 +559,14 @@ QScriptValue QDeclarativeListModel::get(int index) const */ void QDeclarativeListModel::set(int index, const QScriptValue& valuemap) { + QList roles; + set(index, valuemap, &roles); + if (!roles.isEmpty() && !inWorkerThread()) + emit itemsChanged(index, 1, roles); +} + +void QDeclarativeListModel::set(int index, const QScriptValue& valuemap, QList *roles) +{ if (!valuemap.isObject() || valuemap.isArray()) { qmlInfo(this) << tr("set: value is not an object"); return; @@ -571,14 +579,10 @@ void QDeclarativeListModel::set(int index, const QScriptValue& valuemap) if (index == count()) { append(valuemap); } else { - QList roles; if (m_flat) - m_flat->set(index, valuemap, &roles); + m_flat->set(index, valuemap, roles); else - m_nested->set(index, valuemap, &roles); - - if (!inWorkerThread()) - emit itemsChanged(index, 1, roles); + m_nested->set(index, valuemap, roles); } } @@ -597,19 +601,23 @@ void QDeclarativeListModel::set(int index, const QScriptValue& valuemap) */ void QDeclarativeListModel::setProperty(int index, const QString& property, const QVariant& value) { + QList roles; + setProperty(index, property, value, &roles); + if (!roles.isEmpty() && !inWorkerThread()) + emit itemsChanged(index, 1, roles); +} + +void QDeclarativeListModel::setProperty(int index, const QString& property, const QVariant& value, QList *roles) +{ if (count() == 0 || index >= count() || index < 0) { qmlInfo(this) << tr("set: index %1 out of range").arg(index); return; } - QList roles; if (m_flat) - m_flat->setProperty(index, property, value, &roles); + m_flat->setProperty(index, property, value, roles); else - m_nested->setProperty(index, property, value, &roles); - - if (!inWorkerThread()) - emit itemsChanged(index, 1, roles); + m_nested->setProperty(index, property, value, roles); } /*! @@ -1011,9 +1019,11 @@ void FlatListModel::setProperty(int index, const QString& property, const QVaria } else { role = iter.value(); } - roles->append(role); - m_values[index][role] = value; + if (m_values[index][role] != value) { + roles->append(role); + m_values[index][role] = value; + } } void FlatListModel::move(int from, int to, int n) @@ -1043,6 +1053,10 @@ bool FlatListModel::addValue(const QScriptValue &value, QHash *ro iter = m_strings.insert(name, role); if (roles) roles->append(role); + } else { + int role = iter.value(); + if (roles && row->contains(role) && row->value(role) != v) + roles->append(role); } row->insert(*iter, v); } @@ -1151,14 +1165,14 @@ void FlatListScriptClass::setProperty(Object *obj, const Identifier &name, const QHash &row = m_model->m_values[index]; row[role] = value.toVariant(); + QList roles; + roles << role; if (m_model->m_parentAgent) { // This is the list in the worker thread, so tell the agent to // emit itemsChanged() later - m_model->m_parentAgent->changedData(index, 1); + m_model->m_parentAgent->changedData(index, 1, roles); } else { // This is the list in the main thread, so emit itemsChanged() - QList roles; - roles << role; emit m_model->m_listModel->itemsChanged(index, 1, roles); } } @@ -1350,7 +1364,9 @@ void NestedListModel::set(int index, const QScriptValue& valuemap, QList *r Q_ASSERT(index >=0 && index < count()); ModelNode *node = qvariant_cast(_root->values.at(index)); - node->setObjectValue(valuemap); + bool emitItemsChanged = node->setObjectValue(valuemap); + if (!emitItemsChanged) + return; QScriptValueIterator it(valuemap); while (it.hasNext()) { @@ -1369,7 +1385,9 @@ void NestedListModel::setProperty(int index, const QString& property, const QVar Q_ASSERT(index >=0 && index < count()); ModelNode *node = qvariant_cast(_root->values.at(index)); - node->setProperty(property, value); + bool emitItemsChanged = node->setProperty(property, value); + if (!emitItemsChanged) + return; int r = roleStrings.indexOf(property); if (r < 0) { @@ -1441,26 +1459,37 @@ void ModelNode::clear() properties.clear(); } -void ModelNode::setObjectValue(const QScriptValue& valuemap, bool writeToCache) { +bool ModelNode::setObjectValue(const QScriptValue& valuemap, bool writeToCache) +{ + bool emitItemsChanged = false; + QScriptValueIterator it(valuemap); while (it.hasNext()) { it.next(); + ModelNode *prev = properties.value(it.name()); ModelNode *value = new ModelNode(m_model); QScriptValue v = it.value(); + if (v.isArray()) { value->isArray = true; value->setListValue(v); if (writeToCache && objectCache) objectCache->setValue(it.name().toUtf8(), QVariant::fromValue(value->model(m_model))); + emitItemsChanged = true; // for now, too inefficient to check whether list and sublists have changed } else { value->values << v.toVariant(); if (writeToCache && objectCache) objectCache->setValue(it.name().toUtf8(), value->values.last()); + if (!emitItemsChanged && prev && prev->values.count() == 1 + && prev->values[0] != value->values.last()) { + emitItemsChanged = true; + } } if (properties.contains(it.name())) delete properties[it.name()]; properties.insert(it.name(), value); } + return emitItemsChanged; } void ModelNode::setListValue(const QScriptValue& valuelist) { @@ -1483,9 +1512,12 @@ void ModelNode::setListValue(const QScriptValue& valuelist) { } } -void ModelNode::setProperty(const QString& prop, const QVariant& val) { +bool ModelNode::setProperty(const QString& prop, const QVariant& val) { QHash::const_iterator it = properties.find(prop); + bool emitItemsChanged = false; if (it != properties.end()) { + if (val != (*it)->values[0]) + emitItemsChanged = true; (*it)->values[0] = val; } else { ModelNode *n = new ModelNode(m_model); @@ -1494,6 +1526,7 @@ void ModelNode::setProperty(const QString& prop, const QVariant& val) { } if (objectCache) objectCache->setValue(prop.toUtf8(), val); + return emitItemsChanged; } void ModelNode::updateListIndexes() @@ -1561,7 +1594,7 @@ ModelObject::ModelObject(ModelNode *node, NestedListModel *model, QScriptEngine void ModelObject::setValue(const QByteArray &name, const QVariant &val) { m_meta->setValue(name, val); - setProperty(name.constData(), val); + //setProperty(name.constData(), val); } void ModelObject::setNodeUpdatesEnabled(bool enable) @@ -1588,9 +1621,9 @@ void ModelNodeMetaObject::propertyWritten(int index) QScriptValue sv = m_seng->newObject(); sv.setProperty(propName, m_seng->newVariant(value)); - m_obj->m_node->setObjectValue(sv, false); - - m_obj->m_node->changedProperty(propName); + bool changed = m_obj->m_node->setObjectValue(sv, false); + if (changed) + m_obj->m_node->changedProperty(propName); } diff --git a/src/declarative/util/qdeclarativelistmodel_p.h b/src/declarative/util/qdeclarativelistmodel_p.h index 90036f9..0b4becd 100644 --- a/src/declarative/util/qdeclarativelistmodel_p.h +++ b/src/declarative/util/qdeclarativelistmodel_p.h @@ -103,6 +103,9 @@ private: // Constructs a flat list model for a worker agent QDeclarativeListModel(const QDeclarativeListModel *orig, QDeclarativeListModelWorkerAgent *parent); + void set(int index, const QScriptValue&, QList *roles); + void setProperty(int index, const QString& property, const QVariant& value, QList *roles); + bool flatten(); bool inWorkerThread() const; diff --git a/src/declarative/util/qdeclarativelistmodel_p_p.h b/src/declarative/util/qdeclarativelistmodel_p_p.h index 43a0a9b..ec1b538 100644 --- a/src/declarative/util/qdeclarativelistmodel_p_p.h +++ b/src/declarative/util/qdeclarativelistmodel_p_p.h @@ -190,7 +190,7 @@ public: bool insert(int index, const QScriptValue&); QScriptValue get(int index) const; void set(int index, const QScriptValue&, QList *roles); - void setProperty(int index, const QString& property, const QVariant& value, QList *role); + void setProperty(int index, const QString& property, const QVariant& value, QList *roles); void move(int from, int to, int count); QVariant valueForNode(ModelNode *, bool *hasNested = 0) const; @@ -255,9 +255,9 @@ struct ModelNode QDeclarativeListModel *model(const NestedListModel *model); ModelObject *object(const NestedListModel *model); - void setObjectValue(const QScriptValue& valuemap, bool writeToCache = true); + bool setObjectValue(const QScriptValue& valuemap, bool writeToCache = true); void setListValue(const QScriptValue& valuelist); - void setProperty(const QString& prop, const QVariant& val); + bool setProperty(const QString& prop, const QVariant& val); void changedProperty(const QString &name) const; void updateListIndexes(); static void dump(ModelNode *node, int ind); diff --git a/src/declarative/util/qdeclarativelistmodelworkeragent.cpp b/src/declarative/util/qdeclarativelistmodelworkeragent.cpp index 852b055..76d3048 100644 --- a/src/declarative/util/qdeclarativelistmodelworkeragent.cpp +++ b/src/declarative/util/qdeclarativelistmodelworkeragent.cpp @@ -60,25 +60,25 @@ void QDeclarativeListModelWorkerAgent::Data::clearChange() void QDeclarativeListModelWorkerAgent::Data::insertChange(int index, int count) { - Change c = { Change::Inserted, index, count, 0 }; + Change c = { Change::Inserted, index, count, 0, QList() }; changes << c; } void QDeclarativeListModelWorkerAgent::Data::removeChange(int index, int count) { - Change c = { Change::Removed, index, count, 0 }; + Change c = { Change::Removed, index, count, 0, QList() }; changes << c; } void QDeclarativeListModelWorkerAgent::Data::moveChange(int index, int count, int to) { - Change c = { Change::Moved, index, count, to }; + Change c = { Change::Moved, index, count, to, QList() }; changes << c; } -void QDeclarativeListModelWorkerAgent::Data::changedChange(int index, int count) +void QDeclarativeListModelWorkerAgent::Data::changedChange(int index, int count, const QList &roles) { - Change c = { Change::Changed, index, count, 0 }; + Change c = { Change::Changed, index, count, 0, roles }; changes << c; } @@ -165,14 +165,18 @@ QScriptValue QDeclarativeListModelWorkerAgent::get(int index) const void QDeclarativeListModelWorkerAgent::set(int index, const QScriptValue &value) { - m_copy->set(index, value); - data.changedChange(index, 1); + QList roles; + m_copy->set(index, value, &roles); + if (!roles.isEmpty()) + data.changedChange(index, 1, roles); } void QDeclarativeListModelWorkerAgent::setProperty(int index, const QString& property, const QVariant& value) { - m_copy->setProperty(index, property, value); - data.changedChange(index, 1); + QList roles; + m_copy->setProperty(index, property, value, &roles); + if (!roles.isEmpty()) + data.changedChange(index, 1, roles); } void QDeclarativeListModelWorkerAgent::move(int from, int to, int count) @@ -194,9 +198,9 @@ void QDeclarativeListModelWorkerAgent::sync() mutex.unlock(); } -void QDeclarativeListModelWorkerAgent::changedData(int index, int count) +void QDeclarativeListModelWorkerAgent::changedData(int index, int count, const QList &roles) { - data.changedChange(index, count); + data.changedChange(index, count, roles); } bool QDeclarativeListModelWorkerAgent::event(QEvent *e) @@ -255,7 +259,7 @@ bool QDeclarativeListModelWorkerAgent::event(QEvent *e) emit m_orig->itemsMoved(change.index, change.to, change.count); break; case Change::Changed: - emit m_orig->itemsChanged(change.index, change.count, orig->m_roles.keys()); + emit m_orig->itemsChanged(change.index, change.count, change.roles); break; } } diff --git a/src/declarative/util/qdeclarativelistmodelworkeragent_p.h b/src/declarative/util/qdeclarativelistmodelworkeragent_p.h index 10c3bca..0823b7e 100644 --- a/src/declarative/util/qdeclarativelistmodelworkeragent_p.h +++ b/src/declarative/util/qdeclarativelistmodelworkeragent_p.h @@ -124,6 +124,7 @@ private: int index; // Inserted/Removed/Moved/Changed int count; // Inserted/Removed/Moved/Changed int to; // Moved + QList roles; }; struct Data { @@ -133,7 +134,7 @@ private: void insertChange(int index, int count); void removeChange(int index, int count); void moveChange(int index, int count, int to); - void changedChange(int index, int count); + void changedChange(int index, int count, const QList &roles); }; Data data; @@ -143,7 +144,7 @@ private: QDeclarativeListModel *list; }; - void changedData(int index, int count); + void changedData(int index, int count, const QList &roles); QAtomicInt m_ref; QDeclarativeListModel *m_orig; diff --git a/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp b/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp index 55f7421..bdc5988 100644 --- a/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp +++ b/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp @@ -59,6 +59,7 @@ #endif Q_DECLARE_METATYPE(QList) +Q_DECLARE_METATYPE(QList) class tst_qdeclarativelistmodel : public QObject { @@ -101,6 +102,10 @@ private slots: void get_nested_data(); void crash_model_with_multiple_roles(); void set_model_cache(); + void property_changes(); + void property_changes_data(); + void property_changes_worker(); + void property_changes_worker_data(); }; int tst_qdeclarativelistmodel::roleFromName(const QDeclarativeListModel *model, const QString &roleName) { @@ -298,13 +303,10 @@ void tst_qdeclarativelistmodel::dynamic_data() QTest::newRow("nested-append3") << "{append({'foo':123,'bars':[{'a':1},{'a':2},{'a':3}]});get(0).bars.append({'a':4});get(0).bars.get(3).a}" << 4 << ""; QTest::newRow("nested-insert") << "{append({'foo':123});insert(0,{'bars':[{'a':1},{'b':2},{'c':3}]});get(0).bars.get(0).a}" << 1 << ""; - QTest::newRow("nested-set") << "{append({'foo':123});set(0,{'foo':[{'x':123}]});get(0).foo.get(0).x}" << 123 << ""; + QTest::newRow("nested-set") << "{append({'foo':[{'x':1}]});set(0,{'foo':[{'x':123}]});get(0).foo.get(0).x}" << 123 << ""; QTest::newRow("nested-count") << "{append({'foo':123,'bars':[{'a':1},{'a':2},{'a':3}]}); get(0).bars.count}" << 3 << ""; QTest::newRow("nested-clear") << "{append({'foo':123,'bars':[{'a':1},{'a':2},{'a':3}]}); get(0).bars.clear(); get(0).bars.count}" << 0 << ""; - - // XXX - //QTest::newRow("nested-setprop") << "{append({'foo':123});setProperty(0,'foo',[{'x':123}]);get(0).foo.get(0).x}" << 123 << ""; } void tst_qdeclarativelistmodel::dynamic() @@ -421,6 +423,9 @@ void tst_qdeclarativelistmodel::dynamic_worker_sync() if (QByteArray(QTest::currentDataTag()).startsWith("nested")) QTest::ignoreMessage(QtWarningMsg, ": QML ListModel: Cannot add list-type data when modifying or after modification from a worker script"); + if (QByteArray(QTest::currentDataTag()).startsWith("nested-set")) + QTest::ignoreMessage(QtWarningMsg, ": QML ListModel: Cannot add list-type data when modifying or after modification from a worker script"); + QVERIFY(QMetaObject::invokeMethod(item, "evalExpressionViaWorker", Q_ARG(QVariant, operations.mid(0, operations.length()-1)))); waitForWorker(item); @@ -940,6 +945,152 @@ void tst_qdeclarativelistmodel::set_model_cache() QVERIFY(model->property("ok").toBool()); } +void tst_qdeclarativelistmodel::property_changes() +{ + QFETCH(QString, script_setup); + QFETCH(QString, script_change); + QFETCH(QString, roleName); + QFETCH(int, listIndex); + QFETCH(bool, itemsChanged); + QFETCH(QString, testExpression); + + QDeclarativeEngine engine; + QDeclarativeListModel model; + QDeclarativeEngine::setContextForObject(&model, engine.rootContext()); + engine.rootContext()->setContextObject(&model); + + QDeclarativeExpression expr(engine.rootContext(), &model, script_setup); + expr.evaluate(); + QVERIFY2(!expr.hasError(), QTest::toString(expr.error().toString())); + + QString signalHandler = "on" + QString(roleName[0].toUpper()) + roleName.mid(1, roleName.length()) + "Changed:"; + QString qml = "import QtQuick 1.0\n" + "Connections {\n" + "property bool gotSignal: false\n" + "target: model.get(0)\n" + + signalHandler + " gotSignal = true\n" + "}\n"; + QDeclarativeComponent component(&engine); + component.setData(qml.toUtf8(), QUrl::fromLocalFile("")); + engine.rootContext()->setContextProperty("model", &model); + QObject *connectionsObject = component.create(); + QVERIFY2(component.errorString().isEmpty(), QTest::toString(component.errorString())); + + QSignalSpy spyItemsChanged(&model, SIGNAL(itemsChanged(int, int, QList))); + + expr.setExpression(script_change); + expr.evaluate(); + QVERIFY2(!expr.hasError(), QTest::toString(expr.error())); + + // test the object returned by get() emits the correct signals + QCOMPARE(connectionsObject->property("gotSignal").toBool(), itemsChanged); + + // test itemsChanged() is emitted correctly + if (itemsChanged) { + QCOMPARE(spyItemsChanged.count(), 1); + QCOMPARE(spyItemsChanged.at(0).at(0).toInt(), listIndex); + QCOMPARE(spyItemsChanged.at(0).at(1).toInt(), 1); + } else { + QCOMPARE(spyItemsChanged.count(), 0); + } + + expr.setExpression(testExpression); + QCOMPARE(expr.evaluate().toBool(), true); + + delete connectionsObject; +} + +void tst_qdeclarativelistmodel::property_changes_data() +{ + QTest::addColumn("script_setup"); + QTest::addColumn("script_change"); + QTest::addColumn("roleName"); + QTest::addColumn("listIndex"); + QTest::addColumn("itemsChanged"); + QTest::addColumn("testExpression"); + + QTest::newRow("set: plain") << "append({'a':123, 'b':456, 'c':789});" << "set(0,{'b':123});" + << "b" << 0 << true << "get(0).b == 123"; + QTest::newRow("setProperty: plain") << "append({'a':123, 'b':456, 'c':789});" << "setProperty(0, 'b', 123);" + << "b" << 0 << true << "get(0).b == 123"; + + QTest::newRow("set: plain, no changes") << "append({'a':123, 'b':456, 'c':789});" << "set(0,{'b':456});" + << "b" << 0 << false << "get(0).b == 456"; + QTest::newRow("setProperty: plain, no changes") << "append({'a':123, 'b':456, 'c':789});" << "setProperty(0, 'b', 456);" + << "b" << 0 << false << "get(0).b == 456"; + + // Following tests only call set() since setProperty() only allows plain + // values, not lists, as the argument. + // Note that when a list is changed, itemsChanged() is currently always + // emitted regardless of whether it actually changed or not. + + QTest::newRow("nested-set: list, new size") << "append({'a':123, 'b':[{'a':1},{'a':2},{'a':3}], 'c':789});" << "set(0,{'b':[{'a':1},{'a':2}]});" + << "b" << 0 << true << "get(0).b.get(0).a == 1 && get(0).b.get(1).a == 2"; + + QTest::newRow("nested-set: list, empty -> non-empty") << "append({'a':123, 'b':[], 'c':789});" << "set(0,{'b':[{'a':1},{'a':2},{'a':3}]});" + << "b" << 0 << true << "get(0).b.get(0).a == 1 && get(0).b.get(1).a == 2 && get(0).b.get(2).a == 3"; + + QTest::newRow("nested-set: list, non-empty -> empty") << "append({'a':123, 'b':[{'a':1},{'a':2},{'a':3}], 'c':789});" << "set(0,{'b':[]});" + << "b" << 0 << true << "get(0).b.count == 0"; + + QTest::newRow("nested-set: list, same size, different values") << "append({'a':123, 'b':[{'a':1},{'a':2},{'a':3}], 'c':789});" << "set(0,{'b':[{'a':1},{'a':222},{'a':3}]});" + << "b" << 0 << true << "get(0).b.get(0).a == 1 && get(0).b.get(1).a == 222 && get(0).b.get(2).a == 3"; + + QTest::newRow("nested-set: list, no changes") << "append({'a':123, 'b':[{'a':1},{'a':2},{'a':3}], 'c':789});" << "set(0,{'b':[{'a':1},{'a':2},{'a':3}]});" + << "b" << 0 << true << "get(0).b.get(0).a == 1 && get(0).b.get(1).a == 2 && get(0).b.get(2).a == 3"; + + QTest::newRow("nested-set: list, no changes, empty") << "append({'a':123, 'b':[], 'c':789});" << "set(0,{'b':[]});" + << "b" << 0 << true << "get(0).b.count == 0"; +} + + +void tst_qdeclarativelistmodel::property_changes_worker() +{ + // nested models are not supported when WorkerScript is involved + if (QByteArray(QTest::currentDataTag()).startsWith("nested-")) + return; + + QFETCH(QString, script_setup); + QFETCH(QString, script_change); + QFETCH(QString, roleName); + QFETCH(int, listIndex); + QFETCH(bool, itemsChanged); + + QDeclarativeListModel model; + QDeclarativeEngine engine; + QDeclarativeComponent component(&engine, QUrl::fromLocalFile(SRCDIR "/data/model.qml")); + QVERIFY2(component.errorString().isEmpty(), component.errorString().toUtf8()); + QDeclarativeItem *item = createWorkerTest(&engine, &component, &model); + QVERIFY(item != 0); + + QDeclarativeExpression expr(engine.rootContext(), &model, script_setup); + expr.evaluate(); + QVERIFY2(!expr.hasError(), QTest::toString(expr.error().toString())); + + QSignalSpy spyItemsChanged(&model, SIGNAL(itemsChanged(int, int, QList))); + + QVERIFY(QMetaObject::invokeMethod(item, "evalExpressionViaWorker", + Q_ARG(QVariant, QStringList(script_change)))); + waitForWorker(item); + + // test itemsChanged() is emitted correctly + if (itemsChanged) { + QCOMPARE(spyItemsChanged.count(), 1); + QCOMPARE(spyItemsChanged.at(0).at(0).toInt(), listIndex); + QCOMPARE(spyItemsChanged.at(0).at(1).toInt(), 1); + } else { + QCOMPARE(spyItemsChanged.count(), 0); + } + + delete item; + qApp->processEvents(); +} + +void tst_qdeclarativelistmodel::property_changes_worker_data() +{ + property_changes_data(); +} + QTEST_MAIN(tst_qdeclarativelistmodel) #include "tst_qdeclarativelistmodel.moc" -- cgit v0.12