Reviewing merge request #2722: Fix for QTBUG-22840 QDBusArgument string and array extraction operators crash with incorrectly typed DBus arguments
See the bug report https://bugreports.qt.nokia.com/browse/QTBUG-22840 and commit messages.
Commits that would be merged:
Comments
Pushed new version 1
bool isCurrentTypeString();
StringLike might be a better term than String? I've considered adding “string-like” as jargon in the D-Bus Specification (as in: “the /basic types/ are divided into /fixed-length basic types/ like byte and int32, and /string-like basic types/ like string and object-path”).
// Previously QStringList extraction did not check the DBus data it was
// extracting, which results to structs with string fields to be extractable
// as QStringList. This test ensures this functionality will remain supported.
I don’t think this conversion makes sense: I realise you want Qt 4 to keep current functionality, but please make a note (or open a bug or feature request, or however that’s done in Qt) to get rid of it in Qt 5.
I prefer reviewing this on Gerrit. We should have the fix in Qt 5.0 first, then someone will backport it to 4.8.
I will push the fix to Gerrit when I am able to. My contributor agreement approval is still pending.
Pushed new version 2
Thank you form commenting Simon.
bool isCurrentTypeString();
StringLike might be a better term than String? I've considered adding “string-like” as jargon in the D-Bus Specification
Renamed the function to
bool isCurrentTypeStringLike();
to be aligned with with DBus specification terminology.
// Previously QStringList extraction did not check the DBus data it was
// extracting, which results to structs with string fields to be extractable
// as QStringList. This test ensures this functionality will remain supported.
I don’t think this conversion makes sense: I realise you want Qt 4 to keep current functionality, but please make a note (or open a bug or feature request, or however that’s done in Qt) to get rid of it in Qt 5.
I marked the test and implementation with these comments
// ### Qt5: support for DBus struct to QStringList demarshalling to be removed?
and added this next to the description of the feature in commit message
(to be removed in Qt5?)
I will leave the final decision whether to keep the DBus struct to QStringList demarshalling support to the Qt5 DBus reviewer and maintainer.
Pushed new version 3
Updated after comments received in Qt5 / gerrit.
Thank you for your contribution. With Qt 4 having moved [1] under Open
Governance [2] as well, you can now contribute your changes via Gerrit
[3]. At the same time, contributions via Gitorious can no longer be
merged.
We would still like to see your patch on the new platform, and wish
for you to become a member of the Qt Project. Please read [4] and in
particular [5] to familiarize yourself with the new tools. You will
still find a read-only mirror of all Qt repositories on Gitorious.
With a much wider set of possible reviewers, we are also looking
forward to better response times to contributions. Looking forward to
your patch!
Please note that at this point in time Qt 4.8 is a bugfix only branch,
and all fixes should be submitted to Qt 5 first, as far as
applicable. Qt 4 master is closed.
The Nokia Qt team
[1] http://labs.qt.nokia.com/2012/01/10/qt-4-moved-to-open-governance/
[2] http://labs.qt.nokia.com/2011/10/21/the-qt-project-is-live/
[3] http://codereview.qt-project.org
[4] http://www.qt-project.org
[5] http://developer.qt.nokia.com/wiki/Gerrit_Introduction
Fixes in Qt5 as commits
8f19f142745f3cb0690dcd51cebc66153e396805
b4398dc4e372dbe829b21423e1a0a93a6a542994
b9acd85b2f92f887521b952f84ced9a2d1a8a57e
Pushed the fixes to Qt4.8 gerrit:
http://codereview.qt-project.org/#change,13399
http://codereview.qt-project.org/#change,13400
http://codereview.qt-project.org/#change,13401
Closing this merge request.


Add a new comment:
Login or create an account to post a comment