[nemo-qml-plugin-dbus] Recursively convert dbus arguments to standard qt types #30
Conversation
| break; | ||
|
|
||
| default: | ||
| /* Unhandled types produce invalid QVariant */ |
There was a problem hiding this comment.
QDBusArgument::MapEntryType is not handled -- is this a problem in practice?
There was a problem hiding this comment.
The default action could print out a warning about unhandled type. Maybe a qWarning and qmlInfo to help debugging in QML?
There was a problem hiding this comment.
From dbus spec:"A DICT_ENTRY works exactly like a struct, but rather than parentheses it uses curly braces, and it has more restrictions. The restrictions are: it occurs only as an array element type ..."
In qt arrays that contain dict entries -> MapType, i.e. MapEntryType should occur only within MapType context - where it is now handled via beginMapEntry() ... endMapentry().
There was a problem hiding this comment.
Will add qWarning() & qmlInfo. The latter needs this pointer -> changing unwind() to private method.
spiiroin
force-pushed
the
jb23167_normalize_args
branch
from
86fccf6
to
8667079
Compare
|
Can we have one or two unit tests with this? Also, how does it prevent recursively calling itself over and over again (for maybe some strange data type) -- maybe a "maximum recursion" int parameter with a default value that is passed decremented by one to recursive calls of the same function could help here? (also, we might want to have an upper limit to avoid performance issues that go undetected -- 20 recursions are probably deep enough, and if a data structure needs more than that, we might have to optimize, so better fail loudly there?) |
|
Doing this on low level dbus api directly would be safe. But since there is a lot of implicit class construction going on, the variant cases where attempt is made to remove the dbus variant just might result in the data being wrapped again in double-variant. No idea if that is really possible, but it could be - will add recursion limit. |
spiiroin
force-pushed
the
jb23167_normalize_args
branch
from
8667079
to
e8061bf
Compare
|
LGTM |
… qt types. Fixes JB#23167 Partial dbus argument normalization resulted in qml code seeing the values returned from typedCall() as inaccessible dbus container within qt variant types, for example QVariant(QDbusVariant), Try to recursively convert dbus reply message arguments into normal qt types that the script engine can deal with. Also perform similar recursive normalization for arguments passed to dbus signal handlers.
…y it Add dbus test service that implements method calls: - repr: Takes any combination of dbus arguments and returns string representation of the args. Can be used to check that messages send from qml side are encoded on D-Bus in expected manner. - echo: Takes any combination of dbus arguments and returns them back to caller as is. Can be used to check that messages are decoded to QML data in expected manner. - ping: Like echo, but before sending the reply message a pong-signal with the same data is broadcast. Can be used to check that signals are parsed to QML data similarly to method call return messages. - quit: Terminates test server. Usually not needed as the test server will auto-terminate in 5 seconds if no more method call messages are received. - Additionally repr, echo and ping methods use service side generated message content if string parameter "COMPLEX<number>" is received. Can be used to check that also parsing of D-Bus messages that can't be created from QML code are handled correctly. The test service is implemented in C using low level libdbus code to minimize chances of any misfiring type promotion issues in Qt and QML layers happening also on the service side and thus potentially hiding problems. Add test application that: - Uses the above mentioned test service to check whether both sending and receiving of all types of dbus messages that should be supported by the plugin actually work as expected. - Executes 110 tests in total - of which 49 fail when using the previous version of the dbus plugin. Issues found and fixed: - DBUS_TYPE_BYTE was handled as int8 instead of uint8 - Integer types < 32 bits (DBUS_TYPE_BYTE|INT16|UINT16) were encoded as DBUS_TYPE_INT32|UINT32 - While DBUS_TYPE_INT64|UINT64 was encoded correctly, the values from QML side were interpreted incorrectly and effective range was limited to 32 bits - Decoding DBUS_TYPE_OBJECT_PATH failed and values could not be accessed from QML side - DBUS_TYPE_UNIX_FD was encoded as DBUS_TYPE_UINT32 - Only arrays of DBUS_TYPE_STRING could be sent and received - When sending values as variant, the values were not wrapped in DBUS_TYPE_VARIANT containers - When sending arrays as variant, the items within the array were wrapped in DBUS_TYPE_VARIANT containers - Decoding DBUS_TYPE_VARIANT failed and values could not be accessed from QML side - Signal argument decoding did not share logic with return value and thus some datatypes that did work as return values could not be parsed from signal messages - Incorrect usage of QGenericArgument() in QML signal handler invoking could cause problems ranging from wrong data in QML side to crashes depending on datatypes
spiiroin
force-pushed
the
jb23167_normalize_args
branch
from
e8061bf
to
a156d0f
Compare
| ** | ||
| ******************************************************************************/ | ||
| import QtQuick 2.0 | ||
| import Sailfish.Silica 1.0 |
There was a problem hiding this comment.
You can remove Sailfish.Silica import, replace ApplicationWindow with Item, and replace
initialPage: Component {
Page {
Component.onCompleted: initDelay.start()
}
}
With
Component.onCompleted: initDelay.start()
There was a problem hiding this comment.
You could consider using QML TestCase, see http://doc.qt.io/qt-5/qml-qttest-testcase.html#details
That said, making asynchronous test cases with TestCase may require some extra work.
|
The test package needs a test definition, see for example how nemo-qml-plugin-accounts: https://github.com/nemomobile/nemo-qml-plugin-accounts/blob/master/tests/tests.xml . |
|
Test definition docs here: The test definition can then be passed to testrunner-lite (from package testrunner-lite in repo mer-tools) and it runs the test set:
|
|
I ended up doing modifications to the sources of this PR as I reviewed it. So, my review comments (excluding splitting commits) are embodied in this PR which adds to the orginal PR: spiiroin#1 |
Restructure test directories, use QtTest, beautify
|
LGTM. |
| * or if the resulting list would be empty: use the | ||
| * value without modification */ | ||
| QVariantList arr = var.toList(); | ||
| if( arr.empty() ) |
There was a problem hiding this comment.
Nitpicking: part of the code use spaces between an expression and spaces, whereas some parts do not. E.g.
if ( expr ) ;
vs
if (x) ;
|
LGTM, I think this is a really good addition! Noticed a minor code formatting issue, fix if you think it is important. |
[nemo-qml-plugin-dbus] Recursively convert dbus arguments to standard qt types
Partial dbus argument normalization resulted in qml code seeing the
values returned from typedCall() as inaccessible dbus container
within qt variant types, for example QVariant(QDbusVariant),
Try to recursively convert dbus reply message arguments into normal qt
types that the script engine can deal with.
Also perform similar recursive normalization for arguments passed to
dbus signal handlers.