Oxide

Merge lp:~aacid/oxide/qt54_variantjs into lp:~oxide-developers/oxide/oxide.trunk

  1. qt54_variantjs
  2. Merge into oxide.trunk
Proposed by Albert Astals Cid
Status: Merged
Merged at revision: 954
Proposed branch: lp:~aacid/oxide/qt54_variantjs
Merge into: lp:~oxide-developers/oxide/oxide.trunk
Diff against target: 82 lines (+30/-11)
2 files modified
qt/quick/api/oxideqquickwebcontextdelegateworker.cc (+18/-9)
qt/quick/api/oxideqquickwebframe.cc (+12/-2)
To merge this branch: bzr merge lp:~aacid/oxide/qt54_variantjs
Reviewer Review Type Date Requested Status
Chris Coulson Approve
Review via email: [email protected]

Commit message

Adapt to behaviour change in handling of QVariants from QML to C++ in Qt 5.4

[QTBUG-40431] When a JavaScript object/array is passed to C++ through
  a QVariant, the engine no longer immediately converts the object
  recursively into a QVariantMap or QVariantList but instead stores a
  QJSValue in the QVariant. This prevents a loss of data when the JS
  object contains non-primitive types such as function objects for
  example. Code that expects the variant type to be exactly
  QVariant::Map or QVariant::List may need to be adapted. Registered
  conversion functions however ensure that code that merely calls
  toMap() or toList() continues to work. Fixes: https://bugs.launchpad.net/bugs/1417963

To post a comment you must log in.
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good to me, and I’ve verified that the unit tests still pass with Qt 5.3.
Haven’t tested with Qt 5.4 yet.

Revision history for this message
Timo Jyrinki (timo-jyrinki) wrote :

I'll build it against Qt 5.4 once there's a version of the patch that applies to either 1.4 or 1.5. Olivier is looking at it.

Revision history for this message
Timo Jyrinki (timo-jyrinki) wrote :

Ok I've it rebased for 1.4 (just some fuzz) and it'd apply against 1.5 fine. I'll report once the armhf 1.4 build patched with this branch has finished building.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

This Qt change kind of sucks - it's effectively an API break in what should be a stable API. It also means that if existing objects without a QtQml dependency have QVariant properties, they need to add a QtQml dependency for the object to be usable in Qml when they really only needed a QtCore dependency before.

If developers wanted to avoid a loss of data in the case where the JS object contains non-primitive types, surely they would just expose a QJSValue rather than using QVariant and making existing users of QVariant jump through an extra hoop (where we just discard the non-primitive types anyway, resulting in the same loss of data as before)?

review: Needs Information
Revision history for this message
Albert Astals Cid (aacid) wrote :

> This Qt change kind of sucks
Agreed

> It also means that if existing objects without a QtQml dependency have QVariant properties,
> they need to add a QtQml dependency for the object to be usable in Qml when they really only
> needed a QtCore dependency before.
Or they need to stop checking for the type of things, checking for the type of a QVariant is not a great idea when there's canConvert that will do the right thing.

> If developers wanted to avoid a loss of data in the case where the JS object contains
> non-primitive types, surely they would just expose a QJSValue rather than using QVariant and
> making existing users of QVariant jump through an extra hoop (where we just discard the
> non-primitive types anyway, resulting in the same loss of data as before)?
I didn't do that change, so i can't provide any more insight than the changelog entry.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

I'll approve this as I guess we'll need to do it anyway. It's a shame they couldn't have fixed that bug another way though

review: Approve
Revision history for this message
Chris Coulson (chrisccoulson) wrote :

And thanks for fixing it

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qt/quick/api/oxideqquickwebcontextdelegateworker.cc'
2--- qt/quick/api/oxideqquickwebcontextdelegateworker.cc 2014-10-16 19:14:06 +0000
3+++ qt/quick/api/oxideqquickwebcontextdelegateworker.cc 2015-02-05 14:08:26 +0000
4@@ -116,13 +116,17 @@
5 controller_(controller) {}
6
7 void Api::sendMessage(const QVariant& message) {
8- if (message.type() != QVariant::Map &&
9- message.type() != QVariant::List &&
10- message.type() != QVariant::StringList) {
11+ QVariant aux = message;
12+ if (aux.userType() == qMetaTypeId<QJSValue>()) {
13+ aux = aux.value<QJSValue>().toVariant();
14+ }
15+ if (aux.type() != QVariant::Map &&
16+ aux.type() != QVariant::List &&
17+ aux.type() != QVariant::StringList) {
18 return;
19 }
20
21- Q_EMIT controller_->sendMessage(message);
22+ Q_EMIT controller_->sendMessage(aux);
23 }
24
25 void IOThreadControllerImpl::CallEntryPointInWorker(
26@@ -300,16 +304,21 @@
27 void OxideQQuickWebContextDelegateWorker::sendMessage(const QVariant& message) {
28 Q_D(OxideQQuickWebContextDelegateWorker);
29
30- if (message.type() != QVariant::Map &&
31- message.type() != QVariant::List &&
32- message.type() != QVariant::StringList) {
33- qWarning() << "Called WebContextDelegateWorker.sendMessage with an invalid argument";
34+ QVariant aux = message;
35+ if (aux.userType() == qMetaTypeId<QJSValue>()) {
36+ aux = aux.value<QJSValue>().toVariant();
37+ }
38+
39+ if (aux.type() != QVariant::Map &&
40+ aux.type() != QVariant::List &&
41+ aux.type() != QVariant::StringList) {
42+ qWarning() << "Called WebContextDelegateWorker.sendMessage with an invalid argument" << aux;
43 return;
44 }
45
46 QMetaObject::invokeMethod(d->io_thread_controller_.data(),
47 "receiveMessage",
48- Q_ARG(QVariant, message));
49+ Q_ARG(QVariant, aux));
50 }
51
52 #include "oxideqquickwebcontextdelegateworker.moc"
53
54=== modified file 'qt/quick/api/oxideqquickwebframe.cc'
55--- qt/quick/api/oxideqquickwebframe.cc 2015-01-16 22:46:17 +0000
56+++ qt/quick/api/oxideqquickwebframe.cc 2015-02-05 14:08:26 +0000
57@@ -193,7 +193,12 @@
58 OxideQQuickScriptMessageRequest* request =
59 new OxideQQuickScriptMessageRequest();
60
61- if (!d->sendMessage(context, msg_id, args,
62+ QVariant aux = args;
63+ if (aux.userType() == qMetaTypeId<QJSValue>()) {
64+ aux = aux.value<QJSValue>().toVariant();
65+ }
66+
67+ if (!d->sendMessage(context, msg_id, aux,
68 OxideQQuickScriptMessageRequestPrivate::get(request))) {
69 delete request;
70 return nullptr;
71@@ -207,5 +212,10 @@
72 const QVariant& args) {
73 Q_D(OxideQQuickWebFrame);
74
75- d->sendMessageNoReply(context, msg_id, args);
76+ QVariant aux = args;
77+ if (aux.userType() == qMetaTypeId<QJSValue>()) {
78+ aux = aux.value<QJSValue>().toVariant();
79+ }
80+
81+ d->sendMessageNoReply(context, msg_id, aux);
82 }

Subscribers

People subscribed via source and target branches