Merge lp:~dandrader/unity8/switchWindowFocus-lp1431325 into lp:unity8

Proposed by Daniel d'Andrada
Status: Merged
Approved by: Michael Zanetti
Approved revision: 1664
Merged at revision: 1688
Proposed branch: lp:~dandrader/unity8/switchWindowFocus-lp1431325
Merge into: lp:unity8
Diff against target: 528 lines (+278/-14)
11 files modified
plugins/Utils/windowstatestorage.cpp (+22/-1)
plugins/Utils/windowstatestorage.h (+4/-0)
qml/Stages/DecoratedWindow.qml (+5/-4)
qml/Stages/DesktopStage.qml (+15/-3)
qml/Stages/WindowDecoration.qml (+1/-0)
qml/Stages/WindowMoveResizeArea.qml (+5/-3)
tests/mocks/Unity/Application/ApplicationManager.h (+3/-1)
tests/qmltests/CMakeLists.txt (+1/-0)
tests/qmltests/Stages/ApplicationCheckBox.qml (+20/-1)
tests/qmltests/Stages/tst_DesktopStage.qml (+201/-0)
tests/qmltests/Stages/tst_PhoneStage.qml (+1/-1)
To merge this branch: bzr merge lp:~dandrader/unity8/switchWindowFocus-lp1431325
Reviewer Review Type Date Requested Status
Michael Zanetti (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+252942@code.launchpad.net

Commit message

DesktopStage - fix focus switch when user taps on window

Also fixes a threading issue in WindowStateStorage where
async SQL queries could be run when WindowStateStorage was
already destroyed.

Description of the change

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

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* Did you make sure that your branch does not contain spurious tags?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

* If you changed the UI, has there been a design review?
Not applicable.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Zanetti (mzanetti) :
review: Needs Fixing
1663. By Daniel d'Andrada

Ensure saveGeometry finishes before the thread it created actually starts to run

1664. By Daniel d'Andrada

Some improvements:

- Make ApplicationManagaer.availableApplications a property instead of a
  function, so it fits better with declarative (QML) code
- List all available applications tst_DesktopStage
- Refactor some function naming

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Replied to all inline comments

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Zanetti (mzanetti) wrote :

 * Did you perform an exploratory manual test run of the code change and any related functionality?

yes

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

no. but it doesn't look related

 * Did you make sure that the branch does not contain spurious tags?

yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/Utils/windowstatestorage.cpp'
2--- plugins/Utils/windowstatestorage.cpp 2015-02-11 10:54:50 +0000
3+++ plugins/Utils/windowstatestorage.cpp 2015-03-16 14:12:50 +0000
4@@ -18,6 +18,7 @@
5
6 #include <QtConcurrent>
7 #include <QDebug>
8+#include <QFutureSynchronizer>
9 #include <QSqlQuery>
10 #include <QSqlError>
11 #include <QSqlResult>
12@@ -36,8 +37,20 @@
13 initdb();
14 }
15
16+WindowStateStorage::~WindowStateStorage()
17+{
18+ QFutureSynchronizer<void> futureSync;
19+ for (int i = 0; i < m_asyncQueries.count(); ++i) {
20+ futureSync.addFuture(m_asyncQueries[i]);
21+ }
22+ futureSync.waitForFinished();
23+ m_db.close();
24+}
25+
26 void WindowStateStorage::saveGeometry(const QString &windowId, const QRect &rect)
27 {
28+ QMutexLocker mutexLocker(&s_mutex);
29+
30 QString queryString = QString("INSERT OR REPLACE INTO geometry (windowId, x, y, width, height) values ('%1', '%2', '%3', '%4', '%5');")
31 .arg(windowId)
32 .arg(rect.x())
33@@ -45,7 +58,15 @@
34 .arg(rect.width())
35 .arg(rect.height());
36
37- QtConcurrent::run(executeAsyncQuery, queryString);
38+ QFuture<void> future = QtConcurrent::run(executeAsyncQuery, queryString);
39+ m_asyncQueries.append(future);
40+
41+ QFutureWatcher<void> *futureWatcher = new QFutureWatcher<void>();
42+ futureWatcher->setFuture(future);
43+ connect(futureWatcher, &QFutureWatcher<void>::finished,
44+ this,
45+ [=](){ m_asyncQueries.removeAll(futureWatcher->future());
46+ futureWatcher->deleteLater(); });
47 }
48
49 void WindowStateStorage::executeAsyncQuery(const QString &queryString)
50
51=== modified file 'plugins/Utils/windowstatestorage.h'
52--- plugins/Utils/windowstatestorage.h 2015-02-10 17:03:11 +0000
53+++ plugins/Utils/windowstatestorage.h 2015-03-16 14:12:50 +0000
54@@ -17,12 +17,14 @@
55 #include <QObject>
56 #include <QSqlDatabase>
57 #include <QMutex>
58+#include <QFuture>
59
60 class WindowStateStorage: public QObject
61 {
62 Q_OBJECT
63 public:
64 WindowStateStorage(QObject *parent = 0);
65+ virtual ~WindowStateStorage();
66
67 Q_INVOKABLE void saveGeometry(const QString &windowId, const QRect &rect);
68 Q_INVOKABLE QRect getGeometry(const QString &windowId, const QRect &defaultValue);
69@@ -35,4 +37,6 @@
70
71 // NB: This is accessed from threads. Make sure to mutex it.
72 QSqlDatabase m_db;
73+
74+ QList< QFuture<void> > m_asyncQueries;
75 };
76
77=== modified file 'qml/Stages/DecoratedWindow.qml'
78--- qml/Stages/DecoratedWindow.qml 2014-11-20 15:58:59 +0000
79+++ qml/Stages/DecoratedWindow.qml 2015-03-16 14:12:50 +0000
80@@ -1,5 +1,5 @@
81 /*
82- * Copyright (C) 2014 Canonical, Ltd.
83+ * Copyright (C) 2014-2015 Canonical, Ltd.
84 *
85 * This program is free software; you can redistribute it and/or modify
86 * it under the terms of the GNU General Public License as published by
87@@ -20,13 +20,12 @@
88 import Ubuntu.Components 1.1
89 import Unity.Application 0.1
90
91-Item {
92+FocusScope {
93 id: root
94
95 property alias application: applicationWindow.application
96 property alias active: decoration.active
97
98- signal requestFocus();
99 signal close();
100 signal maximize();
101 signal minimize();
102@@ -53,8 +52,10 @@
103
104 ApplicationWindow {
105 id: applicationWindow
106+ objectName: application ? "appWindow_" + application.appId : "appWindow_null"
107 anchors.fill: parent
108 anchors.topMargin: units.gu(3)
109- interactive: index == 0
110+ interactive: true
111+ focus: true
112 }
113 }
114
115=== modified file 'qml/Stages/DesktopStage.qml'
116--- qml/Stages/DesktopStage.qml 2015-01-28 14:15:25 +0000
117+++ qml/Stages/DesktopStage.qml 2015-03-16 14:12:50 +0000
118@@ -1,5 +1,5 @@
119 /*
120- * Copyright (C) 2014 Canonical, Ltd.
121+ * Copyright (C) 2014-2015 Canonical, Ltd.
122 *
123 * This program is free software; you can redistribute it and/or modify
124 * it under the terms of the GNU General Public License as published by
125@@ -20,14 +20,17 @@
126 import Ubuntu.Components 1.1
127 import Unity.Application 0.1
128 import "../Components/PanelState"
129+import Utils 0.1
130
131-Item {
132+FocusScope {
133 id: root
134
135 anchors.fill: parent
136
137 property alias background: wallpaper.source
138
139+ property var windowStateStorage: WindowStateStorage
140+
141 CrossFadeImage {
142 id: wallpaper
143 anchors.fill: parent
144@@ -116,20 +119,29 @@
145 ]
146
147 WindowMoveResizeArea {
148+ windowStateStorage: root.windowStateStorage
149 target: appDelegate
150 minWidth: appDelegate.minWidth
151 minHeight: appDelegate.minHeight
152 resizeHandleWidth: units.gu(0.5)
153 windowId: model.appId // FIXME: Change this to point to windowId once we have such a thing
154
155- onPressed: ApplicationManager.requestFocusApplication(model.appId)
156+ onPressed: decoratedWindow.focus = true;
157 }
158
159 DecoratedWindow {
160+ id: decoratedWindow
161+ objectName: "decoratedWindow_" + appId
162 anchors.fill: parent
163 application: ApplicationManager.get(index)
164 active: ApplicationManager.focusedApplicationId === model.appId
165
166+ onFocusChanged: {
167+ if (focus) {
168+ ApplicationManager.requestFocusApplication(model.appId);
169+ }
170+ }
171+
172 onClose: ApplicationManager.stopApplication(model.appId)
173 onMaximize: appDelegate.state = (appDelegate.state == "maximized" ? "normal" : "maximized")
174 onMinimize: appDelegate.state = "minimized"
175
176=== modified file 'qml/Stages/WindowDecoration.qml'
177--- qml/Stages/WindowDecoration.qml 2014-11-24 11:21:38 +0000
178+++ qml/Stages/WindowDecoration.qml 2015-03-16 14:12:50 +0000
179@@ -55,6 +55,7 @@
180
181 Label {
182 id: titleLabel
183+ objectName: "windowDecorationTitle"
184 color: "#DFDBD2"
185 height: parent.height
186 verticalAlignment: Text.AlignVCenter
187
188=== modified file 'qml/Stages/WindowMoveResizeArea.qml'
189--- qml/Stages/WindowMoveResizeArea.qml 2015-02-11 17:12:32 +0000
190+++ qml/Stages/WindowMoveResizeArea.qml 2015-03-16 14:12:50 +0000
191@@ -1,5 +1,5 @@
192 /*
193- * Copyright (C) 2014 Canonical, Ltd.
194+ * Copyright (C) 2014-2015 Canonical, Ltd.
195 *
196 * This program is free software; you can redistribute it and/or modify
197 * it under the terms of the GNU General Public License as published by
198@@ -25,6 +25,8 @@
199 anchors.fill: target
200 anchors.margins: -resizeHandleWidth
201
202+ property var windowStateStorage: WindowStateStorage
203+
204 // The target item managed by this. Must be a parent or a sibling
205 // The area will anchor to it and manage move and resize events
206 property Item target: null
207@@ -48,7 +50,7 @@
208 }
209
210 Component.onCompleted: {
211- var windowState = WindowStateStorage.getGeometry(root.windowId, Qt.rect(target.x, target.y, target.width, target.height))
212+ var windowState = windowStateStorage.getGeometry(root.windowId, Qt.rect(target.x, target.y, target.width, target.height))
213 if (windowState !== undefined) {
214 target.x = windowState.x
215 target.y = windowState.y
216@@ -102,6 +104,6 @@
217 }
218
219 Component.onDestruction: {
220- WindowStateStorage.saveGeometry(root.windowId, Qt.rect(target.x, target.y, target.width, target.height))
221+ windowStateStorage.saveGeometry(root.windowId, Qt.rect(target.x, target.y, target.width, target.height))
222 }
223 }
224
225=== modified file 'tests/mocks/Unity/Application/ApplicationManager.h'
226--- tests/mocks/Unity/Application/ApplicationManager.h 2015-02-20 19:37:30 +0000
227+++ tests/mocks/Unity/Application/ApplicationManager.h 2015-03-16 14:12:50 +0000
228@@ -34,6 +34,7 @@
229 Q_FLAGS(ExecFlags)
230
231 Q_PROPERTY(bool empty READ isEmpty NOTIFY emptyChanged)
232+ Q_PROPERTY(QStringList availableApplications READ availableApplications NOTIFY availableApplicationsChanged)
233
234 public:
235 ApplicationManager(QObject *parent = nullptr);
236@@ -75,7 +76,7 @@
237 void setForceDashActive(bool forceDashActive) override;
238
239 // Only for testing
240- Q_INVOKABLE QStringList availableApplications();
241+ QStringList availableApplications();
242 Q_INVOKABLE ApplicationInfo* add(QString appId);
243
244 QModelIndex findIndex(ApplicationInfo* application);
245@@ -85,6 +86,7 @@
246 Q_SIGNALS:
247 void focusRequested(const QString &appId);
248 void emptyChanged(bool empty);
249+ void availableApplicationsChanged(QStringList list);
250
251 private Q_SLOTS:
252 void onWindowCreatedTimerTimeout();
253
254=== modified file 'tests/qmltests/CMakeLists.txt'
255--- tests/qmltests/CMakeLists.txt 2015-03-02 12:10:22 +0000
256+++ tests/qmltests/CMakeLists.txt 2015-03-16 14:12:50 +0000
257@@ -85,6 +85,7 @@
258 add_qml_test(Panel/Indicators MenuItemFactory IMPORT_PATHS ${CMAKE_BINARY_DIR}/tests/mocks ${qmltest_DEFAULT_IMPORT_PATHS})
259 add_qml_test(Panel/Indicators MessageMenuItemFactory IMPORT_PATHS ${CMAKE_BINARY_DIR}/tests/mocks ${qmltest_DEFAULT_IMPORT_PATHS})
260 add_qml_test(Stages ApplicationWindow)
261+add_qml_test(Stages DesktopStage)
262 add_qml_test(Stages PhoneStage ENVIRONMENT)
263 add_qml_test(Stages SpreadDelegate ENVIRONMENT)
264 add_qml_test(Stages SurfaceContainer ENVIRONMENT)
265
266=== modified file 'tests/qmltests/Stages/ApplicationCheckBox.qml'
267--- tests/qmltests/Stages/ApplicationCheckBox.qml 2015-01-28 12:59:21 +0000
268+++ tests/qmltests/Stages/ApplicationCheckBox.qml 2015-03-16 14:12:50 +0000
269@@ -24,15 +24,23 @@
270 property string appId
271 property alias checked: checkbox.checked
272
273+ enabled: appId !== "unity8-dash"
274+
275 Layout.fillWidth: true
276 CheckBox {
277 id: checkbox
278 checked: false
279 activeFocusOnPress: false
280+ property bool noop: false
281 onCheckedChanged: {
282+ if (noop) {
283+ return;
284+ }
285 if (checked) {
286- ApplicationManager.startApplication(root.appId);
287+ var application = ApplicationManager.startApplication(root.appId);
288+ appConnections.target = application;
289 } else {
290+ appConnections.target = null;
291 ApplicationManager.stopApplication(root.appId);
292 }
293 }
294@@ -42,4 +50,15 @@
295 color: "white"
296 anchors.verticalCenter: parent.verticalCenter
297 }
298+
299+ Connections {
300+ id: appConnections
301+ onStateChanged: {
302+ if (target.state == ApplicationInfoInterface.Stopped) {
303+ checkbox.noop = true;
304+ checkbox.checked = false;
305+ checkbox.noop = false;
306+ }
307+ }
308+ }
309 }
310
311=== added file 'tests/qmltests/Stages/tst_DesktopStage.qml'
312--- tests/qmltests/Stages/tst_DesktopStage.qml 1970-01-01 00:00:00 +0000
313+++ tests/qmltests/Stages/tst_DesktopStage.qml 2015-03-16 14:12:50 +0000
314@@ -0,0 +1,201 @@
315+/*
316+ * Copyright (C) 2015 Canonical, Ltd.
317+ *
318+ * This program is free software; you can redistribute it and/or modify
319+ * it under the terms of the GNU General Public License as published by
320+ * the Free Software Foundation; version 3.
321+ *
322+ * This program is distributed in the hope that it will be useful,
323+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
324+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
325+ * GNU General Public License for more details.
326+ *
327+ * You should have received a copy of the GNU General Public License
328+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
329+ */
330+
331+import QtQuick 2.0
332+import QtTest 1.0
333+import Ubuntu.Components 1.1
334+import Ubuntu.Components.ListItems 1.0 as ListItem
335+import Unity.Application 0.1
336+import Unity.Test 0.1
337+
338+import "../../../qml/Stages"
339+
340+Rectangle {
341+ id: root
342+ color: "darkblue"
343+ width: desktopStageLoader.width + controls.width
344+ height: desktopStageLoader.height
345+
346+ QtObject {
347+ id: fakeWindowStateStorage
348+
349+ property var storedGeom: [
350+ ["unity8-dash", Qt.rect(units.gu(2), units.gu(2), units.gu(50), units.gu(50))],
351+ ["webbrowser-app", Qt.rect(units.gu(60), units.gu(2), units.gu(50), units.gu(50))]
352+ ]
353+
354+ function saveGeometry(windowId, geometry) {
355+ for (var i = 0; i < storedGeom.length; ++i) {
356+ if (storedGeom[i][0] === windowId) {
357+ storedGeom[i][1] = geometry;
358+ return;
359+ }
360+ }
361+ // if new
362+ storedGeom[storedGeom.length] = [windowId, geometry];
363+ }
364+ function getGeometry(windowId, defaultGeometry) {
365+ for (var i = 0; i < storedGeom.length; ++i) {
366+ if (storedGeom[i][0] === windowId) {
367+ return storedGeom[i][1];
368+ }
369+ }
370+ return defaultGeometry;
371+ }
372+ }
373+
374+ Loader {
375+ id: desktopStageLoader
376+ x: ((root.width - controls.width) - width) / 2
377+ y: (root.height - height) / 2
378+ width: units.gu(160*0.9)
379+ height: units.gu(100*0.9)
380+
381+ focus: true
382+
383+ property bool itemDestroyed: false
384+ sourceComponent: Component {
385+ DesktopStage {
386+ anchors.fill: parent
387+ windowStateStorage: fakeWindowStateStorage
388+ Component.onDestruction: {
389+ desktopStageLoader.itemDestroyed = true;
390+ }
391+ focus: true
392+ }
393+ }
394+ }
395+
396+ Rectangle {
397+ id: controls
398+ color: "darkgrey"
399+ width: units.gu(30)
400+ anchors {
401+ top: parent.top
402+ bottom: parent.bottom
403+ right: parent.right
404+ }
405+
406+ Column {
407+ anchors { left: parent.left; right: parent.right; top: parent.top; margins: units.gu(1) }
408+ spacing: units.gu(1)
409+ Repeater {
410+ model: ApplicationManager.availableApplications
411+ ApplicationCheckBox {
412+ appId: modelData
413+ }
414+ }
415+ }
416+ }
417+
418+ UnityTestCase {
419+ id: testCase
420+ name: "DesktopStage"
421+ when: windowShown
422+
423+ property Item desktopStage: desktopStageLoader.status === Loader.Ready ? desktopStageLoader.item : null
424+
425+ function init() {
426+ }
427+
428+ function cleanup() {
429+ desktopStageLoader.itemDestroyed = false;
430+ desktopStageLoader.active = false;
431+
432+ tryCompare(desktopStageLoader, "status", Loader.Null);
433+ tryCompare(desktopStageLoader, "item", null);
434+ // Loader.status might be Loader.Null and Loader.item might be null but the Loader
435+ // actually took place. Likely because Loader waits until the next event loop
436+ // iteration to do its work. So to ensure the reload, we will wait until the
437+ // Shell instance gets destroyed.
438+ tryCompare(desktopStageLoader, "itemDestroyed", true);
439+
440+ killAllRunningApps();
441+
442+ desktopStageLoader.active = true;
443+ tryCompare(desktopStageLoader, "status", Loader.Ready);
444+ }
445+
446+ function killAllRunningApps() {
447+ while (ApplicationManager.count > 1) {
448+ var appIndex = ApplicationManager.get(0).appId == "unity8-dash" ? 1 : 0
449+ ApplicationManager.stopApplication(ApplicationManager.get(appIndex).appId);
450+ }
451+ compare(ApplicationManager.count, 1)
452+ }
453+
454+ function waitUntilAppSurfaceShowsUp(appId) {
455+ var appWindow = findChild(desktopStage, "appWindow_" + appId);
456+ verify(appWindow);
457+ var appWindowStates = findInvisibleChild(appWindow, "applicationWindowStateGroup");
458+ verify(appWindowStates);
459+ tryCompare(appWindowStates, "state", "surface");
460+ }
461+
462+ function rectsIntersect(aLocal, bLocal) {
463+
464+ var a = aLocal.mapToItem(null, 0, 0, aLocal.width, aLocal.height);
465+ var b = bLocal.mapToItem(null, 0, 0, bLocal.width, bLocal.height);
466+
467+ return !((a.y+a.height) < b.y
468+ || a.y > (b.y+b.height)
469+ || (a.x+a.width) < b.x
470+ || a.x > (b.x+b.width));
471+ }
472+
473+ function test_tappingOnWindowChangesFocusedApp() {
474+ ApplicationManager.startApplication("webbrowser-app");
475+ waitUntilAppSurfaceShowsUp("webbrowser-app");
476+
477+ var webbrowserWindow = findChild(desktopStage, "appWindow_webbrowser-app");
478+ verify(webbrowserWindow);
479+ var dashWindow = findChild(desktopStage, "appWindow_unity8-dash");
480+ verify(dashWindow);
481+
482+ // some sanity check
483+ compare(rectsIntersect(dashWindow, webbrowserWindow), false);
484+
485+ tap(dashWindow);
486+ compare(dashWindow.application.session.surface.activeFocus, true);
487+
488+ tap(webbrowserWindow);
489+ compare(webbrowserWindow.application.session.surface.activeFocus, true);
490+ }
491+
492+ function test_tappingOnWindowTitleChangesFocusedApp() {
493+ ApplicationManager.startApplication("webbrowser-app");
494+ waitUntilAppSurfaceShowsUp("webbrowser-app");
495+
496+ var webbrowserWindow = findChild(desktopStage, "decoratedWindow_webbrowser-app");
497+ verify(webbrowserWindow);
498+ var webbrowserWindowTitle = findChild(webbrowserWindow, "windowDecorationTitle");
499+ verify(webbrowserWindowTitle);
500+ var dashWindow = findChild(desktopStage, "decoratedWindow_unity8-dash");
501+ verify(dashWindow);
502+ var dashWindowTitle = findChild(dashWindow, "windowDecorationTitle");
503+ verify(dashWindowTitle);
504+
505+ // some sanity check
506+ compare(rectsIntersect(dashWindow, webbrowserWindow), false);
507+
508+ tap(dashWindowTitle);
509+ compare(dashWindow.application.session.surface.activeFocus, true);
510+
511+ tap(webbrowserWindowTitle);
512+ compare(webbrowserWindow.application.session.surface.activeFocus, true);
513+ }
514+ }
515+}
516
517=== modified file 'tests/qmltests/Stages/tst_PhoneStage.qml'
518--- tests/qmltests/Stages/tst_PhoneStage.qml 2015-01-28 12:59:21 +0000
519+++ tests/qmltests/Stages/tst_PhoneStage.qml 2015-03-16 14:12:50 +0000
520@@ -116,7 +116,7 @@
521 function addApps(count) {
522 if (count == undefined) count = 1;
523 for (var i = 0; i < count; i++) {
524- var app = ApplicationManager.startApplication(ApplicationManager.availableApplications()[ApplicationManager.count])
525+ var app = ApplicationManager.startApplication(ApplicationManager.availableApplications[ApplicationManager.count])
526 tryCompare(app, "state", ApplicationInfoInterface.Running)
527 var spreadView = findChild(phoneStage, "spreadView");
528 tryCompare(spreadView, "contentX", -spreadView.shift);

Subscribers

People subscribed via source and target branches