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

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 :
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
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 :
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
115. By Michael Sheldon

Register Item with dbus in Service object constructor

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
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
=== modified file 'import/Ubuntu/Content/contentitem.cpp'
--- import/Ubuntu/Content/contentitem.cpp 2014-07-10 18:03:55 +0000
+++ import/Ubuntu/Content/contentitem.cpp 2014-07-15 14:31:29 +0000
@@ -47,16 +47,16 @@
47const QString &ContentItem::name() const47const QString &ContentItem::name() const
48{48{
49 TRACE() << Q_FUNC_INFO;49 TRACE() << Q_FUNC_INFO;
50 return m_name;50 return m_item.name();
51}51}
5252
53void ContentItem::setName(const QString &name)53void ContentItem::setName(const QString &name)
54{54{
55 TRACE() << Q_FUNC_INFO;55 TRACE() << Q_FUNC_INFO;
56 if (name == m_name)56 if (name == m_item.name())
57 return;57 return;
5858
59 m_name = name;59 m_item.setName(name);
60 Q_EMIT nameChanged();60 Q_EMIT nameChanged();
61}61}
6262
@@ -77,7 +77,9 @@
77 if (url == this->url())77 if (url == this->url())
78 return;78 return;
7979
80 QString oldName = m_item.name();
80 m_item = cuc::Item(url);81 m_item = cuc::Item(url);
82 m_item.setName(oldName);
81 Q_EMIT urlChanged();83 Q_EMIT urlChanged();
82}84}
8385
8486
=== modified file 'import/Ubuntu/Content/contentitem.h'
--- import/Ubuntu/Content/contentitem.h 2014-07-10 18:03:55 +0000
+++ import/Ubuntu/Content/contentitem.h 2014-07-15 14:31:29 +0000
@@ -50,7 +50,6 @@
50 void urlChanged();50 void urlChanged();
5151
52private:52private:
53 QString m_name;
54 com::ubuntu::content::Item m_item;53 com::ubuntu::content::Item m_item;
55};54};
5655
5756
=== modified file 'include/com/ubuntu/content/item.h'
--- include/com/ubuntu/content/item.h 2013-08-21 12:08:58 +0000
+++ include/com/ubuntu/content/item.h 2014-07-15 14:31:29 +0000
@@ -18,6 +18,7 @@
18#ifndef COM_UBUNTU_CONTENT_ITEM_H_18#ifndef COM_UBUNTU_CONTENT_ITEM_H_
19#define COM_UBUNTU_CONTENT_ITEM_H_19#define COM_UBUNTU_CONTENT_ITEM_H_
2020
21#include <QtDBus>
21#include <QObject>22#include <QObject>
22#include <QSharedPointer>23#include <QSharedPointer>
23#include <QUrl>24#include <QUrl>
@@ -33,6 +34,7 @@
33{34{
34 Q_OBJECT35 Q_OBJECT
35 Q_PROPERTY(QUrl url READ url())36 Q_PROPERTY(QUrl url READ url())
37 Q_PROPERTY(QString name READ name WRITE setName)
3638
37 public:39 public:
38 Item(const QUrl& = QUrl(), QObject* = nullptr);40 Item(const QUrl& = QUrl(), QObject* = nullptr);
@@ -43,6 +45,8 @@
43 bool operator==(const Item&) const;45 bool operator==(const Item&) const;
4446
45 Q_INVOKABLE const QUrl& url() const;47 Q_INVOKABLE const QUrl& url() const;
48 Q_INVOKABLE const QString& name() const;
49 Q_INVOKABLE void setName(const QString &name) const;
4650
47 private:51 private:
48 struct Private;52 struct Private;
@@ -53,4 +57,15 @@
53}57}
54}58}
5559
60Q_DECL_EXPORT
61QDBusArgument &operator<<(QDBusArgument &argument,
62 const com::ubuntu::content::Item &item);
63
64Q_DECL_EXPORT
65const QDBusArgument &operator>>(const QDBusArgument &argument,
66 com::ubuntu::content::Item &item);
67
68Q_DECLARE_METATYPE(com::ubuntu::content::Item)
69
70
56#endif // COM_UBUNTU_CONTENT_ITEM_H_71#endif // COM_UBUNTU_CONTENT_ITEM_H_
5772
=== modified file 'src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml'
--- src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml 2014-04-03 12:00:41 +0000
+++ src/com/ubuntu/content/detail/com.ubuntu.content.Transfer.xml 2014-07-15 14:31:29 +0000
@@ -20,10 +20,10 @@
20 <method name="Download">20 <method name="Download">
21 </method>21 </method>
22 <method name="Charge">22 <method name="Charge">
23 <arg name="items" type="as" direction="in" />23 <arg name="items" type="av" direction="in" />
24 </method>24 </method>
25 <method name="Collect">25 <method name="Collect">
26 <arg name="items" type="as" direction="out" />26 <arg name="items" type="av" direction="out" />
27 </method>27 </method>
28 <method name="Store">28 <method name="Store">
29 <arg name="uri" type="s" direction="out" />29 <arg name="uri" type="s" direction="out" />
3030
=== modified file 'src/com/ubuntu/content/detail/service.cpp'
--- src/com/ubuntu/content/detail/service.cpp 2014-07-07 12:39:38 +0000
+++ src/com/ubuntu/content/detail/service.cpp 2014-07-15 14:31:29 +0000
@@ -32,6 +32,7 @@
32#include <unistd.h>32#include <unistd.h>
33#include <libnotify/notify.h>33#include <libnotify/notify.h>
3434
35#include <com/ubuntu/content/item.h>
35#include <com/ubuntu/content/peer.h>36#include <com/ubuntu/content/peer.h>
36#include <com/ubuntu/content/type.h>37#include <com/ubuntu/content/type.h>
37#include <com/ubuntu/content/transfer.h>38#include <com/ubuntu/content/transfer.h>
@@ -92,6 +93,7 @@
92 assert(!peer_registry.isNull());93 assert(!peer_registry.isNull());
9394
94 qDBusRegisterMetaType<cuc::Peer>();95 qDBusRegisterMetaType<cuc::Peer>();
96 qDBusRegisterMetaType<cuc::Item>();
9597
96 m_watcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);98 m_watcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
97 m_watcher->setConnection(d->connection);99 m_watcher->setConnection(d->connection);
@@ -202,7 +204,7 @@
202 Q_UNUSED(action);204 Q_UNUSED(action);
203205
204 cucd::Transfer* t = (cucd::Transfer*)data;206 cucd::Transfer* t = (cucd::Transfer*)data;
205 t->Charge(QStringList());207 t->Charge(QVariantList());
206}208}
207209
208void download_notify (cucd::Transfer* t)210void download_notify (cucd::Transfer* t)
@@ -571,7 +573,7 @@
571 {573 {
572 TRACE() << Q_FUNC_INFO << "Found downloaded import, charging";574 TRACE() << Q_FUNC_INFO << "Found downloaded import, charging";
573 if (r->handler->isValid())575 if (r->handler->isValid())
574 t->Charge(QStringList());576 t->Charge(QVariantList());
575 }577 }
576 }578 }
577 }579 }
@@ -588,7 +590,7 @@
588 if (t->Direction() == cuc::Transfer::Export)590 if (t->Direction() == cuc::Transfer::Export)
589 {591 {
590 TRACE() << Q_FUNC_INFO << "Found downloaded import, charging";592 TRACE() << Q_FUNC_INFO << "Found downloaded import, charging";
591 t->Charge(QStringList());593 t->Charge(QVariantList());
592 }594 }
593 }595 }
594 }596 }
595597
=== modified file 'src/com/ubuntu/content/detail/transfer.cpp'
--- src/com/ubuntu/content/detail/transfer.cpp 2014-05-28 10:09:29 +0000
+++ src/com/ubuntu/content/detail/transfer.cpp 2014-07-15 14:31:29 +0000
@@ -52,7 +52,7 @@
52 int direction;52 int direction;
53 QString store;53 QString store;
54 int selection_type;54 int selection_type;
55 QStringList items;55 QVariantList items;
56 bool source_started_by_content_hub;56 bool source_started_by_content_hub;
57 QString download_id;57 QString download_id;
58};58};
@@ -141,7 +141,7 @@
141 Q_EMIT(StateChanged(d->state));141 Q_EMIT(StateChanged(d->state));
142}142}
143143
144void cucd::Transfer::Charge(const QStringList& items)144void cucd::Transfer::Charge(const QVariantList& items)
145{145{
146 TRACE() << __PRETTY_FUNCTION__;146 TRACE() << __PRETTY_FUNCTION__;
147147
@@ -161,12 +161,14 @@
161 return;161 return;
162 } 162 }
163163
164 QStringList ret;164 QVariantList ret;
165 Q_FOREACH(QString i, items)165 Q_FOREACH(QVariant iv, items) {
166 ret.append(copy_to_store(i, d->store));166 cuc::Item origItem = qdbus_cast<Item>(iv);
167167 cuc::Item copiedItem = cuc::Item{QUrl(copy_to_store(origItem.url().toString(), d->store))};
168 Q_FOREACH(QString f, ret)168 copiedItem.setName(origItem.name());
169 TRACE() << Q_FUNC_INFO << "Item:" << f;169 TRACE() << Q_FUNC_INFO << "Item:" << copiedItem.url();
170 ret.append(QVariant::fromValue(copiedItem));
171 }
170172
171 if (ret.count() <= 0)173 if (ret.count() <= 0)
172 {174 {
@@ -211,7 +213,8 @@
211void cucd::Transfer::DownloadComplete(QString destFilePath)213void cucd::Transfer::DownloadComplete(QString destFilePath)
212{214{
213 TRACE() << __PRETTY_FUNCTION__;215 TRACE() << __PRETTY_FUNCTION__;
214 d->items.append(QUrl::fromLocalFile(destFilePath).toString());216 cuc::Item item = cuc::Item{QUrl::fromLocalFile(destFilePath).toString()};
217 d->items.append(QVariant::fromValue(item));
215 d->state = cuc::Transfer::downloaded;218 d->state = cuc::Transfer::downloaded;
216 Q_EMIT(StateChanged(d->state));219 Q_EMIT(StateChanged(d->state));
217}220}
@@ -226,7 +229,7 @@
226 Q_EMIT(StateChanged(d->state));229 Q_EMIT(StateChanged(d->state));
227}230}
228231
229QStringList cucd::Transfer::Collect()232QVariantList cucd::Transfer::Collect()
230{233{
231 TRACE() << __PRETTY_FUNCTION__;234 TRACE() << __PRETTY_FUNCTION__;
232235
233236
=== modified file 'src/com/ubuntu/content/detail/transfer.h'
--- src/com/ubuntu/content/detail/transfer.h 2014-05-22 09:11:23 +0000
+++ src/com/ubuntu/content/detail/transfer.h 2014-07-15 14:31:29 +0000
@@ -63,8 +63,8 @@
63 int State();63 int State();
64 void Start();64 void Start();
65 void Handled();65 void Handled();
66 void Charge(const QStringList&);66 void Charge(const QVariantList&);
67 QStringList Collect();67 QVariantList Collect();
68 void Abort();68 void Abort();
69 void Finalize();69 void Finalize();
70 QString Store();70 QString Store();
7171
=== modified file 'src/com/ubuntu/content/hub.cpp'
--- src/com/ubuntu/content/hub.cpp 2014-05-30 17:55:50 +0000
+++ src/com/ubuntu/content/hub.cpp 2014-07-15 14:31:29 +0000
@@ -63,6 +63,8 @@
63 setLoggingLevel(value);63 setLoggingLevel(value);
64 }64 }
6565
66 qDBusRegisterMetaType<cuc::Item>();
67
66 if (qApp)68 if (qApp)
67 qApp->installEventFilter(this);69 qApp->installEventFilter(this);
68}70}
6971
=== modified file 'src/com/ubuntu/content/item.cpp'
--- src/com/ubuntu/content/item.cpp 2013-07-16 11:04:48 +0000
+++ src/com/ubuntu/content/item.cpp 2014-07-15 14:31:29 +0000
@@ -16,21 +16,25 @@
16 * Authored by: Thomas Voß <thomas.voss@canonical.com>16 * Authored by: Thomas Voß <thomas.voss@canonical.com>
17 */17 */
1818
19#include <QDBusArgument>
20
19#include <com/ubuntu/content/item.h>21#include <com/ubuntu/content/item.h>
22#include "debug.h"
2023
21namespace cuc = com::ubuntu::content;24namespace cuc = com::ubuntu::content;
2225
23struct cuc::Item::Private26struct cuc::Item::Private
24{27{
25 QUrl url;28 QUrl url;
29 QString name;
2630
27 bool operator==(const Private& rhs) const31 bool operator==(const Private& rhs) const
28 {32 {
29 return url == rhs.url;33 return url == rhs.url && name == rhs.name;
30 }34 }
31};35};
3236
33cuc::Item::Item(const QUrl& url, QObject* parent) : QObject(parent), d{new cuc::Item::Private{url}}37cuc::Item::Item(const QUrl& url, QObject* parent) : QObject(parent), d{new cuc::Item::Private{url, QString()}}
34{38{
35}39}
3640
@@ -60,3 +64,39 @@
60{64{
61 return d->url;65 return d->url;
62}66}
67
68const QString& cuc::Item::name() const
69{
70 return d->name;
71}
72
73void cuc::Item::setName(const QString& newName) const
74{
75 if (newName != d->name)
76 d->name = newName;
77}
78
79QDBusArgument &operator<<(QDBusArgument &argument, const cuc::Item& item)
80{
81 argument.beginStructure();
82 argument << item.name() << item.url().toDisplayString();
83 argument.endStructure();
84 return argument;
85}
86
87const QDBusArgument &operator>>(const QDBusArgument &argument, cuc::Item &item)
88{
89 TRACE() << Q_FUNC_INFO;
90 QString name;
91 QString urlString;
92
93
94 argument.beginStructure();
95 argument >> name >> urlString;
96 argument.endStructure();
97
98 item = cuc::Item{QUrl(urlString)};
99 item.setName(name);
100 return argument;
101}
102
63103
=== modified file 'src/com/ubuntu/content/service/main.cpp'
--- src/com/ubuntu/content/service/main.cpp 2014-05-01 17:03:40 +0000
+++ src/com/ubuntu/content/service/main.cpp 2014-07-15 14:31:29 +0000
@@ -19,6 +19,7 @@
19#include <QCoreApplication>19#include <QCoreApplication>
20#include <QProcessEnvironment>20#include <QProcessEnvironment>
21#include <csignal>21#include <csignal>
22#include <com/ubuntu/content/item.h>
2223
23#include "detail/app_manager.h"24#include "detail/app_manager.h"
24#include "debug.h"25#include "debug.h"
2526
=== modified file 'src/com/ubuntu/content/transfer_p.h'
--- src/com/ubuntu/content/transfer_p.h 2014-04-03 12:00:41 +0000
+++ src/com/ubuntu/content/transfer_p.h 2014-07-15 14:31:29 +0000
@@ -106,13 +106,12 @@
106106
107 bool charge(const QVector<Item>& items)107 bool charge(const QVector<Item>& items)
108 {108 {
109 QStringList l;109 QVariantList itemVariants;
110 Q_FOREACH(const Item& item, items)110 Q_FOREACH(const Item& item, items)
111 {111 {
112 l << item.url().toDisplayString();112 itemVariants << QVariant::fromValue(item);
113 }113 }
114 114 auto reply = remote_transfer->Charge(itemVariants);
115 auto reply = remote_transfer->Charge(l);
116 reply.waitForFinished();115 reply.waitForFinished();
117116
118 return not reply.isError();117 return not reply.isError();
@@ -128,10 +127,13 @@
128 if (reply.isError())127 if (reply.isError())
129 return result;128 return result;
130129
131 Q_FOREACH(const QString& url, reply.value())130 auto items = reply.value();
132 {131
133 result << Item(QUrl(url));132 Q_FOREACH(const QVariant& itemVariant, items)
133 {
134 result << qdbus_cast<Item>(itemVariant);
134 }135 }
136
135 return result;137 return result;
136 }138 }
137139
138140
=== modified file 'tests/acceptance-tests/app_hub_communication_transfer.cpp'
--- tests/acceptance-tests/app_hub_communication_transfer.cpp 2014-02-21 02:30:11 +0000
+++ tests/acceptance-tests/app_hub_communication_transfer.cpp 2014-07-15 14:31:29 +0000
@@ -128,13 +128,19 @@
128 {128 {
129 QVector<cuc::Item> source_items;129 QVector<cuc::Item> source_items;
130 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file1").absoluteFilePath()));130 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file1").absoluteFilePath()));
131 source_items[0].setName("name1");
131 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file2").absoluteFilePath()));132 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file2").absoluteFilePath()));
133 source_items[1].setName("name2");
132 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file3").absoluteFilePath()));134 source_items << cuc::Item(QUrl::fromLocalFile(QFileInfo("file3").absoluteFilePath()));
135 source_items[2].setName("name3");
133 136
134 QVector<cuc::Item> expected_items;137 QVector<cuc::Item> expected_items;
135 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file1"));138 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file1"));
139 expected_items[0].setName("name1");
136 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file2"));140 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file2"));
141 expected_items[1].setName("name2");
137 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file3"));142 expected_items << cuc::Item(QUrl("file:///tmp/Incoming/file3"));
143 expected_items[2].setName("name3");
138144
139 /** [Importing pictures] */145 /** [Importing pictures] */
140 auto hub = cuc::Hub::Client::instance();146 auto hub = cuc::Hub::Client::instance();
141147
=== modified file 'tests/qml-tests/tst_ContentHub.qml'
--- tests/qml-tests/tst_ContentHub.qml 2014-03-07 21:03:36 +0000
+++ tests/qml-tests/tst_ContentHub.qml 2014-07-15 14:31:29 +0000
@@ -34,12 +34,13 @@
34 function test_export_request() {34 function test_export_request() {
35 var filePath = "file:///foo/bar.png";35 var filePath = "file:///foo/bar.png";
36 var transfer = destPeer.request();36 var transfer = destPeer.request();
37 transfer.items = [ resultComponent.createObject(test, {"url": filePath}) ];37 transfer.items = [ resultComponent.createObject(test, {"url": filePath, "name": "test"}) ];
38 transfer.state = ContentTransfer.Charged;38 transfer.state = ContentTransfer.Charged;
39 // This shouldn't be necessary, but without it we compare the results to fast39 // This shouldn't be necessary, but without it we compare the results to fast
40 ContentHub.exportRequested(transfer);40 ContentHub.exportRequested(transfer);
41 compare(test.exportTransfer, transfer, "Transfer object not correcty set");41 compare(test.exportTransfer, transfer, "Transfer object not correcty set");
42 compare(test.exportTransfer.items[0].url, filePath, "Transfer contents incorrect");42 compare(test.exportTransfer.items[0].url, filePath, "Transfer contents incorrect");
43 compare(test.exportTransfer.items[0].name, "test", "Transfer name incorrect");
43 }44 }
4445
45 Component {46 Component {

Subscribers

People subscribed via source and target branches