Improvements to listmodel implementation and tests.

- Fixed edge case crash bug with QObjects being set on existing
    listmodel element.
- Improved warning messages when assigning wrong type to role.
- Removed a few code paths that can never be hit.
- Added several tests to cover functionality not hit by coverage.

Change-Id: I3d237c0555afbba6377b4d898bec911515b1b4ea
Reviewed-by: Martin Jones <martin.jones@nokia.com>

diff --git a/src/declarative/util/qdeclarativelistmodel.cpp b/src/declarative/util/qdeclarativelistmodel.cpp
index 4459440..2cc52fb 100644 (file)
--- a/src/declarative/util/qdeclarativelistmodel.cpp
+++ b/src/declarative/util/qdeclarativelistmodel.cpp
@@ -64,14 +64,24 @@ enum { MIN_LISTMODEL_UID = 1024 };
 
 QAtomicInt ListModel::uidCounter(MIN_LISTMODEL_UID);
 
+static QString roleTypeName(ListLayout::Role::DataType t)
+{
+    QString result;
+    const char *roleTypeNames[] = { "String", "Number", "Bool", "List", "QObject" };
+
+    if (t > ListLayout::Role::Invalid && t < ListLayout::Role::MaxDataType)
+        result = QString::fromLatin1(roleTypeNames[t]);
+
+    return result;
+}
+
 const ListLayout::Role &ListLayout::getRoleOrCreate(const QString &key, Role::DataType type)
 {
     QStringHash<Role *>::Node *node = roleHash.findNode(key);
     if (node) {
         const Role &r = *node->value;
-        if (type != r.type) {
-            qmlInfo(0) << "Can't assign to pre-existing role of different type " << r.name;
-        }
+        if (type != r.type)
+            qmlInfo(0) << QString::fromLatin1("Can't assign to existing role '%1' of different type [%2 -> %3]").arg(r.name).arg(roleTypeName(type)).arg(roleTypeName(r.type));
         return r;
     }
 
@@ -84,9 +94,8 @@ const ListLayout::Role &ListLayout::getRoleOrCreate(v8::Handle<v8::String> key,
     QStringHash<Role *>::Node *node = roleHash.findNode(hashedKey);
     if (node) {
         const Role &r = *node->value;
-        if (type != r.type) {
-            qmlInfo(0) << "Can't assign to pre-existing role of different type " << r.name;
-        }
+        if (type != r.type)
+            qmlInfo(0) << QString::fromLatin1("Can't assign to existing role '%1' of different type [%2 -> %3]").arg(r.name).arg(roleTypeName(type)).arg(roleTypeName(r.type));
         return r;
     }
 
@@ -99,8 +108,8 @@ const ListLayout::Role &ListLayout::getRoleOrCreate(v8::Handle<v8::String> key,
 
 const ListLayout::Role &ListLayout::createRole(const QString &key, ListLayout::Role::DataType type)
 {
-    const int dataSizes[] = { sizeof(QString), sizeof(double), sizeof(bool), sizeof(QDeclarativeListModel *), sizeof(ListModel *), sizeof(QDeclarativeGuard<QObject>) };
-    const int dataAlignments[] = { sizeof(QString), sizeof(double), sizeof(bool), sizeof(QDeclarativeListModel *), sizeof(ListModel *), sizeof(QObject *) };
+    const int dataSizes[] = { sizeof(QString), sizeof(double), sizeof(bool), sizeof(ListModel *), sizeof(QDeclarativeGuard<QObject>) };
+    const int dataAlignments[] = { sizeof(QString), sizeof(double), sizeof(bool), sizeof(ListModel *), sizeof(QObject *) };
 
     Role *r = new Role;
     r->name = key;
@@ -429,9 +438,6 @@ void ListModel::set(int elementIndex, v8::Handle<v8::Object> object, QList<int>
             }
 
             roleIndex = e->setListProperty(r, subModel);
-        } else if (propertyValue->IsInt32()) {
-            const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::Number);
-            roleIndex = e->setDoubleProperty(r, (double) propertyValue->Int32Value());
         } else if (propertyValue->IsBoolean()) {
             const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::Bool);
             roleIndex = e->setBoolProperty(r, propertyValue->BooleanValue());
@@ -498,11 +504,6 @@ void ListModel::set(int elementIndex, v8::Handle<v8::Object> object)
 
                 e->setListPropertyFast(r, subModel);
             }
-        } else if (propertyValue->IsInt32()) {
-            const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::Number);
-            if (r.type == ListLayout::Role::Number) {
-                e->setDoublePropertyFast(r, (double) propertyValue->Int32Value());
-            }
         } else if (propertyValue->IsBoolean()) {
             const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::Bool);
             if (r.type == ListLayout::Role::Bool) {
@@ -624,7 +625,20 @@ QObject *ListElement::getQObjectProperty(const ListLayout::Role &role)
 QDeclarativeGuard<QObject> *ListElement::getGuardProperty(const ListLayout::Role &role)
 {
     char *mem = getPropertyMemory(role);
-    QDeclarativeGuard<QObject> *o = reinterpret_cast<QDeclarativeGuard<QObject> *>(mem);
+
+    bool existingGuard = false;
+    for (size_t i=0 ; i < sizeof(QDeclarativeGuard<QObject>) ; ++i) {
+        if (mem[i] != 0) {
+            existingGuard = true;
+            break;
+        }
+    }
+
+    QDeclarativeGuard<QObject> *o = 0;
+
+    if (existingGuard)
+        o = reinterpret_cast<QDeclarativeGuard<QObject> *>(mem);
+
     return o;
 }
 
@@ -709,8 +723,6 @@ int ListElement::setStringProperty(const ListLayout::Role &role, const QString &
         }
         if (changed)
             roleIndex = role.index;
-    } else {
-        qmlInfo(0) << "Unable to assign string to role '" << role.name << "' of different type.";
     }
 
     return roleIndex;
@@ -727,8 +739,6 @@ int ListElement::setDoubleProperty(const ListLayout::Role &role, double d)
         *value = d;
         if (changed)
             roleIndex = role.index;
-    } else {
-        qmlInfo(0) << "Unable to assign number to role '" << role.name << "' of different type.";
     }
 
     return roleIndex;
@@ -745,8 +755,6 @@ int ListElement::setBoolProperty(const ListLayout::Role &role, bool b)
         *value = b;
         if (changed)
             roleIndex = role.index;
-    } else {
-        qmlInfo(0) << "Unable to assign bool to role '" << role.name << "' of different type.";
     }
 
     return roleIndex;
@@ -765,8 +773,6 @@ int ListElement::setListProperty(const ListLayout::Role &role, ListModel *m)
         }
         *value = m;
         roleIndex = role.index;
-    } else {
-        qmlInfo(0) << "Unable to assign list to role '" << role.name << "' of different type.";
     }
 
     return roleIndex;
@@ -779,13 +785,23 @@ int ListElement::setQObjectProperty(const ListLayout::Role &role, QObject *o)
     if (role.type == ListLayout::Role::QObject) {
         char *mem = getPropertyMemory(role);
         QDeclarativeGuard<QObject> *g = reinterpret_cast<QDeclarativeGuard<QObject> *>(mem);
-        bool changed = g->data() != o;
-        g->~QDeclarativeGuard();
+        bool existingGuard = false;
+        for (size_t i=0 ; i < sizeof(QDeclarativeGuard<QObject>) ; ++i) {
+            if (mem[i] != 0) {
+                existingGuard = true;
+                break;
+            }
+        }
+        bool changed;
+        if (existingGuard) {
+            changed = g->data() != o;
+            g->~QDeclarativeGuard();
+        } else {
+            changed = true;
+        }
         new (mem) QDeclarativeGuard<QObject>(o);
         if (changed)
             roleIndex = role.index;
-    } else {
-        qmlInfo(0) << "Unable to assign string to role '" << role.name << "' of different type.";
     }
 
     return roleIndex;
@@ -935,7 +951,8 @@ void ListElement::destroy(ListLayout *layout)
                 case ListLayout::Role::QObject:
                     {
                         QDeclarativeGuard<QObject> *guard = getGuardProperty(r);
-                        guard->~QDeclarativeGuard();
+                        if (guard)
+                            guard->~QDeclarativeGuard();
                     }
                     break;
                 default:
@@ -999,8 +1016,6 @@ int ListElement::setJsProperty(const ListLayout::Role &role, v8::Handle<v8::Valu
             subModel->append(subObject);
         }
         roleIndex = setListProperty(role, subModel);
-    } else if (d->IsInt32()) {
-        roleIndex = setDoubleProperty(role, (double) d->Int32Value());
     } else if (d->IsBoolean()) {
         roleIndex = setBoolProperty(role, d->BooleanValue());
     } else if (d->IsObject()) {

diff --git a/src/declarative/util/qdeclarativelistmodel_p_p.h b/src/declarative/util/qdeclarativelistmodel_p_p.h
index f1e47d7..24e8724 100644 (file)
--- a/src/declarative/util/qdeclarativelistmodel_p_p.h
+++ b/src/declarative/util/qdeclarativelistmodel_p_p.h
@@ -129,6 +129,7 @@ public:
         explicit Role(const Role *other);
         ~Role();
 
+        // This enum must be kept in sync with the roleTypeNames variable in qdeclarativelistmodel.cpp
         enum DataType
         {
             Invalid = -1,
@@ -137,7 +138,9 @@ public:
             Number,
             Bool,
             List,
-            QObject
+            QObject,
+
+            MaxDataType
         };
 
         QString name;

diff --git a/src/declarative/util/qdeclarativelistmodelworkeragent.cpp b/src/declarative/util/qdeclarativelistmodelworkeragent.cpp
index 2fed035..2e7ef05 100644 (file)
--- a/src/declarative/util/qdeclarativelistmodelworkeragent.cpp
+++ b/src/declarative/util/qdeclarativelistmodelworkeragent.cpp
@@ -94,10 +94,6 @@ QDeclarativeListModelWorkerAgent::QDeclarativeListModelWorkerAgent(QDeclarativeL
 {
 }
 
-QDeclarativeListModelWorkerAgent::~QDeclarativeListModelWorkerAgent()
-{
-}
-
 void QDeclarativeListModelWorkerAgent::setV8Engine(QV8Engine *eng)
 {
     m_copy->m_engine = eng;
@@ -183,43 +179,39 @@ bool QDeclarativeListModelWorkerAgent::event(QEvent *e)
 
         const QList<Change> &changes = s->data.changes;
 
-        if (m_copy) {
-            bool cc = m_orig->count() != s->list->count();
-
-            QHash<int, ListModel *> targetModelHash;
-            ListModel::sync(s->list->m_listModel, m_orig->m_listModel, &targetModelHash);
-
-            for (int ii = 0; ii < changes.count(); ++ii) {
-                const Change &change = changes.at(ii);
-
-                ListModel *model = targetModelHash.value(change.modelUid);
-
-                if (model && model->m_modelCache) {
-                    switch (change.type) {
-                    case Change::Inserted:
-                        emit model->m_modelCache->itemsInserted(change.index, change.count);
-                        break;
-                    case Change::Removed:
-                        emit model->m_modelCache->itemsRemoved(change.index, change.count);
-                        break;
-                    case Change::Moved:
-                        emit model->m_modelCache->itemsMoved(change.index, change.to, change.count);
-                        break;
-                    case Change::Changed:
-                        emit model->m_modelCache->itemsChanged(change.index, change.count, change.roles);
-                        break;
-                    }
+        bool cc = m_orig->count() != s->list->count();
+
+        QHash<int, ListModel *> targetModelHash;
+        ListModel::sync(s->list->m_listModel, m_orig->m_listModel, &targetModelHash);
+
+        for (int ii = 0; ii < changes.count(); ++ii) {
+            const Change &change = changes.at(ii);
+
+            ListModel *model = targetModelHash.value(change.modelUid);
+
+            if (model && model->m_modelCache) {
+                switch (change.type) {
+                case Change::Inserted:
+                    emit model->m_modelCache->itemsInserted(change.index, change.count);
+                    break;
+                case Change::Removed:
+                    emit model->m_modelCache->itemsRemoved(change.index, change.count);
+                    break;
+                case Change::Moved:
+                    emit model->m_modelCache->itemsMoved(change.index, change.to, change.count);
+                    break;
+                case Change::Changed:
+                    emit model->m_modelCache->itemsChanged(change.index, change.count, change.roles);
+                    break;
                 }
             }
+        }
 
-            syncDone.wakeAll();
-            locker.unlock();
+        syncDone.wakeAll();
+        locker.unlock();
 
-            if (cc)
-                emit m_orig->countChanged();
-        } else {
-            syncDone.wakeAll();
-        }
+        if (cc)
+            emit m_orig->countChanged();
     }
 
     return QObject::event(e);

diff --git a/src/declarative/util/qdeclarativelistmodelworkeragent_p.h b/src/declarative/util/qdeclarativelistmodelworkeragent_p.h
index b6c42ae..5865138 100644 (file)
--- a/src/declarative/util/qdeclarativelistmodelworkeragent_p.h
+++ b/src/declarative/util/qdeclarativelistmodelworkeragent_p.h
@@ -76,7 +76,6 @@ class QDeclarativeListModelWorkerAgent : public QObject
 
 public:
     QDeclarativeListModelWorkerAgent(QDeclarativeListModel *);
-    ~QDeclarativeListModelWorkerAgent();
 
     void setV8Engine(QV8Engine *eng);
 

diff --git a/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp b/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp
index 3ce3d5f..6c26581 100644 (file)
--- a/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp
+++ b/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp
@@ -248,12 +248,12 @@ void tst_qdeclarativelistmodel::static_types_data()
     QTest::newRow("role error")
         << "ListElement { foo: 1 } ListElement { foo: 'string' }"
         << QVariant()
-        << QString("<Unknown File>: Can't assign to pre-existing role of different type foo");
+        << QString("<Unknown File>: Can't assign to existing role 'foo' of different type [String -> Number]");
 
     QTest::newRow("list type error")
         << "ListElement { foo: 1 } ListElement { foo: ListElement { bar: 1 } }"
         << QVariant()
-        << QString("<Unknown File>: Can't assign to pre-existing role of different type foo");
+        << QString("<Unknown File>: Can't assign to existing role 'foo' of different type [List -> Number]");
 }
 
 void tst_qdeclarativelistmodel::static_types()
@@ -491,7 +491,24 @@ void tst_qdeclarativelistmodel::dynamic_data()
 
     QTest::newRow("large1") << "{append({'a':1,'b':2,'c':3,'d':4,'e':5,'f':6,'g':7,'h':8});get(0).h}" << 8 << "";
 
-    QTest::newRow("datatypes1") << "{append({'a':1});append({'a':'string'});}" << 0 << "<Unknown File>: Can't assign to pre-existing role of different type a";
+    QTest::newRow("datatypes1") << "{append({'a':1});append({'a':'string'});}" << 0 << "<Unknown File>: Can't assign to existing role 'a' of different type [String -> Number]";
+
+    QTest::newRow("null") << "{append({'a':null});}" << 0 << "";
+    QTest::newRow("setNull") << "{append({'a':1});set(0, {'a':null});}" << 0 << "";
+    QTest::newRow("setString") << "{append({'a':'hello'});set(0, {'a':'world'});get(0).a == 'world'}" << 1 << "";
+    QTest::newRow("setInt") << "{append({'a':5});set(0, {'a':10});get(0).a}" << 10 << "";
+    QTest::newRow("setNumber") << "{append({'a':6});set(0, {'a':5.5});get(0).a < 5.6}" << 1 << "";
+    QTest::newRow("badType0") << "{append({'a':'hello'});set(0, {'a':1});}" << 0 << "<Unknown File>: Can't assign to existing role 'a' of different type [Number -> String]";
+    QTest::newRow("invalidInsert0") << "{insert(0);}" << 0 << "<Unknown File>: QML ListModel: insert: value is not an object";
+    QTest::newRow("invalidAppend0") << "{append();}" << 0 << "<Unknown File>: QML ListModel: append: value is not an object";
+    QTest::newRow("invalidInsert1") << "{insert(0, 34);}" << 0 << "<Unknown File>: QML ListModel: insert: value is not an object";
+    QTest::newRow("invalidAppend1") << "{append(37);}" << 0 << "<Unknown File>: QML ListModel: append: value is not an object";
+
+    // QObjects
+    QTest::newRow("qobject0") << "{append({'a':dummyItem0});}" << 0 << "";
+    QTest::newRow("qobject1") << "{append({'a':dummyItem0});set(0,{'a':dummyItem1});get(0).a == dummyItem1;}" << 1 << "";
+    QTest::newRow("qobject2") << "{append({'a':dummyItem0});get(0).a == dummyItem0;}" << 1 << "";
+    QTest::newRow("qobject3") << "{append({'a':dummyItem0});append({'b':1});}" << 0 << "";
 
     // Nested models
     QTest::newRow("nested-append1") << "{append({'foo':123,'bars':[{'a':1},{'a':2},{'a':3}]});count}" << 1 << "";
@@ -511,10 +528,13 @@ void tst_qdeclarativelistmodel::dynamic()
     QFETCH(int, result);
     QFETCH(QString, warning);
 
+    QQuickItem dummyItem0, dummyItem1;
     QDeclarativeEngine engine;
     QDeclarativeListModel model;
     QDeclarativeEngine::setContextForObject(&model,engine.rootContext());
     engine.rootContext()->setContextObject(&model);
+    engine.rootContext()->setContextProperty("dummyItem0", QVariant::fromValue(&dummyItem0));
+    engine.rootContext()->setContextProperty("dummyItem1", QVariant::fromValue(&dummyItem1));
     QDeclarativeExpression e(engine.rootContext(), &model, script);
     if (!warning.isEmpty())
         QTest::ignoreMessage(QtWarningMsg, warning.toLatin1());
@@ -542,6 +562,9 @@ void tst_qdeclarativelistmodel::dynamic_worker()
     QFETCH(int, result);
     QFETCH(QString, warning);
 
+    if (QByteArray(QTest::currentDataTag()).startsWith("qobject"))
+        return;
+
     // This is same as dynamic() except it applies the test to a ListModel called
     // from a WorkerScript.
 
@@ -587,6 +610,9 @@ void tst_qdeclarativelistmodel::dynamic_worker_sync()
     QFETCH(int, result);
     QFETCH(QString, warning);
 
+    if (QByteArray(QTest::currentDataTag()).startsWith("qobject"))
+        return;
+
     // This is the same as dynamic_worker() except that it executes a set of list operations
     // from the worker script, calls sync(), and tests the changes are reflected in the
     // list in the main thread