Merge lp:~unity-team/unity8/sidestage-fixes into lp:unity8
- sidestage-fixes
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Michał Sawicz |
Approved revision: | 2393 |
Merged at revision: | 2402 |
Proposed branch: | lp:~unity-team/unity8/sidestage-fixes |
Merge into: | lp:unity8 |
Prerequisite: | lp:~nick-dedekind/unity8/osk-offset |
Diff against target: |
264 lines (+94/-13) 7 files modified
qml/Stages/SurfaceContainer.qml (+3/-0) qml/Stages/TabletStage.qml (+4/-3) tests/mocks/Unity/Application/MirSurfaceItem.cpp (+16/-7) tests/mocks/Unity/Application/MirSurfaceItem.h (+1/-1) tests/mocks/Utils/windowstatestorage.cpp (+1/-0) tests/mocks/Utils/windowstatestorage.h (+3/-0) tests/qmltests/Stages/tst_TabletStage.qml (+66/-2) |
To merge this branch: | bzr merge lp:~unity-team/unity8/sidestage-fixes |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Daniel d'Andrada (community) | Approve | ||
Unity8 CI Bot | continuous-integration | Needs Fixing | |
Michał Sawicz | Approve | ||
Review via email:
|
This proposal supersedes a proposal from 2016-05-05.
Commit message
Fixed saving current stage and loss of surface focus when switching stages
Description of the change
* Are there any related MPs required for this MP to build/function as expected? Please list.
https:/
* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes
* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A
* If you changed the UI, has there been a design review?
N/A
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal | # |
In qml/Stages/
Instead of exposing surfaceItem.
-------
I see no regression test for that loss of active focus that you claim to have fixed.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal | # |
By the way, you're mixing two independent fixes on a single branch. That's not a good practice.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nick Dedekind (nick-dedekind) wrote : Posted in a previous version of this proposal | # |
> In qml/Stages/
>
> Instead of exposing surfaceItem.
> more consistent API) to tie it to the existing "interactive" property instead.
>
> -------
>
> I see no regression test for that loss of active focus that you claim to have
> fixed.
Added test.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:2389
https:/
Executed test runs:
UNSTABLE: https:/
UNSTABLE: https:/
UNSTABLE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal | # |
Excellent, thanks!
* 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 final results yet.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal | # |
Please check the test results - there's a bunch of failing QML tests.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nick Dedekind (nick-dedekind) wrote : Posted in a previous version of this proposal | # |
> Excellent, thanks!
>
> * 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 final results yet.
I've hat to revert the API change for exposing "consumesInput". Using the interactive property broke a bunch of the tests.
The tests are assuming the interactive property alone controls the activeFocus, where in qtmir and the mocks it's controlled by "consumesInput" as well.
I have fixed the mocks to handle the consumesInput change and added a FIXME to the code; but until it's fixed and released in qtmir we can't remove.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:2390
https:/
Executed test runs:
UNSTABLE: https:/
SUCCESS: https:/
UNSTABLE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal | # |
ACK
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:2392
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michał Sawicz (saviq) wrote : | # |
Approving as per superseded MP.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:2393
https:/
Executed test runs:
UNSTABLE: https:/
UNSTABLE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Daniel d'Andrada (dandrader) : | # |
Preview Diff
1 | === modified file 'qml/Stages/SurfaceContainer.qml' |
2 | --- qml/Stages/SurfaceContainer.qml 2016-05-17 19:25:23 +0000 |
3 | +++ qml/Stages/SurfaceContainer.qml 2016-05-18 22:09:41 +0000 |
4 | @@ -34,6 +34,9 @@ |
5 | property int surfaceOrientationAngle: 0 |
6 | property bool resizeSurface: true |
7 | property bool isPromptSurface: false |
8 | + // FIME - dont export, use interactive property. Need to fix qtmir to handle consumesInputChanged |
9 | + // to update surface activeFocus. See mock MirSurfaceItem. |
10 | + property alias consumesInput: surfaceItem.consumesInput |
11 | |
12 | onSurfaceChanged: { |
13 | // Not a binding because animations might remove the surface from the surfaceItem |
14 | |
15 | === modified file 'qml/Stages/TabletStage.qml' |
16 | --- qml/Stages/TabletStage.qml 2016-04-29 20:07:21 +0000 |
17 | +++ qml/Stages/TabletStage.qml 2016-05-18 22:09:41 +0000 |
18 | @@ -752,7 +752,7 @@ |
19 | |
20 | readonly property bool wantsMainStage: stage == ApplicationInfoInterface.MainStage |
21 | |
22 | - readonly property bool isDash: model.application.appId == "unity8-dash" |
23 | + readonly property bool isDash: application.appId == "unity8-dash" |
24 | |
25 | onFocusChanged: { |
26 | if (focus && !spreadRepeater.startingUp) { |
27 | @@ -768,7 +768,7 @@ |
28 | onFocusRequested: spreadTile.focus = true; |
29 | } |
30 | Connections { |
31 | - target: model.application |
32 | + target: spreadTile.application |
33 | onFocusRequested: { |
34 | if (!model.surface) { |
35 | // when an app has no surfaces, we assume there's only one entry representing it: |
36 | @@ -844,7 +844,7 @@ |
37 | _constructing = false; |
38 | } |
39 | Component.onDestruction: { |
40 | - WindowStateStorage.saveStage(model.application.appId, stage); |
41 | + WindowStateStorage.saveStage(application.appId, stage); |
42 | } |
43 | |
44 | function refreshStage() { |
45 | @@ -1073,6 +1073,7 @@ |
46 | |
47 | surface: spreadDelegate ? spreadDelegate.surface : null |
48 | |
49 | + consumesInput: false |
50 | interactive: false |
51 | resizeSurface: false |
52 | focus: false |
53 | |
54 | === modified file 'tests/mocks/Unity/Application/MirSurfaceItem.cpp' |
55 | --- tests/mocks/Unity/Application/MirSurfaceItem.cpp 2016-04-27 15:01:10 +0000 |
56 | +++ tests/mocks/Unity/Application/MirSurfaceItem.cpp 2016-05-18 22:09:41 +0000 |
57 | @@ -56,7 +56,11 @@ |
58 | Qt::ExtraButton9 | Qt::ExtraButton10 | Qt::ExtraButton11 | |
59 | Qt::ExtraButton12 | Qt::ExtraButton13); |
60 | |
61 | + connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceActiveFocus); |
62 | connect(this, &QQuickItem::visibleChanged, this, &MirSurfaceItem::updateMirSurfaceVisibility); |
63 | + connect(this, &MirSurfaceItem::consumesInputChanged, this, [this]() { |
64 | + updateMirSurfaceActiveFocus(hasActiveFocus()); |
65 | + }); |
66 | } |
67 | |
68 | MirSurfaceItem::~MirSurfaceItem() |
69 | @@ -234,6 +238,10 @@ |
70 | delete m_qmlContentComponent; |
71 | m_qmlContentComponent = nullptr; |
72 | |
73 | + if (hasActiveFocus() && m_consumesInput && m_qmlSurface->live()) { |
74 | + m_qmlSurface->setActiveFocus(false); |
75 | + } |
76 | + |
77 | disconnect(m_qmlSurface, nullptr, this, nullptr); |
78 | m_qmlSurface->unregisterView((qintptr)this); |
79 | } |
80 | @@ -243,8 +251,6 @@ |
81 | if (m_qmlSurface) { |
82 | m_qmlSurface->registerView((qintptr)this); |
83 | |
84 | - m_qmlSurface->setActiveFocus(hasActiveFocus()); |
85 | - |
86 | updateSurfaceSize(); |
87 | updateMirSurfaceVisibility(); |
88 | |
89 | @@ -277,17 +283,20 @@ |
90 | default: |
91 | qFatal("MirSurfaceItem: Unhandled component status"); |
92 | } |
93 | + |
94 | + if (m_consumesInput) { |
95 | + m_qmlSurface->setActiveFocus(hasActiveFocus()); |
96 | + } |
97 | } |
98 | |
99 | Q_EMIT surfaceChanged(m_qmlSurface); |
100 | } |
101 | |
102 | -void MirSurfaceItem::itemChange(ItemChange change, const ItemChangeData & value) |
103 | + |
104 | +void MirSurfaceItem::updateMirSurfaceActiveFocus(bool focused) |
105 | { |
106 | - if (change == QQuickItem::ItemActiveFocusHasChanged) { |
107 | - if (m_qmlSurface) { |
108 | - m_qmlSurface->setActiveFocus(value.boolValue); |
109 | - } |
110 | + if (m_qmlSurface && m_consumesInput && m_qmlSurface->live()) { |
111 | + m_qmlSurface->setActiveFocus(focused); |
112 | } |
113 | } |
114 | |
115 | |
116 | === modified file 'tests/mocks/Unity/Application/MirSurfaceItem.h' |
117 | --- tests/mocks/Unity/Application/MirSurfaceItem.h 2016-03-29 03:47:39 +0000 |
118 | +++ tests/mocks/Unity/Application/MirSurfaceItem.h 2016-05-18 22:09:41 +0000 |
119 | @@ -98,12 +98,12 @@ |
120 | void mousePressEvent(QMouseEvent * event) override; |
121 | void mouseMoveEvent(QMouseEvent * event) override; |
122 | void mouseReleaseEvent(QMouseEvent * event) override; |
123 | - void itemChange(ItemChange change, const ItemChangeData & value) override; |
124 | |
125 | private Q_SLOTS: |
126 | void onComponentStatusChanged(QQmlComponent::Status status); |
127 | void updateScreenshot(QUrl screenshot); |
128 | void updateMirSurfaceVisibility(); |
129 | + void updateMirSurfaceActiveFocus(bool focused); |
130 | |
131 | private: |
132 | void createQmlContentItem(); |
133 | |
134 | === modified file 'tests/mocks/Utils/windowstatestorage.cpp' |
135 | --- tests/mocks/Utils/windowstatestorage.cpp 2016-03-29 03:47:39 +0000 |
136 | +++ tests/mocks/Utils/windowstatestorage.cpp 2016-05-18 22:09:41 +0000 |
137 | @@ -50,6 +50,7 @@ |
138 | void WindowStateStorage::saveStage(const QString &appId, int stage) |
139 | { |
140 | m_stage[appId] = stage; |
141 | + Q_EMIT stageSaved(appId, stage); |
142 | } |
143 | |
144 | int WindowStateStorage::getStage(const QString &appId) const |
145 | |
146 | === modified file 'tests/mocks/Utils/windowstatestorage.h' |
147 | --- tests/mocks/Utils/windowstatestorage.h 2016-03-29 03:47:39 +0000 |
148 | +++ tests/mocks/Utils/windowstatestorage.h 2016-05-18 22:09:41 +0000 |
149 | @@ -45,6 +45,9 @@ |
150 | Q_SIGNALS: |
151 | void geometryChanged(const QVariantMap& geometry); |
152 | |
153 | + // For testing. |
154 | + void stageSaved(const QString& appId, int stage); |
155 | + |
156 | private: |
157 | void setGeometry(const QVariantMap& geometry); |
158 | QVariantMap geometry() const; |
159 | |
160 | === modified file 'tests/qmltests/Stages/tst_TabletStage.qml' |
161 | --- tests/qmltests/Stages/tst_TabletStage.qml 2016-04-27 15:01:10 +0000 |
162 | +++ tests/qmltests/Stages/tst_TabletStage.qml 2016-05-18 22:09:41 +0000 |
163 | @@ -129,6 +129,12 @@ |
164 | } |
165 | } |
166 | |
167 | + SignalSpy { |
168 | + id: stageSaver |
169 | + target: WindowStateStorage |
170 | + signalName: "stageSaved" |
171 | + } |
172 | + |
173 | UnityTestCase { |
174 | id: testCase |
175 | name: "TabletStage" |
176 | @@ -138,6 +144,7 @@ |
177 | property Item sideStage: tabletStage ? findChild(tabletStage, "sideStage") : null |
178 | |
179 | function init() { |
180 | + stageSaver.clear(); |
181 | tabletStageLoader.active = true; |
182 | tryCompare(tabletStageLoader, "status", Loader.Ready); |
183 | |
184 | @@ -510,7 +517,35 @@ |
185 | tryCompare(stagesPriv, "sideStageAppId", data.sideStageAppId); |
186 | } |
187 | |
188 | - function test_loadSideStageByDragginFromMainStage() { |
189 | + function test_applicationSavesLastStage_data() { |
190 | + return [ |
191 | + { tag: "MainStage", stage: ApplicationInfoInterface.MainStage}, |
192 | + { tag: "SideStage", stage: ApplicationInfoInterface.SideStage}, |
193 | + ]; |
194 | + } |
195 | + |
196 | + function test_applicationSavesLastStage(data) { |
197 | + WindowStateStorage.saveStage(webbrowserCheckBox.appId, data.stage); |
198 | + stageSaver.clear(); |
199 | + |
200 | + var stagesPriv = findInvisibleChild(tabletStage, "stagesPriv"); |
201 | + verify(stagesPriv); |
202 | + |
203 | + tryCompare(stagesPriv, "mainStageAppId", "unity8-dash"); |
204 | + tryCompare(stagesPriv, "sideStageAppId", ""); |
205 | + |
206 | + var webbrowserSurfaceId = topSurfaceList.nextId; |
207 | + webbrowserCheckBox.checked = true; |
208 | + waitUntilAppSurfaceShowsUp(webbrowserSurfaceId); |
209 | + |
210 | + webbrowserCheckBox.checked = false; |
211 | + |
212 | + tryCompare(stageSaver, "count", 1); |
213 | + compare(stageSaver.signalArguments[0][0], "webbrowser-app") |
214 | + compare(stageSaver.signalArguments[0][1], data.stage) |
215 | + } |
216 | + |
217 | + function test_loadSideStageByDraggingFromMainStage() { |
218 | sideStage.showNow(); |
219 | var webbrowserSurfaceId = topSurfaceList.nextId; |
220 | webbrowserCheckBox.checked = true; |
221 | @@ -538,7 +573,7 @@ |
222 | tryCompare(appDelegate, "stage", ApplicationInfoInterface.SideStage); |
223 | } |
224 | |
225 | - function test_unloadSideStageByDragginFromStageStage() { |
226 | + function test_unloadSideStageByDraggingFromStageStage() { |
227 | sideStage.showNow(); |
228 | WindowStateStorage.saveStage(webbrowserCheckBox.appId, ApplicationInfoInterface.SideStage) |
229 | var webbrowserSurfaceId = topSurfaceList.nextId; |
230 | @@ -690,5 +725,34 @@ |
231 | // only unity8-dash surface is left |
232 | compare(topSurfaceList.count, 1); |
233 | } |
234 | + |
235 | + function test_draggingSurfaceKeepsSurfaceFocus() { |
236 | + var webbrowserSurfaceId = topSurfaceList.nextId; |
237 | + webbrowserCheckBox.checked = true; |
238 | + waitUntilAppSurfaceShowsUp(webbrowserSurfaceId); |
239 | + |
240 | + var appDelegate = findChild(tabletStage, "spreadDelegate_" + webbrowserSurfaceId); |
241 | + verify(appDelegate); |
242 | + compare(appDelegate.stage, ApplicationInfoInterface.MainStage); |
243 | + |
244 | + tryCompare(appDelegate.surface, "activeFocus", true); |
245 | + |
246 | + var pos = tabletStage.width - sideStage.width - (tabletStage.width - sideStage.width) / 2; |
247 | + var end_pos = tabletStage.width - sideStage.width / 2; |
248 | + |
249 | + multiTouchDragUntil([0,1,2], |
250 | + tabletStage, |
251 | + pos, |
252 | + tabletStage.height / 2, |
253 | + units.gu(3), |
254 | + 0, |
255 | + function() { |
256 | + pos += units.gu(3); |
257 | + return sideStage.shown && !sideStage.showAnimation.running && |
258 | + pos >= end_pos; |
259 | + }); |
260 | + |
261 | + tryCompare(appDelegate.surface, "activeFocus", true); |
262 | + } |
263 | } |
264 | } |
FAILED: Continuous integration, rev:2386 /unity8- jenkins. ubuntu. com/job/ lp-unity8- ci/1132/ /unity8- jenkins. ubuntu. com/job/ test-0- autopkgtest/ label=amd64, release= vivid+overlay, testname= qmluitests. sh/687 /unity8- jenkins. ubuntu. com/job/ test-0- autopkgtest/ label=amd64, release= xenial+ overlay, testname= qmluitests. sh/687 /unity8- jenkins. ubuntu. com/job/ test-0- autopkgtest/ label=phone- armhf,release= vivid+overlay, testname= autopilot. sh/687 /unity8- jenkins. ubuntu. com/job/ build-0- fetch/1518 /unity8- jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= vivid+overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= xenial+ overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=amd64, release= vivid+overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=amd64, release= vivid+overlay/ 1481/artifact/ output/ *zip*/output. zip /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=amd64, release= xenial+ overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=amd64, release= xenial+ overlay/ 1481/artifact/ output/ *zip*/output. zip /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=armhf, release= vivid+overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=armhf, release= vivid+overlay/ 1481/artifact/ output/ *zip*/output. zip /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=armhf, release= xenial+ overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=armhf, release= xenial+ overlay/ 1481/artifact/ output/ *zip*/output. zip /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=i386, release= vivid+overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=i386, release= vivid+overlay/ 1481/artifact/ output/ *zip*/output. zip /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=i386, release= xenial+ overlay/ 1481 /unity8- jenkins. ubuntu. com/job/ build-2- binpkg/ arch=i386, release= xenial+ overlay/ 1481/artifact/ output/ *zip*/output. zip
https:/
Executed test runs:
UNSTABLE: https:/
SUCCESS: https:/
UNSTABLE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild: /unity8- jenkins. ubuntu. com/job/ lp-unity8- ci/1132/ rebuild
https:/