Merge lp:~autopilot/autopilot-qt/update-for-1.3 into lp:autopilot-qt
- update-for-1.3
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Thomi Richards | ||||
Approved revision: | 60 | ||||
Merged at revision: | 53 | ||||
Proposed branch: | lp:~autopilot/autopilot-qt/update-for-1.3 | ||||
Merge into: | lp:autopilot-qt | ||||
Diff against target: |
233 lines (+43/-23) 7 files modified
debian/changelog (+6/-0) debian/control (+2/-2) driver/qtnode.cpp (+13/-7) driver/qtnode.h (+3/-1) driver/rootnode.cpp (+8/-3) driver/rootnode.h (+1/-0) tests/unittests/tst_introspection.cpp (+10/-10) |
||||
To merge this branch: | bzr merge lp:~autopilot/autopilot-qt/update-for-1.3 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Thomi Richards (community) | Approve | ||
PS Jenkins bot | continuous-integration | Approve | |
Michael Zanetti (community) | Needs Fixing | ||
Review via email:
|
Commit message
Update dbus wire protocol to that needed for 1.3
Description of the change
This branch changes autopilot-qt to build against the latest libxpathselect, and updates it's dbus wire protocol to the latest version.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Zanetti (mzanetti) wrote : | # |
Given that this is a Qt project I think we should try to adapt Qt quidelines.
Please use QString instead of std::string and one of Qt's shared pointer mechanisms. In fact, I'm not a big fan of shared pointers at all as in most cases they lead to bad design choices. So if not really necessary, try avoiding those.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Zanetti (mzanetti) wrote : | # |
I'd like to get the tests from the other MP merged before this for 2 reasons:
- knowing what this breaks before merging it
- the tests should still be released to raring while this will not.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : | # |
> Given that this is a Qt project I think we should try to adapt Qt quidelines.
>
> Please use QString instead of std::string and one of Qt's shared pointer
> mechanisms.
Michael,
The GetName and GetPath methods are defined in xpathselect. We cannot use QStrings here since xpathselect does not (and should not) rely on Qt. Similarly, we use std::shared_ptr because that's what the interface defines.
> In fact, I'm not a big fan of shared pointers at all as in most
> cases they lead to bad design choices. So if not really necessary, try
> avoiding those.
I'm not sure what you mean - this is a classic case where shared_ptrs are necessary. Qt has it's fancy copy-on-write semantics which make passing objects around by value very cheap, but other toolkits don't have that. Since we're dealing with an interface that has to be toolkit-agnostic, we can't start using Qt's magic.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : | # |
> I'd like to get the tests from the other MP merged before this for 2 reasons:
> - knowing what this breaks before merging it
> - the tests should still be released to raring while this will not.
I'm happy if you want to merge the tests with trunk, but please remember that trunk will never get released into raring. If you want your tests to go into raring you need to merge them in the stable branch.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:57
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Zanetti (mzanetti) wrote : | # |
7 + * Bump version number
Doesn't seem to describe what this changeset actually does
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:57
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Zanetti (mzanetti) wrote : | # |
hmm.. doesn't build in jenkins because libpathselect >= 1.3 cannot be found. Is that already in some PPA I could add to the job?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Zanetti (mzanetti) wrote : | # |
> The GetName and GetPath methods are defined in xpathselect. We cannot use
> QStrings here since xpathselect does not (and should not) rely on Qt.
> Similarly, we use std::shared_ptr because that's what the interface defines.
Right... missed that.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Zanetti (mzanetti) wrote : | # |
Tests are merged to trunk and they fail because of the GatPath changes.
This would need to be merged with them and updated to pass CI (once Jenkins can find libxpathselect >= 1.3.)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Terry (mterry) wrote : | # |
> Doesn't seem to describe what this changeset actually does
The other change will be added to the changelog by jenkins due to the linked bug.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:57
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:58
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:59
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:60
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:60
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : | # |
CI job passes, tests pass. Approving now.
Preview Diff
1 | === modified file 'debian/changelog' |
2 | --- debian/changelog 2013-03-22 00:01:49 +0000 |
3 | +++ debian/changelog 2013-04-21 21:24:26 +0000 |
4 | @@ -1,3 +1,9 @@ |
5 | +autopilot-qt (1.3) UNRELEASED; urgency=low |
6 | + |
7 | + * Bump version number |
8 | + |
9 | + -- Christopher Lee <chris.lee@canonical.com> Thu, 18 Apr 2013 15:53:10 +1200 |
10 | + |
11 | autopilot-qt (0.8daily13.03.22-0ubuntu1) raring; urgency=low |
12 | |
13 | [ Sebastien Bacher ] |
14 | |
15 | === modified file 'debian/control' |
16 | --- debian/control 2013-04-18 14:16:36 +0000 |
17 | +++ debian/control 2013-04-21 21:24:26 +0000 |
18 | @@ -10,7 +10,7 @@ |
19 | libqt4-dev, |
20 | qtbase5-dev, |
21 | qtdeclarative5-dev, |
22 | - libxpathselect-dev, |
23 | + libxpathselect-dev (>= 1.3), |
24 | mesa-common-dev, |
25 | libgl1-mesa-dev, |
26 | libgles2-mesa-dev, |
27 | @@ -28,7 +28,7 @@ |
28 | Section: libs |
29 | Architecture: amd64 armel armhf i386 |
30 | Depends: ${misc:Depends}, |
31 | - libxpathselect1.2, |
32 | + libxpathselect1.3, |
33 | Replaces: autopilot-qt, autopilot-qt5 |
34 | Conflicts: autopilot-qt, autopilot-qt5 |
35 | Provides: autopilot-qt, autopilot-qt5 |
36 | |
37 | === modified file 'driver/qtnode.cpp' |
38 | --- driver/qtnode.cpp 2013-01-17 19:48:27 +0000 |
39 | +++ driver/qtnode.cpp 2013-04-21 21:24:26 +0000 |
40 | @@ -18,9 +18,10 @@ |
41 | |
42 | const QByteArray AP_ID_NAME("_autopilot_id"); |
43 | |
44 | -QtNode::QtNode(QObject *obj) |
45 | +QtNode::QtNode(QObject *obj, std::string const& parent_path) |
46 | : object_(obj) |
47 | { |
48 | + full_path_ = parent_path + "/" + GetName(); |
49 | } |
50 | |
51 | QObject* QtNode::getWrappedObject() const |
52 | @@ -31,7 +32,7 @@ |
53 | QVariant QtNode::IntrospectNode() const |
54 | { |
55 | // return must be (name, state_map) |
56 | - QString object_name = QString::fromStdString(GetName()); |
57 | + QString object_name = QString::fromStdString(GetPath()); |
58 | QVariantMap object_properties = GetNodeProperties(object_); |
59 | object_properties["id"] = GetObjectId(); |
60 | QList<QVariant> object_tuple = { QVariant(object_name), QVariant(object_properties) }; |
61 | @@ -62,6 +63,11 @@ |
62 | return name.toStdString(); |
63 | } |
64 | |
65 | +std::string QtNode::GetPath() const |
66 | +{ |
67 | + return full_path_; |
68 | +} |
69 | + |
70 | bool QtNode::MatchProperty(const std::string& name, const std::string& value) const |
71 | { |
72 | if (name == "id") |
73 | @@ -95,21 +101,21 @@ |
74 | |
75 | QQuickView *view = qobject_cast<QQuickView*>(object_); |
76 | if (view) { |
77 | - children.push_back(std::make_shared<QtNode>(view->rootObject())); |
78 | + children.push_back(std::make_shared<QtNode>(view->rootObject(), GetPath())); |
79 | } |
80 | |
81 | QQuickItem* item = qobject_cast<QQuickItem*>(object_); |
82 | if (item) { |
83 | foreach (QQuickItem *childItem, item->childItems()) { |
84 | if (childItem->parentItem() == item) { |
85 | - children.push_back(std::make_shared<QtNode>(childItem)); |
86 | + children.push_back(std::make_shared<QtNode>(childItem, GetPath())); |
87 | } |
88 | } |
89 | } else { |
90 | foreach (QObject *child, object_->children()) |
91 | { |
92 | if (child->parent() == object_) |
93 | - children.push_back(std::make_shared<QtNode>(child)); |
94 | + children.push_back(std::make_shared<QtNode>(child, GetPath())); |
95 | } |
96 | } |
97 | |
98 | @@ -117,7 +123,7 @@ |
99 | foreach (QObject *child, object_->children()) |
100 | { |
101 | if (child->parent() == object_) |
102 | - children.push_back(std::make_shared<QtNode>(child)); |
103 | + children.push_back(std::make_shared<QtNode>(child, GetPath())); |
104 | } |
105 | |
106 | // If our wrapped object is a QGraphicsScene, we need to explicitly grab any child graphics |
107 | @@ -131,7 +137,7 @@ |
108 | { |
109 | QGraphicsObject *obj = item->toGraphicsObject(); |
110 | if (obj && ! obj->parent()) |
111 | - children.push_back(std::make_shared<QtNode>(obj)); |
112 | + children.push_back(std::make_shared<QtNode>(obj, GetPath())); |
113 | } |
114 | } |
115 | #endif |
116 | |
117 | === modified file 'driver/qtnode.h' |
118 | --- driver/qtnode.h 2012-08-23 22:01:03 +0000 |
119 | +++ driver/qtnode.h 2013-04-21 21:24:26 +0000 |
120 | @@ -14,7 +14,7 @@ |
121 | public: |
122 | typedef std::shared_ptr<QtNode> Ptr; |
123 | |
124 | - QtNode(QObject* object); |
125 | + QtNode(QObject* object, std::string const& parent_path); |
126 | |
127 | QObject* getWrappedObject() const; |
128 | |
129 | @@ -22,10 +22,12 @@ |
130 | virtual qint64 GetObjectId() const; |
131 | |
132 | virtual std::string GetName() const; |
133 | + virtual std::string GetPath() const; |
134 | virtual bool MatchProperty(const std::string& name, const std::string& value) const; |
135 | virtual xpathselect::NodeList Children() const; |
136 | private: |
137 | QObject *object_; |
138 | + std::string full_path_; |
139 | }; |
140 | |
141 | #endif // QTNODE_H |
142 | |
143 | === modified file 'driver/rootnode.cpp' |
144 | --- driver/rootnode.cpp 2013-03-15 12:50:02 +0000 |
145 | +++ driver/rootnode.cpp 2013-04-21 21:24:26 +0000 |
146 | @@ -5,7 +5,7 @@ |
147 | #include <QStringList> |
148 | |
149 | RootNode::RootNode(QCoreApplication* application) |
150 | - : QtNode(application) |
151 | + : QtNode(application, std::string()) |
152 | , application_(application) |
153 | { |
154 | } |
155 | @@ -13,7 +13,7 @@ |
156 | QVariant RootNode::IntrospectNode() const |
157 | { |
158 | // return must be (name, state_map) |
159 | - QString object_name = QString::fromStdString(GetName()); |
160 | + QString object_name = QString::fromStdString(GetPath()); |
161 | QStringList child_names; |
162 | foreach(QObject* child, children_) |
163 | { |
164 | @@ -43,6 +43,11 @@ |
165 | return appName.isEmpty() ? "Root" : appName.toStdString(); |
166 | } |
167 | |
168 | +std::string RootNode::GetPath() const |
169 | +{ |
170 | + return "/" + GetName(); |
171 | +} |
172 | + |
173 | bool RootNode::MatchProperty(const std::string& name, const std::string& value) const |
174 | { |
175 | if (name == "id") |
176 | @@ -55,6 +60,6 @@ |
177 | { |
178 | xpathselect::NodeList children; |
179 | foreach(QObject* child, children_) |
180 | - children.push_back(std::make_shared<QtNode>(child)); |
181 | + children.push_back(std::make_shared<QtNode>(child, GetPath())); |
182 | return children; |
183 | } |
184 | |
185 | === modified file 'driver/rootnode.h' |
186 | --- driver/rootnode.h 2012-08-11 00:46:21 +0000 |
187 | +++ driver/rootnode.h 2013-04-21 21:24:26 +0000 |
188 | @@ -19,6 +19,7 @@ |
189 | void AddChild(QObject* child); |
190 | |
191 | virtual std::string GetName() const; |
192 | + virtual std::string GetPath() const; |
193 | virtual bool MatchProperty(const std::string& name, const std::string& value) const; |
194 | virtual xpathselect::NodeList Children() const; |
195 | private: |
196 | |
197 | === modified file 'tests/unittests/tst_introspection.cpp' |
198 | --- tests/unittests/tst_introspection.cpp 2013-03-18 20:10:35 +0000 |
199 | +++ tests/unittests/tst_introspection.cpp 2013-04-21 21:24:26 +0000 |
200 | @@ -110,24 +110,24 @@ |
201 | QTest::addColumn<QVariant>("firstResultPropertyValue"); |
202 | |
203 | #ifdef QT5_SUPPORT |
204 | - QTest::newRow("/") << "/" << 1 << "tst_introspection" << "Children" << QVariant(QStringList() << "QMainWindow" << "QWidgetWindow"); |
205 | - QTest::newRow("//QWidget[id=6]") << "//QWidget[id=6]" << 1 << "QWidget" << "objectName" << QVariant("centralTestWidget"); |
206 | - QTest::newRow("//QPushButton[id=9]") << "//QPushButton[id=9]" << 1 << "QPushButton" << "objectName" << QVariant("myButton2"); |
207 | + QTest::newRow("/") << "/" << 1 << "/tst_introspection" << "Children" << QVariant(QStringList() << "QMainWindow" << "QWidgetWindow"); |
208 | + QTest::newRow("//QWidget[id=6]") << "//QWidget[id=6]" << 1 << "/tst_introspection/QMainWindow/QWidget" << "objectName" << QVariant("centralTestWidget"); |
209 | + QTest::newRow("//QPushButton[id=9]") << "//QPushButton[id=9]" << 1 << "/tst_introspection/QMainWindow/QWidget/QPushButton" << "objectName" << QVariant("myButton2"); |
210 | #else |
211 | - QTest::newRow("/") << "/" << 1 << "tst_introspection" << "Children" << QVariant(QStringList() << "QMainWindow"); |
212 | - QTest::newRow("//QWidget[id=5]") << "//QWidget[id=5]" << 1 << "QWidget" << "objectName" << QVariant("centralTestWidget"); |
213 | + QTest::newRow("/") << "/" << 1 << "/tst_introspection" << "Children" << QVariant(QStringList() << "QMainWindow"); |
214 | + QTest::newRow("//QWidget[id=5]") << "//QWidget[id=5]" << 1 << "/tst_introspection/QMainWindow/QWidget" << "objectName" << QVariant("centralTestWidget"); |
215 | |
216 | // Depending on the environment, Qt4 could add a second QWidget at position 6. That moves other items down by one. |
217 | if (Introspect("//QWidget[id=6]").count() > 0) { |
218 | - QTest::newRow("//QPushButton[id=9]") << "//QPushButton[id=9]" << 1 << "QPushButton" << "objectName" << QVariant("myButton2"); |
219 | + QTest::newRow("//QPushButton[id=9]") << "//QPushButton[id=9]" << 1 << "/tst_introspection/QMainWindow/QWidget/QPushButton" << "objectName" << QVariant("myButton2"); |
220 | } else { |
221 | - QTest::newRow("//QPushButton[id=8]") << "//QPushButton[id=8]" << 1 << "QPushButton" << "objectName" << QVariant("myButton2"); |
222 | + QTest::newRow("//QPushButton[id=8]") << "//QPushButton[id=8]" << 1 << "/tst_introspection/QMainWindow/QWidget/QPushButton" << "objectName" << QVariant("myButton2"); |
223 | } |
224 | #endif |
225 | |
226 | - QTest::newRow("//GridLayout") << "//QGridLayout" << 1 << "QGridLayout" << "objectName" << QVariant("myTestLayout"); |
227 | - QTest::newRow("//QPushButton") << "//QPushButton" << 2 << "QPushButton" << "objectName" << QVariant("myButton1"); |
228 | - QTest::newRow("//QWidget/*") << "//QWidget/*" << 5 << "QGridLayout" << "objectName" << QVariant("myTestLayout"); |
229 | + QTest::newRow("//GridLayout") << "//QGridLayout" << 1 << "/tst_introspection/QMainWindow/QWidget/QGridLayout" << "objectName" << QVariant("myTestLayout"); |
230 | + QTest::newRow("//QPushButton") << "//QPushButton" << 2 << "/tst_introspection/QMainWindow/QWidget/QPushButton" << "objectName" << QVariant("myButton1"); |
231 | + QTest::newRow("//QWidget/*") << "//QWidget/*" << 5 << "/tst_introspection/QMainWindow/QWidget/QGridLayout" << "objectName" << QVariant("myTestLayout"); |
232 | QTest::newRow("broken query") << "broken query" << 0 << QString() << QString() << QVariant(); |
233 | } |
234 |
FAILED: Continuous integration, rev:56 jenkins. qa.ubuntu. com/job/ autopilot- qt-ci/48/ jenkins. qa.ubuntu. com/job/ autopilot- qt-raring- amd64-ci/ 17/console jenkins. qa.ubuntu. com/job/ autopilot- qt-raring- armhf-ci/ 17/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins: 8080/job/ autopilot- qt-ci/48/ rebuild
http://