From d8d547b92e02546cb030ac9d4a45fe0d89e0e7f6 Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Thu, 17 Jun 2010 12:27:43 +0200 Subject: Usefully convert from QtScript object/array to QVariant For arrays, the conversion would crash if the array was cyclic. Introduce a set that keeps track of which objects are being converted, and return an empty list when a cycle is detected. For other types of objects, the object was previously attempted to be converted to a primitive, which typically meant you would get the string representation ("[object Object]"), since most (practically all) objects can be converted to a string -- not useful at all. Change the conversion so it converts the object to a QVariantMap instead. This was already done for slots that took a QVariantMap as argument, but only one level deep. Make the conversion recursive, using the same mechanism as for arrays to detect cycles. This change also means that you get a meaningful JS object => QVariant => JS object roundtrip. It also aligns the behavior with the Qt WebKit bridge. Update the documentation to describe the new behavior. The 4.7 changelog will also be updated under "important behavioral changes". This change exposed an issue with one of the QML autotests: A JS object was assigned to a QVariant property, which caused it to be converted to a string (rather than a QVariantMap) -- just shows that the previous behavior was unintuitive). Later, this variant property is compared to another object, the intention being to compare the _properties_ of the two objects; but because the variant property contained a string, this would cause the other operand (object) to be converted to a string as well ("[object Object]"), causing a meaningless test pass. Change the test to deserialize both objects using JSON.stringify, and compare the resulting strings, so that actual JS object => QVariant(Map) => JS object roundtrip is tested (the intention). Task-number: QTBUG-3511 Reviewed-by: Olivier Goffart --- src/script/api/qscriptengine.cpp | 30 +++-- src/script/api/qscriptengine_p.h | 6 +- src/script/api/qscriptvalue.cpp | 4 +- .../qdeclarativeworkerscript/data/BaseWorker.qml | 2 +- .../tst_qdeclarativeworkerscript.cpp | 6 +- .../qscriptextqobject/tst_qscriptextqobject.cpp | 140 +++++++++++++++++++++ tests/auto/qscriptvalue/tst_qscriptvalue.cpp | 89 ++++++++++++- tests/auto/qscriptvalue/tst_qscriptvalue.h | 2 + 8 files changed, 257 insertions(+), 22 deletions(-) diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp index f02ea52..e2999c1 100644 --- a/src/script/api/qscriptengine.cpp +++ b/src/script/api/qscriptengine.cpp @@ -1011,12 +1011,17 @@ JSC::JSValue QScriptEnginePrivate::arrayFromVariantList(JSC::ExecState *exec, co return arr; } -QVariantList QScriptEnginePrivate::variantListFromArray(JSC::ExecState *exec, JSC::JSValue arr) +QVariantList QScriptEnginePrivate::variantListFromArray(JSC::ExecState *exec, JSC::JSArray *arr) { + QScriptEnginePrivate *eng = QScript::scriptEngineFromExec(exec); + if (eng->visitedConversionObjects.contains(arr)) + return QVariantList(); // Avoid recursion. + eng->visitedConversionObjects.insert(arr); QVariantList lst; uint len = toUInt32(exec, property(exec, arr, exec->propertyNames().length)); for (uint i = 0; i < len; ++i) lst.append(toVariant(exec, property(exec, arr, i))); + eng->visitedConversionObjects.remove(arr); return lst; } @@ -1029,14 +1034,19 @@ JSC::JSValue QScriptEnginePrivate::objectFromVariantMap(JSC::ExecState *exec, co return obj; } -QVariantMap QScriptEnginePrivate::variantMapFromObject(JSC::ExecState *exec, JSC::JSValue obj) +QVariantMap QScriptEnginePrivate::variantMapFromObject(JSC::ExecState *exec, JSC::JSObject *obj) { + QScriptEnginePrivate *eng = QScript::scriptEngineFromExec(exec); + if (eng->visitedConversionObjects.contains(obj)) + return QVariantMap(); // Avoid recursion. + eng->visitedConversionObjects.insert(obj); JSC::PropertyNameArray propertyNames(exec); - JSC::asObject(obj)->getOwnPropertyNames(exec, propertyNames, JSC::IncludeDontEnumProperties); + obj->getOwnPropertyNames(exec, propertyNames, JSC::IncludeDontEnumProperties); QVariantMap vmap; JSC::PropertyNameArray::const_iterator it = propertyNames.begin(); for( ; it != propertyNames.end(); ++it) vmap.insert(it->ustring(), toVariant(exec, property(exec, obj, *it))); + eng->visitedConversionObjects.remove(obj); return vmap; } @@ -1661,16 +1671,10 @@ QVariant QScriptEnginePrivate::toVariant(JSC::ExecState *exec, JSC::JSValue valu return QVariant(toRegExp(exec, value)); #endif else if (isArray(value)) - return variantListFromArray(exec, value); + return variantListFromArray(exec, JSC::asArray(value)); else if (QScriptDeclarativeClass *dc = declarativeClass(value)) return dc->toVariant(declarativeObject(value)); - // try to convert to primitive - JSC::JSValue savedException; - saveException(exec, &savedException); - JSC::JSValue prim = value.toPrimitive(exec); - restoreException(exec, savedException); - if (!prim.isObject()) - return toVariant(exec, prim); + return variantMapFromObject(exec, JSC::asObject(value)); } else if (value.isNumber()) { return QVariant(toNumber(exec, value)); } else if (value.isString()) { @@ -3129,12 +3133,12 @@ bool QScriptEnginePrivate::convertValue(JSC::ExecState *exec, JSC::JSValue value } break; case QMetaType::QVariantList: if (isArray(value)) { - *reinterpret_cast(ptr) = variantListFromArray(exec, value); + *reinterpret_cast(ptr) = variantListFromArray(exec, JSC::asArray(value)); return true; } break; case QMetaType::QVariantMap: if (isObject(value)) { - *reinterpret_cast(ptr) = variantMapFromObject(exec, value); + *reinterpret_cast(ptr) = variantMapFromObject(exec, JSC::asObject(value)); return true; } break; case QMetaType::QVariant: diff --git a/src/script/api/qscriptengine_p.h b/src/script/api/qscriptengine_p.h index fd47208..56366e2 100644 --- a/src/script/api/qscriptengine_p.h +++ b/src/script/api/qscriptengine_p.h @@ -215,10 +215,10 @@ public: static QStringList stringListFromArray(JSC::ExecState*, JSC::JSValue arr); static JSC::JSValue arrayFromVariantList(JSC::ExecState*, const QVariantList &lst); - static QVariantList variantListFromArray(JSC::ExecState*, JSC::JSValue arr); + static QVariantList variantListFromArray(JSC::ExecState*, JSC::JSArray *arr); static JSC::JSValue objectFromVariantMap(JSC::ExecState*, const QVariantMap &vmap); - static QVariantMap variantMapFromObject(JSC::ExecState*, JSC::JSValue obj); + static QVariantMap variantMapFromObject(JSC::ExecState*, JSC::JSObject *obj); JSC::JSValue defaultPrototype(int metaTypeId) const; void setDefaultPrototype(int metaTypeId, JSC::JSValue prototype); @@ -378,6 +378,8 @@ public: QHash loadedScripts; QScriptValue m_currentException; + QSet visitedConversionObjects; + #ifndef QT_NO_QOBJECT QHash m_qobjectData; #endif diff --git a/src/script/api/qscriptvalue.cpp b/src/script/api/qscriptvalue.cpp index 1310c8c..451d1b0 100644 --- a/src/script/api/qscriptvalue.cpp +++ b/src/script/api/qscriptvalue.cpp @@ -1215,8 +1215,8 @@ qsreal QScriptValue::toInteger() const \row \o QObject Object \o A QVariant containing a pointer to the QObject. \row \o Date Object \o A QVariant containing the date value (toDateTime()). \row \o RegExp Object \o A QVariant containing the regular expression value (toRegExp()). - \row \o Array Object \o The array is converted to a QVariantList. - \row \o Object \o If the value is primitive, then the result is converted to a QVariant according to the above rules; otherwise, an invalid QVariant is returned. + \row \o Array Object \o The array is converted to a QVariantList. Each element is converted to a QVariant, recursively; cyclic references are not followed. + \row \o Object \o The object is converted to a QVariantMap. Each property is converted to a QVariant, recursively; cyclic references are not followed. \endtable \sa isVariant() diff --git a/tests/auto/declarative/qdeclarativeworkerscript/data/BaseWorker.qml b/tests/auto/declarative/qdeclarativeworkerscript/data/BaseWorker.qml index d275ca8..e06afa2 100644 --- a/tests/auto/declarative/qdeclarativeworkerscript/data/BaseWorker.qml +++ b/tests/auto/declarative/qdeclarativeworkerscript/data/BaseWorker.qml @@ -13,7 +13,7 @@ WorkerScript { function compareLiteralResponse(expected) { var e = eval('(' + expected + ')') - return worker.response == e + return JSON.stringify(worker.response) == JSON.stringify(e) } onMessage: { diff --git a/tests/auto/declarative/qdeclarativeworkerscript/tst_qdeclarativeworkerscript.cpp b/tests/auto/declarative/qdeclarativeworkerscript/tst_qdeclarativeworkerscript.cpp index 52c11e9..8e98874 100644 --- a/tests/auto/declarative/qdeclarativeworkerscript/tst_qdeclarativeworkerscript.cpp +++ b/tests/auto/declarative/qdeclarativeworkerscript/tst_qdeclarativeworkerscript.cpp @@ -170,13 +170,15 @@ void tst_QDeclarativeWorkerScript::messaging_sendJsObject() QDeclarativeWorkerScript *worker = qobject_cast(component.create()); QVERIFY(worker != 0); - QString jsObject = "{'name': 'zyz', 'spell power': 3101, 'haste': 1125}"; + // Properties are in alphabetical order to enable string-based comparison after + // QVariant roundtrip, since the properties will be stored in a QVariantMap. + QString jsObject = "{'haste': 1125, 'name': 'zyz', 'spell power': 3101}"; QScriptEngine *engine = QDeclarativeEnginePrivate::getScriptEngine(qmlEngine(worker)); QScriptValue sv = engine->newObject(); + sv.setProperty("haste", 1125); sv.setProperty("name", "zyz"); sv.setProperty("spell power", 3101); - sv.setProperty("haste", 1125); QVERIFY(QMetaObject::invokeMethod(worker, "testSend", Q_ARG(QVariant, qVariantFromValue(sv)))); waitForEchoMessage(worker); diff --git a/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp b/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp index 09070e6..11813d8 100644 --- a/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp +++ b/tests/auto/qscriptextqobject/tst_qscriptextqobject.cpp @@ -287,6 +287,8 @@ public: { m_qtFunctionInvoked = 15; m_actuals << v; return v; } Q_INVOKABLE QVariantMap myInvokableWithVariantMapArg(const QVariantMap &vm) { m_qtFunctionInvoked = 16; m_actuals << vm; return vm; } + Q_INVOKABLE QVariantList myInvokableWithVariantListArg(const QVariantList &lst) + { m_qtFunctionInvoked = 62; m_actuals.append(QVariant(lst)); return lst; } Q_INVOKABLE QList myInvokableWithListOfIntArg(const QList &lst) { m_qtFunctionInvoked = 17; m_actuals << qVariantFromValue(lst); return lst; } Q_INVOKABLE QObject* myInvokableWithQObjectStarArg(QObject *obj) @@ -535,6 +537,10 @@ private slots: void emitAfterReceiverDeleted(); void inheritedSlots(); void enumerateMetaObject(); + void nestedArrayAsSlotArgument_data(); + void nestedArrayAsSlotArgument(); + void nestedObjectAsSlotArgument_data(); + void nestedObjectAsSlotArgument(); private: QScriptEngine *m_engine; @@ -3164,5 +3170,139 @@ void tst_QScriptExtQObject::enumerateMetaObject() } } +void tst_QScriptExtQObject::nestedArrayAsSlotArgument_data() +{ + QTest::addColumn("program"); + QTest::addColumn("expected"); + + QTest::newRow("[[]]") + << QString::fromLatin1("[[]]") + << (QVariantList() << (QVariant(QVariantList()))); + QTest::newRow("[[123]]") + << QString::fromLatin1("[[123]]") + << (QVariantList() << (QVariant(QVariantList() << 123))); + QTest::newRow("[[], 123]") + << QString::fromLatin1("[[], 123]") + << (QVariantList() << QVariant(QVariantList()) << 123); + + // Cyclic + QTest::newRow("var a=[]; a.push(a)") + << QString::fromLatin1("var a=[]; a.push(a); a") + << (QVariantList() << QVariant(QVariantList())); + QTest::newRow("var a=[]; a.push(123, a)") + << QString::fromLatin1("var a=[]; a.push(123, a); a") + << (QVariantList() << 123 << QVariant(QVariantList())); + QTest::newRow("var a=[]; var b=[]; a.push(b); b.push(a)") + << QString::fromLatin1("var a=[]; var b=[]; a.push(b); b.push(a); a") + << (QVariantList() << QVariant(QVariantList() << QVariant(QVariantList()))); + QTest::newRow("var a=[]; var b=[]; a.push(123, b); b.push(456, a)") + << QString::fromLatin1("var a=[]; var b=[]; a.push(123, b); b.push(456, a); a") + << (QVariantList() << 123 << QVariant(QVariantList() << 456 << QVariant(QVariantList()))); +} + +void tst_QScriptExtQObject::nestedArrayAsSlotArgument() +{ + QFETCH(QString, program); + QFETCH(QVariantList, expected); + QScriptValue a = m_engine->evaluate(program); + QVERIFY(!a.isError()); + QVERIFY(a.isArray()); + // Slot that takes QVariantList + { + QVERIFY(!m_engine->evaluate("myObject.myInvokableWithVariantListArg") + .call(QScriptValue(), QScriptValueList() << a).isError()); + QCOMPARE(m_myObject->qtFunctionInvoked(), 62); + QCOMPARE(m_myObject->qtFunctionActuals().size(), 1); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).type(), QVariant::List); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).toList(), expected); + } + // Slot that takes QVariant + { + m_myObject->resetQtFunctionInvoked(); + QVERIFY(!m_engine->evaluate("myObject.myInvokableWithVariantArg") + .call(QScriptValue(), QScriptValueList() << a).isError()); + QCOMPARE(m_myObject->qtFunctionInvoked(), 15); + QCOMPARE(m_myObject->qtFunctionActuals().size(), 1); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).type(), QVariant::List); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).toList(), expected); + } +} + +void tst_QScriptExtQObject::nestedObjectAsSlotArgument_data() +{ + QTest::addColumn("program"); + QTest::addColumn("expected"); + + { + QVariantMap m; + m["a"] = QVariantMap(); + QTest::newRow("{ a:{} }") + << QString::fromLatin1("({ a:{} })") + << m; + } + { + QVariantMap m, m2; + m2["b"] = 10; + m2["c"] = 20; + m["a"] = m2; + QTest::newRow("{ a:{b:10, c:20} }") + << QString::fromLatin1("({ a:{b:10, c:20} })") + << m; + } + { + QVariantMap m; + m["a"] = 10; + m["b"] = QVariantList() << 20 << 30; + QTest::newRow("{ a:10, b:[20, 30]}") + << QString::fromLatin1("({ a:10, b:[20,30]})") + << m; + } + + // Cyclic + { + QVariantMap m; + m["p"] = QVariantMap(); + QTest::newRow("var o={}; o.p=o") + << QString::fromLatin1("var o={}; o.p=o; o") + << m; + } + { + QVariantMap m; + m["p"] = 123; + m["q"] = QVariantMap(); + QTest::newRow("var o={}; o.p=123; o.q=o") + << QString::fromLatin1("var o={}; o.p=123; o.q=o; o") + << m; + } +} + +void tst_QScriptExtQObject::nestedObjectAsSlotArgument() +{ + QFETCH(QString, program); + QFETCH(QVariantMap, expected); + QScriptValue o = m_engine->evaluate(program); + QVERIFY(!o.isError()); + QVERIFY(o.isObject()); + // Slot that takes QVariantMap + { + QVERIFY(!m_engine->evaluate("myObject.myInvokableWithVariantMapArg") + .call(QScriptValue(), QScriptValueList() << o).isError()); + QCOMPARE(m_myObject->qtFunctionInvoked(), 16); + QCOMPARE(m_myObject->qtFunctionActuals().size(), 1); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).type(), QVariant::Map); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).toMap(), expected); + } + // Slot that takes QVariant + { + m_myObject->resetQtFunctionInvoked(); + QVERIFY(!m_engine->evaluate("myObject.myInvokableWithVariantArg") + .call(QScriptValue(), QScriptValueList() << o).isError()); + QCOMPARE(m_myObject->qtFunctionInvoked(), 15); + QCOMPARE(m_myObject->qtFunctionActuals().size(), 1); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).type(), QVariant::Map); + QCOMPARE(m_myObject->qtFunctionActuals().at(0).toMap(), expected); + } +} + QTEST_MAIN(tst_QScriptExtQObject) #include "tst_qscriptextqobject.moc" diff --git a/tests/auto/qscriptvalue/tst_qscriptvalue.cpp b/tests/auto/qscriptvalue/tst_qscriptvalue.cpp index aa9fa15..8aa4e711 100644 --- a/tests/auto/qscriptvalue/tst_qscriptvalue.cpp +++ b/tests/auto/qscriptvalue/tst_qscriptvalue.cpp @@ -1270,7 +1270,7 @@ void tst_QScriptValue::toVariant_old() QCOMPARE(opaque.toVariant(), var); QScriptValue object = eng.newObject(); - QCOMPARE(object.toVariant(), QVariant(QString("[object Object]"))); + QCOMPARE(object.toVariant(), QVariant(QVariantMap())); QScriptValue qobject = eng.newQObject(this); { @@ -2296,7 +2296,7 @@ void tst_QScriptValue::getSetScriptClass() QCOMPARE(obj.scriptClass(), (QScriptClass*)&testClass); QVERIFY(obj.isObject()); QVERIFY(!obj.isVariant()); - QVERIFY(!obj.toVariant().isValid()); + QCOMPARE(obj.toVariant(), QVariant(QVariantMap())); } { QScriptValue obj = eng.newQObject(this); @@ -3472,4 +3472,89 @@ void tst_QScriptValue::objectId() QVERIFY(obj.strictlyEquals(eng.globalObject())); } +void tst_QScriptValue::nestedObjectToVariant_data() +{ + QTest::addColumn("program"); + QTest::addColumn("expected"); + + // Array literals + QTest::newRow("[[]]") + << QString::fromLatin1("[[]]") + << QVariant(QVariantList() << (QVariant(QVariantList()))); + QTest::newRow("[[123]]") + << QString::fromLatin1("[[123]]") + << QVariant(QVariantList() << (QVariant(QVariantList() << 123))); + QTest::newRow("[[], 123]") + << QString::fromLatin1("[[], 123]") + << QVariant(QVariantList() << QVariant(QVariantList()) << 123); + + // Cyclic arrays + QTest::newRow("var a=[]; a.push(a)") + << QString::fromLatin1("var a=[]; a.push(a); a") + << QVariant(QVariantList() << QVariant(QVariantList())); + QTest::newRow("var a=[]; a.push(123, a)") + << QString::fromLatin1("var a=[]; a.push(123, a); a") + << QVariant(QVariantList() << 123 << QVariant(QVariantList())); + QTest::newRow("var a=[]; var b=[]; a.push(b); b.push(a)") + << QString::fromLatin1("var a=[]; var b=[]; a.push(b); b.push(a); a") + << QVariant(QVariantList() << QVariant(QVariantList() << QVariant(QVariantList()))); + QTest::newRow("var a=[]; var b=[]; a.push(123, b); b.push(456, a)") + << QString::fromLatin1("var a=[]; var b=[]; a.push(123, b); b.push(456, a); a") + << QVariant(QVariantList() << 123 << QVariant(QVariantList() << 456 << QVariant(QVariantList()))); + + // Object literals + { + QVariantMap m; + m["a"] = QVariantMap(); + QTest::newRow("{ a:{} }") + << QString::fromLatin1("({ a:{} })") + << QVariant(m); + } + { + QVariantMap m, m2; + m2["b"] = 10; + m2["c"] = 20; + m["a"] = m2; + QTest::newRow("{ a:{b:10, c:20} }") + << QString::fromLatin1("({ a:{b:10, c:20} })") + << QVariant(m); + } + { + QVariantMap m; + m["a"] = 10; + m["b"] = QVariantList() << 20 << 30; + QTest::newRow("{ a:10, b:[20, 30]}") + << QString::fromLatin1("({ a:10, b:[20,30]})") + << QVariant(m); + } + + // Cyclic objects + { + QVariantMap m; + m["p"] = QVariantMap(); + QTest::newRow("var o={}; o.p=o") + << QString::fromLatin1("var o={}; o.p=o; o") + << QVariant(m); + } + { + QVariantMap m; + m["p"] = 123; + m["q"] = QVariantMap(); + QTest::newRow("var o={}; o.p=123; o.q=o") + << QString::fromLatin1("var o={}; o.p=123; o.q=o; o") + << QVariant(m); + } +} + +void tst_QScriptValue::nestedObjectToVariant() +{ + QScriptEngine eng; + QFETCH(QString, program); + QFETCH(QVariant, expected); + QScriptValue o = eng.evaluate(program); + QVERIFY(!o.isError()); + QVERIFY(o.isObject()); + QCOMPARE(o.toVariant(), expected); +} + QTEST_MAIN(tst_QScriptValue) diff --git a/tests/auto/qscriptvalue/tst_qscriptvalue.h b/tests/auto/qscriptvalue/tst_qscriptvalue.h index aae35b2..8bfaa6a 100644 --- a/tests/auto/qscriptvalue/tst_qscriptvalue.h +++ b/tests/auto/qscriptvalue/tst_qscriptvalue.h @@ -225,6 +225,8 @@ private slots: void engineDeleted(); void valueOfWithClosure(); void objectId(); + void nestedObjectToVariant_data(); + void nestedObjectToVariant(); private: typedef void (tst_QScriptValue::*InitDataFunction)(); -- cgit v0.12