set() and setProperty() should not always trigger change signals

Fix set() and setProperty() to only trigger itemsChanged(), and the
the <property>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

5 files changed, 83 insertions, 42 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<int> roles;
+    set(index, valuemap, &roles);
+    if (!roles.isEmpty() && !inWorkerThread())
+        emit itemsChanged(index, 1, roles);
+}
+
+void QDeclarativeListModel::set(int index, const QScriptValue& valuemap, QList<int> *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<int> 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<int> 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<int> *roles)
+{
     if (count() == 0 || index >= count() || index < 0) {
         qmlInfo(this) << tr("set: index %1 out of range").arg(index);
         return;
     }
 
-    QList<int> 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<int, QVariant> *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<int, QVariant> &row = m_model->m_values[index];
         row[role] = value.toVariant();
 
+        QList<int> 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<int> 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<int> *r
     Q_ASSERT(index >=0 && index < count());
 
     ModelNode *node = qvariant_cast<ModelNode *>(_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<ModelNode *>(_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<QString, ModelNode *>::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<int> *roles);
+    void setProperty(int index, const QString& property, const QVariant& value, QList<int> *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<int> *roles);
-    void setProperty(int index, const QString& property, const QVariant& value, QList<int> *role);
+    void setProperty(int index, const QString& property, const QVariant& value, QList<int> *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<int>() };
     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<int>() };
     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<int>() };
     changes << c;
 }
 
-void QDeclarativeListModelWorkerAgent::Data::changedChange(int index, int count)
+void QDeclarativeListModelWorkerAgent::Data::changedChange(int index, int count, const QList<int> &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<int> 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<int> 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<int> &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<int> 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<int> &roles);
     };
     Data data;
 
@@ -143,7 +144,7 @@ private:
         QDeclarativeListModel *list;
     };
 
-    void changedData(int index, int count);
+    void changedData(int index, int count, const QList<int> &roles);
 
     QAtomicInt m_ref;
     QDeclarativeListModel *m_orig;