From: Glenn Watson Date: Tue, 8 Nov 2011 22:11:21 +0000 (+1000) Subject: Improvements to listmodel implementation and tests. X-Git-Url: http://git.silmor.de/gitweb/?a=commitdiff_plain;h=ac5743847fa8283201458ea0ae977df366b752ab;p=konrad%2Fqtdeclarative.git 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 --- diff --git a/src/declarative/util/qdeclarativelistmodel.cpp b/src/declarative/util/qdeclarativelistmodel.cpp index 4459440..2cc52fb 100644 --- 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::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 key, QStringHash::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 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) }; - 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) }; + 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 object, QList } 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 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 *ListElement::getGuardProperty(const ListLayout::Role &role) { char *mem = getPropertyMemory(role); - QDeclarativeGuard *o = reinterpret_cast *>(mem); + + bool existingGuard = false; + for (size_t i=0 ; i < sizeof(QDeclarativeGuard) ; ++i) { + if (mem[i] != 0) { + existingGuard = true; + break; + } + } + + QDeclarativeGuard *o = 0; + + if (existingGuard) + o = reinterpret_cast *>(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 *g = reinterpret_cast *>(mem); - bool changed = g->data() != o; - g->~QDeclarativeGuard(); + bool existingGuard = false; + for (size_t i=0 ; i < sizeof(QDeclarativeGuard) ; ++i) { + if (mem[i] != 0) { + existingGuard = true; + break; + } + } + bool changed; + if (existingGuard) { + changed = g->data() != o; + g->~QDeclarativeGuard(); + } else { + changed = true; + } new (mem) QDeclarativeGuard(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 *guard = getGuardProperty(r); - guard->~QDeclarativeGuard(); + if (guard) + guard->~QDeclarativeGuard(); } break; default: @@ -999,8 +1016,6 @@ int ListElement::setJsProperty(const ListLayout::Role &role, v8::Handleappend(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 --- 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 --- 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 &changes = s->data.changes; - if (m_copy) { - bool cc = m_orig->count() != s->list->count(); - - QHash 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 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 --- 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 --- 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(": Can't assign to pre-existing role of different type foo"); + << QString(": 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(": Can't assign to pre-existing role of different type foo"); + << QString(": 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 << ": Can't assign to pre-existing role of different type a"; + QTest::newRow("datatypes1") << "{append({'a':1});append({'a':'string'});}" << 0 << ": 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 << ": Can't assign to existing role 'a' of different type [Number -> String]"; + QTest::newRow("invalidInsert0") << "{insert(0);}" << 0 << ": QML ListModel: insert: value is not an object"; + QTest::newRow("invalidAppend0") << "{append();}" << 0 << ": QML ListModel: append: value is not an object"; + QTest::newRow("invalidInsert1") << "{insert(0, 34);}" << 0 << ": QML ListModel: insert: value is not an object"; + QTest::newRow("invalidAppend1") << "{append(37);}" << 0 << ": 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