content-hub

Merge lp:~michael-sheldon/content-hub/fix-content-item-name into lp:content-hub

  1. fix-content-item-name
  2. Merge into trunk
Proposed by Michael Sheldon
Status: Merged
Approved by: Ken VanDine
Approved revision: 115
Merged at revision: 125
Proposed branch: lp:~michael-sheldon/content-hub/fix-content-item-name
Merge into: lp:content-hub
Diff against target: 410 lines (+105/-32)
13 files modified
import/Ubuntu/Content/contentitem.cpp (+5/-3)
import/Ubuntu/Content/contentitem.h (+0/-1)
include/com/ubuntu/content/item.h (+15/-0)
src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml (+2/-2)
src/com/ubuntu/content/detail/service.cpp (+5/-3)
src/com/ubuntu/content/detail/transfer.cpp (+13/-10)
src/com/ubuntu/content/detail/transfer.h (+2/-2)
src/com/ubuntu/content/hub.cpp (+2/-0)
src/com/ubuntu/content/item.cpp (+42/-2)
src/com/ubuntu/content/service/main.cpp (+1/-0)
src/com/ubuntu/content/transfer_p.h (+10/-8)
tests/acceptance-tests/app_hub_communication_transfer.cpp (+6/-0)
tests/qml-tests/tst_ContentHub.qml (+2/-1)
To merge this branch: bzr merge lp:~michael-sheldon/content-hub/fix-content-item-name
Reviewer Review Type Date Requested Status
Ken VanDine Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+226669@code.launchpad.net

Commit message

Transfer entire content Item objects over dbus, instead of just their URL component. Adds name property to C++ backend and connects to existing QML property.

Description of the change

Transfers entire content Item objects over dbus, instead of just their URL component. Allowing for additional properties to be provided, such as the name property which this branch also adds to the C++ backend and connects to the existing QML name property.

To post a comment you must log in.
Revision history for this message
Michael Sheldon (michael-sheldon) wrote :

Are there any related MPs required for this MP to build/function as expected? Please list.

 * No

Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)

 * Yes

Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?

 * Yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/content-hub) on device or emulator?

 * Yes

If you changed the UI, was the change specified/approved by design?

 * No change

If you changed the packaging (debian), did you add a core-dev as a reviewer to this MP?

 * No change

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:112
http://jenkins.qa.ubuntu.com/job/content-hub-ci/173/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-amd64-ci/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-armhf-ci/30
        deb: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-armhf-ci/30/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-i386-ci/30

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/content-hub-ci/173/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

Looks good, but lets add the name to the items created in the transfer tests to be safe.

review: Needs Fixing
lp:~michael-sheldon/content-hub/fix-content-item-name updated
113. By Michael Sheldon

Include name in content item comparison

114. By Michael Sheldon

Update tests to set names on items

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:114
http://jenkins.qa.ubuntu.com/job/content-hub-ci/174/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-amd64-ci/31
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-armhf-ci/31
        deb: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-armhf-ci/31/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-i386-ci/31

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/content-hub-ci/174/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

I see you register the dbus type for cuc::Item in the service's main.cpp, which works fine. Why not register it in the cucd::Service constructor for places where the service is instantiated outside of main.cpp? I think the test suite creates a cucd::Service.

review: Needs Information
lp:~michael-sheldon/content-hub/fix-content-item-name updated
115. By Michael Sheldon

Register Item with dbus in Service object constructor

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:115
http://jenkins.qa.ubuntu.com/job/content-hub-ci/175/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-amd64-ci/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-armhf-ci/32
        deb: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-armhf-ci/32/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/content-hub-utopic-i386-ci/32

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/content-hub-ci/175/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

Did you perform an exploratory manual test run of the code change and any related functionality on device or emulator?

 * Yes

Did CI run pass? If not, please explain why.

 * Yes

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?

 * Yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'import/Ubuntu/Content/contentitem.cpp'
2--- import/Ubuntu/Content/contentitem.cpp 2014-07-10 18:03:55 +0000
3+++ import/Ubuntu/Content/contentitem.cpp 2014-07-15 14:31:29 +0000
4@@ -47,16 +47,16 @@
5 const QString &ContentItem::name() const
6 {
7 TRACE() << Q_FUNC_INFO;
8- return m_name;
9+ return m_item.name();
10 }
11
12 void ContentItem::setName(const QString &name)
13 {
14 TRACE() << Q_FUNC_INFO;
15- if (name == m_name)
16+ if (name == m_item.name())
17 return;
18
19- m_name = name;
20+ m_item.setName(name);
21 Q_EMIT nameChanged();
22 }
23
24@@ -77,7 +77,9 @@
25 if (url == this->url())
26 return;
27
28+ QString oldName = m_item.name();
29 m_item = cuc::Item(url);
30+ m_item.setName(oldName);
31 Q_EMIT urlChanged();
32 }
33
34
35=== modified file 'import/Ubuntu/Content/contentitem.h'
36--- import/Ubuntu/Content/contentitem.h 2014-07-10 18:03:55 +0000
37+++ import/Ubuntu/Content/contentitem.h 2014-07-15 14:31:29 +0000
38@@ -50,7 +50,6 @@
39 void urlChanged();
40
41 private:
42- QString m_name;
43 com::ubuntu::content::Item m_item;
44 };
45
46
47=== modified file 'include/com/ubuntu/content/item.h'
48--- include/com/ubuntu/content/item.h 2013-08-21 12:08:58 +0000
49+++ include/com/ubuntu/content/item.h 2014-07-15 14:31:29 +0000
50@@ -18,6 +18,7 @@
51 #ifndef COM_UBUNTU_CONTENT_ITEM_H_
52 #define COM_UBUNTU_CONTENT_ITEM_H_
53
54+#include <QtDBus>
55 #include <QObject>
56 #include <QSharedPointer>
57 #include <QUrl>
58@@ -33,6 +34,7 @@
59 {
60 Q_OBJECT
61 Q_PROPERTY(QUrl url READ url())
62+ Q_PROPERTY(QString name READ name WRITE setName)
63
64 public:
65 Item(const QUrl& = QUrl(), QObject* = nullptr);
66@@ -43,6 +45,8 @@
67 bool operator==(const Item&) const;
68
69 Q_INVOKABLE const QUrl& url() const;
70+ Q_INVOKABLE const QString& name() const;
71+ Q_INVOKABLE void setName(const QString &name) const;
72
73 private:
74 struct Private;
75@@ -53,4 +57,15 @@
76 }
77 }
78
79+Q_DECL_EXPORT
80+QDBusArgument &operator<<(QDBusArgument &argument,
81+ const com::ubuntu::content::Item &item);
82+
83+Q_DECL_EXPORT
84+const QDBusArgument &operator>>(const QDBusArgument &argument,
85+ com::ubuntu::content::Item &item);
86+
87+Q_DECLARE_METATYPE(com::ubuntu::content::Item)
88+
89+
90 #endif // COM_UBUNTU_CONTENT_ITEM_H_
91
92=== modified file 'src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml'
93--- src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml 2014-04-03 12:00:41 +0000
94+++ src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml 2014-07-15 14:31:29 +0000
95@@ -20,10 +20,10 @@
96 <method name="Download">
97 </method>
98 <method name="Charge">
99- <arg name="items" type="as" direction="in" />
100+ <arg name="items" type="av" direction="in" />
101 </method>
102 <method name="Collect">
103- <arg name="items" type="as" direction="out" />
104+ <arg name="items" type="av" direction="out" />
105 </method>
106 <method name="Store">
107 <arg name="uri" type="s" direction="out" />
108
109=== modified file 'src/com/ubuntu/content/detail/service.cpp'
110--- src/com/ubuntu/content/detail/service.cpp 2014-07-07 12:39:38 +0000
111+++ src/com/ubuntu/content/detail/service.cpp 2014-07-15 14:31:29 +0000
112@@ -32,6 +32,7 @@
113 #include <unistd.h>
114 #include <libnotify/notify.h>
115
116+#include <com/ubuntu/content/item.h>
117 #include <com/ubuntu/content/peer.h>
118 #include <com/ubuntu/content/type.h>
119 #include <com/ubuntu/content/transfer.h>
120@@ -92,6 +93,7 @@
121 assert(!peer_registry.isNull());
122
123 qDBusRegisterMetaType<cuc::Peer>();
124+ qDBusRegisterMetaType<cuc::Item>();
125
126 m_watcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
127 m_watcher->setConnection(d->connection);
128@@ -202,7 +204,7 @@
129 Q_UNUSED(action);
130
131 cucd::Transfer* t = (cucd::Transfer*)data;
132- t->Charge(QStringList());
133+ t->Charge(QVariantList());
134 }
135
136 void download_notify (cucd::Transfer* t)
137@@ -571,7 +573,7 @@
138 {
139 TRACE() << Q_FUNC_INFO << "Found downloaded import, charging";
140 if (r->handler->isValid())
141- t->Charge(QStringList());
142+ t->Charge(QVariantList());
143 }
144 }
145 }
146@@ -588,7 +590,7 @@
147 if (t->Direction() == cuc::Transfer::Export)
148 {
149 TRACE() << Q_FUNC_INFO << "Found downloaded import, charging";
150- t->Charge(QStringList());
151+ t->Charge(QVariantList());
152 }
153 }
154 }
155
156=== modified file 'src/com/ubuntu/content/detail/transfer.cpp'
157--- src/com/ubuntu/content/detail/transfer.cpp 2014-05-28 10:09:29 +0000
158+++ src/com/ubuntu/content/detail/transfer.cpp 2014-07-15 14:31:29 +0000
159@@ -52,7 +52,7 @@
160 int direction;
161 QString store;
162 int selection_type;
163- QStringList items;
164+ QVariantList items;
165 bool source_started_by_content_hub;
166 QString download_id;
167 };
168@@ -141,7 +141,7 @@
169 Q_EMIT(StateChanged(d->state));
170 }
171
172-void cucd::Transfer::Charge(const QStringList& items)
173+void cucd::Transfer::Charge(const QVariantList& items)
174 {
175 TRACE() << __PRETTY_FUNCTION__;
176
177@@ -161,12 +161,14 @@
178 return;
179 }
180
181- QStringList ret;
182- Q_FOREACH(QString i, items)
183- ret.append(copy_to_store(i, d->store));
184-
185- Q_FOREACH(QString f, ret)
186- TRACE() << Q_FUNC_INFO << "Item:" << f;
187+ QVariantList ret;
188+ Q_FOREACH(QVariant iv, items) {
189+ cuc::Item origItem = qdbus_cast<Item>(iv);
190+ cuc::Item copiedItem = cuc::Item{QUrl(copy_to_store(origItem.url().toString(), d->store))};
191+ copiedItem.setName(origItem.name());
192+ TRACE() << Q_FUNC_INFO << "Item:" << copiedItem.url();
193+ ret.append(QVariant::fromValue(copiedItem));
194+ }
195
196 if (ret.count() <= 0)
197 {
198@@ -211,7 +213,8 @@
199 void cucd::Transfer::DownloadComplete(QString destFilePath)
200 {
201 TRACE() << __PRETTY_FUNCTION__;
202- d->items.append(QUrl::fromLocalFile(destFilePath).toString());
203+ cuc::Item item = cuc::Item{QUrl::fromLocalFile(destFilePath).toString()};
204+ d->items.append(QVariant::fromValue(item));
205 d->state = cuc::Transfer::downloaded;
206 Q_EMIT(StateChanged(d->state));
207 }
208@@ -226,7 +229,7 @@
209 Q_EMIT(StateChanged(d->state));
210 }
211
212-QStringList cucd::Transfer::Collect()
213+QVariantList cucd::Transfer::Collect()
214 {
215 TRACE() << __PRETTY_FUNCTION__;
216
217
218=== modified file 'src/com/ubuntu/content/detail/transfer.h'
219--- src/com/ubuntu/content/detail/transfer.h 2014-05-22 09:11:23 +0000
220+++ src/com/ubuntu/content/detail/transfer.h 2014-07-15 14:31:29 +0000
221@@ -63,8 +63,8 @@
222 int State();
223 void Start();
224 void Handled();
225- void Charge(const QStringList&);
226- QStringList Collect();
227+ void Charge(const QVariantList&);
228+ QVariantList Collect();
229 void Abort();
230 void Finalize();
231 QString Store();
232
233=== modified file 'src/com/ubuntu/content/hub.cpp'
234--- src/com/ubuntu/content/hub.cpp 2014-05-30 17:55:50 +0000
235+++ src/com/ubuntu/content/hub.cpp 2014-07-15 14:31:29 +0000
236@@ -63,6 +63,8 @@
237 setLoggingLevel(value);
238 }
239
240+ qDBusRegisterMetaType<cuc::Item>();
241+
242 if (qApp)
243 qApp->installEventFilter(this);
244 }
245
246=== modified file 'src/com/ubuntu/content/item.cpp'
247--- src/com/ubuntu/content/item.cpp 2013-07-16 11:04:48 +0000
248+++ src/com/ubuntu/content/item.cpp 2014-07-15 14:31:29 +0000
249@@ -16,21 +16,25 @@
250 * Authored by: Thomas Voß <thomas.voss@canonical.com>
251 */
252
253+#include <QDBusArgument>
254+
255 #include <com/ubuntu/content/item.h>
256+#include "debug.h"
257
258 namespace cuc = com::ubuntu::content;
259
260 struct cuc::Item::Private
261 {
262 QUrl url;
263+ QString name;
264
265 bool operator==(const Private& rhs) const
266 {
267- return url == rhs.url;
268+ return url == rhs.url && name == rhs.name;
269 }
270 };
271
272-cuc::Item::Item(const QUrl& url, QObject* parent) : QObject(parent), d{new cuc::Item::Private{url}}
273+cuc::Item::Item(const QUrl& url, QObject* parent) : QObject(parent), d{new cuc::Item::Private{url, QString()}}
274 {
275 }
276
277@@ -60,3 +64,39 @@
278 {
279 return d->url;
280 }
281+
282+const QString& cuc::Item::name() const
283+{
284+ return d->name;
285+}
286+
287+void cuc::Item::setName(const QString& newName) const
288+{
289+ if (newName != d->name)
290+ d->name = newName;
291+}
292+
293+QDBusArgument &operator<<(QDBusArgument &argument, const cuc::Item& item)
294+{
295+ argument.beginStructure();
296+ argument << item.name() << item.url().toDisplayString();
297+ argument.endStructure();
298+ return argument;
299+}
300+
301+const QDBusArgument &operator>>(const QDBusArgument &argument, cuc::Item &item)
302+{
303+ TRACE() << Q_FUNC_INFO;
304+ QString name;
305+ QString urlString;
306+
307+
308+ argument.beginStructure();
309+ argument >> name >> urlString;
310+ argument.endStructure();
311+
312+ item = cuc::Item{QUrl(urlString)};
313+ item.setName(name);
314+ return argument;
315+}
316+
317
318=== modified file 'src/com/ubuntu/content/service/main.cpp'
319--- src/com/ubuntu/content/service/main.cpp 2014-05-01 17:03:40 +0000
320+++ src/com/ubuntu/content/service/main.cpp 2014-07-15 14:31:29 +0000
321@@ -19,6 +19,7 @@
322 #include <QCoreApplication>
323 #include <QProcessEnvironment>
324 #include <csignal>
325+#include <com/ubuntu/content/item.h>
326
327 #include "detail/app_manager.h"
328 #include "debug.h"
329
330=== modified file 'src/com/ubuntu/content/transfer_p.h'
331--- src/com/ubuntu/content/transfer_p.h 2014-04-03 12:00:41 +0000
332+++ src/com/ubuntu/content/transfer_p.h 2014-07-15 14:31:29 +0000
333@@ -106,13 +106,12 @@
334
335 bool charge(const QVector<Item>& items)
336 {
337- QStringList l;
338+ QVariantList itemVariants;
339 Q_FOREACH(const Item& item, items)
340- {
341- l << item.url().toDisplayString();
342+ {
343+ itemVariants << QVariant::fromValue(item);
344 }
345-
346- auto reply = remote_transfer->Charge(l);
347+ auto reply = remote_transfer->Charge(itemVariants);
348 reply.waitForFinished();
349
350 return not reply.isError();
351@@ -128,10 +127,13 @@
352 if (reply.isError())
353 return result;
354
355- Q_FOREACH(const QString& url, reply.value())
356- {
357- result << Item(QUrl(url));
358+ auto items = reply.value();
359+
360+ Q_FOREACH(const QVariant& itemVariant, items)
361+ {
362+ result << qdbus_cast<Item>(itemVariant);
363 }
364+
365 return result;
366 }
367
368
369=== modified file 'tests/acceptance-tests/app_hub_communication_transfer.cpp'
370--- tests/acceptance-tests/app_hub_communication_transfer.cpp 2014-02-21 02:30:11 +0000
371+++ tests/acceptance-tests/app_hub_communication_transfer.cpp 2014-07-15 14:31:29 +0000
372@@ -128,13 +128,19 @@
373 {
374 QVector<cuc::Item> source_items;
375 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file1").absoluteFilePath()));
376+ source_items[0].setName("name1");
377 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file2").absoluteFilePath()));
378+ source_items[1].setName("name2");
379 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file3").absoluteFilePath()));
380+ source_items[2].setName("name3");
381
382 QVector<cuc::Item> expected_items;
383 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file1"));
384+ expected_items[0].setName("name1");
385 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file2"));
386+ expected_items[1].setName("name2");
387 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file3"));
388+ expected_items[2].setName("name3");
389
390 /** [Importing pictures] */
391 auto hub = cuc::Hub::Client::instance();
392
393=== modified file 'tests/qml-tests/tst_ContentHub.qml'
394--- tests/qml-tests/tst_ContentHub.qml 2014-03-07 21:03:36 +0000
395+++ tests/qml-tests/tst_ContentHub.qml 2014-07-15 14:31:29 +0000
396@@ -34,12 +34,13 @@
397 function test_export_request() {
398 var filePath = "file:///foo/bar.png";
399 var transfer = destPeer.request();
400- transfer.items = [ resultComponent.createObject(test, {"url": filePath}) ];
401+ transfer.items = [ resultComponent.createObject(test, {"url": filePath, "name": "test"}) ];
402 transfer.state = ContentTransfer.Charged;
403 // This shouldn't be necessary, but without it we compare the results to fast
404 ContentHub.exportRequested(transfer);
405 compare(test.exportTransfer, transfer, "Transfer object not correcty set");
406 compare(test.exportTransfer.items[0].url, filePath, "Transfer contents incorrect");
407+ compare(test.exportTransfer.items[0].name, "test", "Transfer name incorrect");
408 }
409
410 Component {

Subscribers

People subscribed via source and target branches