Reviewing merge request #2287: Patch to QTBUG-914
Patch to bug in QtDBus - "writeProxy() not generating signals correctly in all cases". Modified writeProxy(), writeAdaptor(), makeArgName and writeArgList() of qdbusxml2cpp.cpp, in order to allow the correct "out" parameters processing for signals.
Commits that would be merged:
Comments
hi, thanks.
please rebase your commits against current master (git pull —rebase mainline master) and make a forced push to your repository. merge commits in MRs are a really bad idea, both for actual reviewing and because they overload gitorious.
Rebased and forced push as requested.
I suggest you to make two merge request instead.
The bugs are completely unrelated, it’s going to be more difficult to have a review for the two commits together.
About http://bugreports.qt.nokia.com/browse/QTBUG-5357
It has been fixed upstream: https://bugs.webkit.org/show_bug.cgi?id=30549
Please comment on webkit.org if you think your patch is better.
For WebKit, we don’t use qt.gitorious.org. QtWebKit is developed directly on webkit.org and the patch should be done there: http://trac.webkit.org/wiki/QtWebKitContrib.
I put in revise and resubmit. Please resubmit a226e30 alone. It’s probably good, but I don’t know enough about DBus to review it.
Resubmitted a226e30 (now with another code) alone. Will create another merge request for QTBUG-5357 later.
Hi, sorry for the delay.
The patch is incorrect. It correctly changes the direction of the signal annotation, but it also changes the C++ code it generates.
So instead of:
void NameOwnerChanged(const QString &in0, const QString &in1, const QString &in2);
We now have:
void NameOwnerChanged(QString &out0, QString &out1, QString &out2);
Also, for compatibility reasons, could you make the code fall back to the older annotation too?
Hello, I tried to reproduce the above error with the xml file below, but the end result for the “next” method (which would be similar to the NameOwnerChanged method above) seems to be correct:
inline Q_NOREPLY void next(const QString &in0, const QString &in1, const QString &in2) {…}
Maybe I didn’t understand what the error is.
Also, what do you mean by “older annotation”?
<!DOCTYPE node PUBLIC “–//freedesktop//DTD D-BUS Object Introspection 1.0//EN”
“http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd”>
<annotation name="org.freedesktop.DBus.Method.NoReply"
value="true" />
<arg name="" type="(isss)" direction="in" />
<annotation name="com.trolltech.QtDBus.QtTypeName.In0" value="QString" />
<arg name="" type="(isss)" direction="in" />
<annotation name="com.trolltech.QtDBus.QtTypeName.In1" value="QString" />
<arg name="" type="(isss)" direction="in" />
<annotation name="com.trolltech.QtDBus.QtTypeName.In2" value="QString" />
<arg name ="" type="(isss)" direction="in" />
<annotation name="com.trolltech.QtDBus.QtTypeName.In0"
value="QString" />
<arg name ="" type="(isss)" direction="out" />
<annotation name="com.trolltech.QtDBus.QtTypeName.Out0" value="QString" />
<arg name="" type="(isss)" direction="out" />
<annotation name="com.trolltech.QtDBus.QtTypeName.Out0" value="Soprano::Node" />
I'm marking this as New in order to further discuss its revision.
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


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