Ubuntu UI Toolkit

Merge lp:~lukas-kde/ubuntu-ui-toolkit/asyncDbusClipboard into lp:ubuntu-ui-toolkit/staging

  1. asyncDbusClipboard
  2. Merge into staging
Proposed by Lukáš Tinkl on 2017-02-10
Status: Merged
Approved by: Christian Dywan on 2017-02-12
Approved revision: 2172
Merged at revision: 2170
Proposed branch: lp:~lukas-kde/ubuntu-ui-toolkit/asyncDbusClipboard
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 323 lines (+79/-81)
3 files modified
src/UbuntuToolkit/privates/uccontenthub.cpp (+45/-45)
src/UbuntuToolkit/privates/uccontenthub_p.h (+17/-14)
tests/unit/contenthub/tst_contenthub.cpp (+17/-22)
To merge this branch: bzr merge lp:~lukas-kde/ubuntu-ui-toolkit/asyncDbusClipboard
Reviewer Review Type Date Requested Status
ubuntu-sdk-build-bot continuous-integration Approve on 2017-02-12
Christian Dywan 2017-02-10 Approve on 2017-02-12
Arthur Mello (community) 2017-02-10 Approve on 2017-02-10
Review via email: mp+316965@code.launchpad.net

Commit Message

Unbreak the startup race between unity8/qtmir and UITK trying talk to content-hub

Description of the Change

Unbreak the startup race between unity8/qtmir and UITK trying talk to content-hub

Do not initialize the UCContentHub class until the content hub service becomes available on DBUS; make the onPasteboardChanged() method async too, this is also blocking for ~25 seconds.

Fixes lp:1663106 - [regression] Logging in to Unity8 takes 25 seconds (the default DBus timeout)

To post a comment you must log in.
Christian Dywan (kalikiana) wrote :

Notes from discussing this on IRC:

<ltinkl> unity8/qtmir is starting up, we have a TextField somewhere in our GUI which contains the call to UCContentHub, calls content-hub over DBUS, but at the same time content-hub is asking qtmir for authorization
I assume content-hub somehow also queries the clipboard, but the implementation of QPlatformClipboard is in qtmir :) so we get a nice circular dependency and DBUS gets stuck, until one of the calls times out
he tricky (blocking) part is creating QDBusInterface in your CTOR, you don't even have to make any calls
<kalikiana> How about making the mock service sleep when it initializes or responds?
Maybe the mock service should actually just time out the first call?

ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:2170
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/900/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/1658/console
    FAILURE: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/1443/console
    FAILURE: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/1408/console
    FAILURE: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-yakkety/222/console
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7670/console
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7669
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7668
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7666
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7667

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/900/rebuild

review: Needs Fixing (continuous-integration)
Arthur Mello (artmello) wrote :

Changes looks good to me. Thx for working on that.

review: Approve
lp:~lukas-kde/ubuntu-ui-toolkit/asyncDbusClipboard updated on 2017-02-10
2171. By Lukáš Tinkl on 2017-02-10

if content-hub is already running, go and init directly

2172. By Lukáš Tinkl on 2017-02-10

fix the tests

do not wait needlessly, QTRY_COMPARE() already has a 5 second timeout

ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

PASSED: Continuous integration, rev:2172
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/901/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/1659
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/1444
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/1409
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-yakkety/223
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7675/console
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7673
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7672
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7671
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/7674

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/901/rebuild

review: Approve (continuous-integration)
Christian Dywan (kalikiana) wrote :

Nice. Thanks for fixing the tests!

review: Approve
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/UbuntuToolkit/privates/uccontenthub.cpp'
2--- src/UbuntuToolkit/privates/uccontenthub.cpp 2017-01-26 14:29:30 +0000
3+++ src/UbuntuToolkit/privates/uccontenthub.cpp 2017-02-10 18:15:36 +0000
4@@ -21,8 +21,10 @@
5 #include <QtCore/QLoggingCategory>
6 #include <QtCore/QMimeData>
7 #include <QtDBus/QDBusConnection>
8+#include <QtDBus/QDBusConnectionInterface>
9 #include <QtDBus/QDBusInterface>
10 #include <QtDBus/QDBusReply>
11+#include <QtDBus/QDBusServiceWatcher>
12 #include <QtQuick/QQuickItem>
13
14 Q_LOGGING_CATEGORY(ucContentHub, "ubuntu.components.UCContentHub", QtMsgType::QtWarningMsg)
15@@ -40,11 +42,17 @@
16 UT_NAMESPACE_BEGIN
17
18 UCContentHub::UCContentHub(QObject *parent)
19- : QObject(parent),
20- m_dbusIface(0),
21- m_contentHubIface(0),
22- m_canPaste(false),
23- m_targetItem(0)
24+ : QObject(parent)
25+{
26+ if (QDBusConnection::sessionBus().interface()->isServiceRegistered(contentHubService)) { // content hub already running
27+ init();
28+ } else {
29+ m_watcher = new QDBusServiceWatcher(contentHubService, QDBusConnection::sessionBus(), QDBusServiceWatcher::WatchForRegistration, this);
30+ connect(m_watcher, &QDBusServiceWatcher::serviceRegistered, this, &UCContentHub::init);
31+ }
32+}
33+
34+void UCContentHub::init()
35 {
36 m_dbusIface = new QDBusInterface(dbusService,
37 dbusObjectPath,
38@@ -76,24 +84,13 @@
39 SLOT(onPasteboardChanged())
40 );
41
42- m_canPaste = checkPasteFormats();
43- }
44-}
45-
46-UCContentHub::~UCContentHub()
47-{
48- if (m_dbusIface) {
49- delete m_dbusIface;
50- }
51-
52- if (m_contentHubIface) {
53- delete m_contentHubIface;
54+ onPasteboardChanged();
55 }
56 }
57
58 void UCContentHub::requestPaste(QQuickItem *targetItem)
59 {
60- if (!m_contentHubIface->isValid()) {
61+ if (!m_contentHubIface || !m_contentHubIface->isValid()) {
62 CONTENT_HUB_TRACE("Invalid Content Hub DBusInterface");
63 return;
64 }
65@@ -106,12 +103,12 @@
66 m_contentHubIface->call(QStringLiteral("RequestPasteByAppId"), appProfile);
67 }
68
69-bool UCContentHub::canPaste()
70+bool UCContentHub::canPaste() const
71 {
72 return m_canPaste;
73 }
74
75-void UCContentHub::onPasteSelected(QString appId, QByteArray mimedata, bool pasteAsRichText)
76+void UCContentHub::onPasteSelected(const QString &appId, const QByteArray &mimedata, bool pasteAsRichText)
77 {
78 if (getAppProfile() != appId) {
79 return;
80@@ -138,15 +135,39 @@
81
82 void UCContentHub::onPasteboardChanged()
83 {
84- if (checkPasteFormats() != m_canPaste) {
85- m_canPaste = !m_canPaste;
86+ if (!m_contentHubIface || !m_contentHubIface->isValid()) {
87+ CONTENT_HUB_TRACE("Invalid Content Hub DBusInterface");
88+ return;
89+ }
90+
91+ QDBusPendingCall pcall = m_contentHubIface->asyncCall(QStringLiteral("PasteFormats"));
92+ QDBusPendingCallWatcher * watcher = new QDBusPendingCallWatcher(pcall, this);
93+ connect(watcher, &QDBusPendingCallWatcher::finished, [this](QDBusPendingCallWatcher * call) {
94+ QDBusPendingReply<QStringList> reply = *call;
95+ call->deleteLater();
96+ if (reply.isValid()) {
97+ // TODO: ContentHub clipboard keeps a list of all available paste formats.
98+ // Probably apps could make use of this information to check if a specific
99+ // data type is available, instead of only checking if list is empty or not.
100+ // (LP: #1657111)
101+ setCanPaste(!reply.value().isEmpty());
102+ } else {
103+ CONTENT_HUB_TRACE("Invalid return from DBus call PasteFormats");
104+ }
105+ });
106+}
107+
108+void UCContentHub::setCanPaste(bool value)
109+{
110+ if (value != m_canPaste) {
111+ m_canPaste = value;
112 Q_EMIT canPasteChanged();
113 }
114 }
115
116-QString UCContentHub::getAppProfile()
117+QString UCContentHub::getAppProfile() const
118 {
119- if (!m_dbusIface->isValid()) {
120+ if (!m_dbusIface || !m_dbusIface->isValid()) {
121 CONTENT_HUB_TRACE("Invalid DBus DBusInterface");
122 return QString();
123 }
124@@ -194,25 +215,4 @@
125 return mimeData;
126 }
127
128-bool UCContentHub::checkPasteFormats()
129-{
130- if (!m_contentHubIface->isValid()) {
131- CONTENT_HUB_TRACE("Invalid Content Hub DBusInterface");
132- return false;
133- }
134-
135- QDBusReply<QStringList> reply = m_contentHubIface->call(QStringLiteral("PasteFormats"));
136- if (reply.isValid()) {
137- // TODO: ContentHub clipboard keeps a list of all available paste formats.
138- // Probably apps could make use of this information to check if a specific
139- // data type is available, instead of only checking if list is empty or not.
140- // (LP: #1657111)
141- return !reply.value().isEmpty();
142- } else {
143- CONTENT_HUB_TRACE("Invalid return from DBus call PasteFormats");
144- }
145-
146- return false;
147-}
148-
149 UT_NAMESPACE_END
150
151=== modified file 'src/UbuntuToolkit/privates/uccontenthub_p.h'
152--- src/UbuntuToolkit/privates/uccontenthub_p.h 2017-01-17 19:30:01 +0000
153+++ src/UbuntuToolkit/privates/uccontenthub_p.h 2017-02-10 18:15:36 +0000
154@@ -25,6 +25,7 @@
155
156 class QMimeData;
157 class QDBusInterface;
158+class QDBusServiceWatcher;
159 class QQuickItem;
160
161 UT_NAMESPACE_BEGIN
162@@ -32,35 +33,37 @@
163 class UBUNTUTOOLKIT_EXPORT UCContentHub : public QObject
164 {
165 Q_OBJECT
166-
167+ friend class tst_UCContentHub;
168 Q_PROPERTY(bool canPaste READ canPaste NOTIFY canPasteChanged)
169
170 public:
171- UCContentHub(QObject* parent = 0);
172- ~UCContentHub();
173+ UCContentHub(QObject* parent = nullptr);
174+ ~UCContentHub() = default;
175
176 Q_INVOKABLE void requestPaste(QQuickItem *targetItem);
177
178- bool canPaste();
179- QString getAppProfile();
180+ bool canPaste() const;
181+ QString getAppProfile() const;
182 QMimeData* deserializeMimeData(const QByteArray &serializedMimeData);
183
184 Q_SIGNALS:
185 void pasteSelected(QQuickItem *targetItem, const QString &data);
186 void canPasteChanged();
187
188-public Q_SLOTS:
189- void onPasteSelected(QString appId, QByteArray mimedata, bool pasteAsRichText);
190+private Q_SLOTS:
191+ void init();
192+ void onPasteSelected(const QString &appId, const QByteArray &mimedata, bool pasteAsRichText);
193 void onPasteboardChanged();
194
195 private:
196- bool checkPasteFormats();
197-
198- QDBusInterface *m_dbusIface;
199- QDBusInterface *m_contentHubIface;
200-
201- bool m_canPaste;
202- QQuickItem *m_targetItem;
203+ void setCanPaste(bool value);
204+ QDBusInterface *m_dbusIface{nullptr};
205+ QDBusInterface *m_contentHubIface{nullptr};
206+
207+ bool m_canPaste{false};
208+ QQuickItem *m_targetItem{nullptr};
209+
210+ QDBusServiceWatcher * m_watcher{nullptr};
211 };
212
213 UT_NAMESPACE_END
214
215=== modified file 'tests/unit/contenthub/tst_contenthub.cpp'
216--- tests/unit/contenthub/tst_contenthub.cpp 2017-01-26 16:02:48 +0000
217+++ tests/unit/contenthub/tst_contenthub.cpp 2017-02-10 18:15:36 +0000
218@@ -68,12 +68,10 @@
219 tst_UCContentHub() {}
220
221 private:
222- MockContentService *mockService;
223- UCContentHub *contentHub;
224- QSignalSpy *pasteRequestedSpy;
225- QSignalSpy *pasteSelectedSpy;
226-
227- const int testTimeout = 5000;
228+ MockContentService *mockService{nullptr};
229+ UCContentHub *contentHub{nullptr};
230+ QSignalSpy *pasteRequestedSpy{nullptr};
231+ QSignalSpy *pasteSelectedSpy{nullptr};
232
233 const QString dummyAppId = "DummyAppId";
234
235@@ -143,11 +141,11 @@
236 QDBusConnection connection = QDBusConnection::sessionBus();
237 connection.registerObject("/", mockService);
238 connection.registerService("com.ubuntu.content.dbus.Service");
239- pasteRequestedSpy = new QSignalSpy(mockService, SIGNAL(PasteRequested()));
240+ pasteRequestedSpy = new QSignalSpy(mockService, &MockContentService::PasteRequested);
241
242 qRegisterMetaType<QQuickItem*>();
243 contentHub = new UCContentHub();
244- pasteSelectedSpy = new QSignalSpy(contentHub, SIGNAL(pasteSelected(QQuickItem*, const QString&)));
245+ pasteSelectedSpy = new QSignalSpy(contentHub, &UCContentHub::pasteSelected);
246 }
247
248 void cleanupTestCase()
249@@ -155,6 +153,7 @@
250 delete pasteRequestedSpy;
251 delete pasteSelectedSpy;
252 delete contentHub;
253+ delete mockService;
254 }
255
256 void cleanup()
257@@ -187,8 +186,7 @@
258 QMimeData textPaste;
259 textPaste.setText(sampleHtml);
260 contentHub->onPasteSelected(contentHub->getAppProfile(), serializeMimeData(textPaste), false);
261- pasteSelectedSpy->wait(testTimeout);
262- QCOMPARE(pasteSelectedSpy->count(), 1);
263+ QTRY_COMPARE(pasteSelectedSpy->count(), 1);
264 QList<QVariant> args = pasteSelectedSpy->takeFirst();
265 QVERIFY(args.at(1).toString() == textPaste.text());
266 }
267@@ -198,8 +196,7 @@
268 QMimeData htmlPaste;
269 htmlPaste.setHtml(sampleHtml);
270 contentHub->onPasteSelected(contentHub->getAppProfile(), serializeMimeData(htmlPaste), false);
271- pasteSelectedSpy->wait(testTimeout);
272- QCOMPARE(pasteSelectedSpy->count(), 1);
273+ QTRY_COMPARE(pasteSelectedSpy->count(), 1);
274 QList<QVariant> args = pasteSelectedSpy->takeFirst();
275 QVERIFY(args.at(1).toString() == htmlPaste.text());
276 }
277@@ -209,8 +206,7 @@
278 QMimeData htmlPaste;
279 htmlPaste.setHtml(sampleHtml);
280 contentHub->onPasteSelected(contentHub->getAppProfile(), serializeMimeData(htmlPaste), true);
281- pasteSelectedSpy->wait(testTimeout);
282- QCOMPARE(pasteSelectedSpy->count(), 1);
283+ QTRY_COMPARE(pasteSelectedSpy->count(), 1);
284 QList<QVariant> args = pasteSelectedSpy->takeFirst();
285 QVERIFY(args.at(1).toString() == htmlPaste.html());
286 }
287@@ -220,8 +216,7 @@
288 QMimeData textPaste;
289 textPaste.setText(sampleText);
290 contentHub->onPasteSelected(dummyAppId, serializeMimeData(textPaste), false);
291- pasteSelectedSpy->wait(testTimeout);
292- QCOMPARE(pasteSelectedSpy->count(), 0);
293+ QTRY_COMPARE(pasteSelectedSpy->count(), 0);
294 }
295
296 void test_KeyboardShortcutOnTextField()
297@@ -230,10 +225,10 @@
298 testCase->rootObject()->forceActiveFocus();
299 QQuickItem *textField = testCase->findItem<QQuickItem*>("textField");
300 QTest::keyClick(textField->window(), Qt::Key_Tab);
301- QTRY_COMPARE_WITH_TIMEOUT(textField->property("activeFocus").toBool(), true, testTimeout);
302+ QTRY_COMPARE(textField->property("activeFocus").toBool(), true);
303+ QTRY_COMPARE(contentHub->canPaste(), true);
304 QTest::keyClick(textField->window(), Qt::Key_V, Qt::ControlModifier|Qt::ShiftModifier);
305- pasteRequestedSpy->wait(testTimeout);
306- QCOMPARE(pasteRequestedSpy->count(), 1);
307+ QTRY_COMPARE(pasteRequestedSpy->count(), 1);
308 }
309
310 void test_KeyboardShortcutOnTextArea()
311@@ -242,10 +237,10 @@
312 testCase->rootObject()->forceActiveFocus();
313 QQuickItem *textArea = testCase->findItem<QQuickItem*>("textArea");
314 QTest::keyClick(textArea->window(), Qt::Key_Tab);
315- QTRY_COMPARE_WITH_TIMEOUT(textArea->property("activeFocus").toBool(), true, testTimeout);
316+ QTRY_COMPARE(textArea->property("activeFocus").toBool(), true);
317+ QTRY_COMPARE(contentHub->canPaste(), true);
318 QTest::keyClick(textArea->window(), Qt::Key_V, Qt::ControlModifier|Qt::ShiftModifier);
319- pasteRequestedSpy->wait(testTimeout);
320- QCOMPARE(pasteRequestedSpy->count(), 1);
321+ QTRY_COMPARE(pasteRequestedSpy->count(), 1);
322 }
323 };
324

Subscribers

People subscribed via source and target branches