Merge lp:~lukas-kde/unity8/fix-rotation-lp1614070 into lp:unity8
- fix-rotation-lp1614070
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Albert Astals Cid | ||||
Approved revision: | 2603 | ||||
Merged at revision: | 2597 | ||||
Proposed branch: | lp:~lukas-kde/unity8/fix-rotation-lp1614070 | ||||
Merge into: | lp:unity8 | ||||
Diff against target: |
239 lines (+104/-9) 6 files modified
qml/Notifications/Notification.qml (+5/-2) qml/Notifications/Notifications.qml (+5/-1) tests/mocks/Unity/Notifications/MockNotification.cpp (+15/-0) tests/mocks/Unity/Notifications/MockNotification.h (+5/-0) tests/qmltests/Notifications/Notification.qml (+1/-0) tests/qmltests/Notifications/tst_Notifications.qml (+73/-6) |
||||
To merge this branch: | bzr merge lp:~lukas-kde/unity8/fix-rotation-lp1614070 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Unity8 CI Bot | continuous-integration | Needs Fixing | |
Albert Astals Cid (community) | Approve | ||
Michał Sawicz | Needs Fixing | ||
Review via email:
|
Commit message
Reset topmostIsFullscreen to correctly rotate the shell when dismissing SIM PIN screen
Description of the change
Reset topmostIsFullscreen to correctly rotate the shell when dismissing SIM PIN screen
Fixes lp:1614070
* Are there any related MPs required for this MP to build/function as expected? Please list.
N
* Did you perform an exploratory manual test run of your code change and any related functionality?
Y
* 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 : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michał Sawicz (saviq) wrote : | # |
Hey, this is not enough, the value of topmostIsFullscreen is not getting reset after you have a fullscreen one, we've decided to revert this commit whole, will need to bring it back, fixed, with a test for rotation ideally.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michał Sawicz (saviq) wrote : | # |
OK after reading your comment on the other MP you're right, we shouldn't land it as is, but we still need a proper fix.
- 2598. By Lukáš Tinkl
-
reset when there's no more notifications around
- 2599. By Lukáš Tinkl
-
don't let all the notifications fight for topmostIsFullscreen
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:2598
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
SUCCESS: 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:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 2600. By Lukáš Tinkl
-
add a mock and test for the fullscreen notifications
- 2601. By Lukáš Tinkl
-
correctly cleanup
- 2602. By Lukáš Tinkl
-
guard another unconditional
- 2603. By Lukáš Tinkl
-
remove stray empty line
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Albert Astals Cid (aacid) 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.
Let's wait before top approval.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:2599
https:/
Executed test runs:
SUCCESS: https:/
UNSTABLE: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: 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:/
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)
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:2603
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
SUCCESS: 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:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Preview Diff
1 | === modified file 'qml/Notifications/Notification.qml' |
2 | --- qml/Notifications/Notification.qml 2016-08-02 11:33:03 +0000 |
3 | +++ qml/Notifications/Notification.qml 2016-08-19 13:41:38 +0000 |
4 | @@ -37,7 +37,8 @@ |
5 | property var hints |
6 | property var notification |
7 | property color color: theme.palette.normal.background |
8 | - property bool fullscreen: false |
9 | + property bool fullscreen: notification.notification && typeof notification.notification.fullscreen != "undefined" ? |
10 | + notification.notification.fullscreen : false // fullscreen prop only exists in the mock |
11 | property int maxHeight |
12 | property int margins: units.gu(1) |
13 | |
14 | @@ -123,7 +124,9 @@ |
15 | if (fullscreen) { |
16 | notification.notification.urgency = Notification.Critical; |
17 | } |
18 | - ListView.view.topmostIsFullscreen = fullscreen; |
19 | + if (index == 0) { |
20 | + ListView.view.topmostIsFullscreen = fullscreen; |
21 | + } |
22 | } |
23 | |
24 | Behavior on implicitHeight { |
25 | |
26 | === modified file 'qml/Notifications/Notifications.qml' |
27 | --- qml/Notifications/Notifications.qml 2016-08-02 11:33:03 +0000 |
28 | +++ qml/Notifications/Notifications.qml 2016-08-19 13:41:38 +0000 |
29 | @@ -69,7 +69,9 @@ |
30 | |
31 | property int theIndex: index |
32 | onTheIndexChanged: { |
33 | - ListView.view.topmostIsFullscreen = fullscreen; // when we get pushed down by e.g. volume notification |
34 | + if (theIndex == 0) { |
35 | + ListView.view.topmostIsFullscreen = fullscreen; // when we get pushed down by e.g. volume notification |
36 | + } |
37 | } |
38 | |
39 | // make sure there's no opacity-difference between the several |
40 | @@ -78,6 +80,8 @@ |
41 | //layer.enabled: add.running || remove.running || populate.running |
42 | } |
43 | |
44 | + onCountChanged: if (count == 0) topmostIsFullscreen = false; // reset |
45 | + |
46 | // FIXME: disabled all transitions because of LP: #1354406 workaround |
47 | /*populate: Transition { |
48 | UbuntuNumberAnimation { |
49 | |
50 | === modified file 'tests/mocks/Unity/Notifications/MockNotification.cpp' |
51 | --- tests/mocks/Unity/Notifications/MockNotification.cpp 2015-09-18 13:58:21 +0000 |
52 | +++ tests/mocks/Unity/Notifications/MockNotification.cpp 2016-08-19 13:41:38 +0000 |
53 | @@ -32,6 +32,7 @@ |
54 | QStringList actions; |
55 | ActionModel* actionsModel; |
56 | QVariantMap hints; |
57 | + bool fullscreen = false; |
58 | }; |
59 | |
60 | MockNotification::MockNotification(QObject *parent) : QObject(parent), p(new MockNotificationPrivate()) { |
61 | @@ -181,3 +182,17 @@ |
62 | void MockNotification::close() { |
63 | Q_EMIT completed(p->id); |
64 | } |
65 | + |
66 | +bool MockNotification::fullscreen() const |
67 | +{ |
68 | + return p->fullscreen; |
69 | +} |
70 | + |
71 | +void MockNotification::setFullscreen(bool fullscreen) |
72 | +{ |
73 | + if (p->fullscreen == fullscreen) |
74 | + return; |
75 | + |
76 | + p->fullscreen = fullscreen; |
77 | + Q_EMIT fullscreenChanged(fullscreen); |
78 | +} |
79 | |
80 | === modified file 'tests/mocks/Unity/Notifications/MockNotification.h' |
81 | --- tests/mocks/Unity/Notifications/MockNotification.h 2015-09-18 13:58:21 +0000 |
82 | +++ tests/mocks/Unity/Notifications/MockNotification.h 2016-08-19 13:41:38 +0000 |
83 | @@ -41,6 +41,7 @@ |
84 | Q_PROPERTY(QStringList rawActions READ rawActions WRITE setActions) |
85 | Q_PROPERTY(ActionModel* actions READ getActions NOTIFY actionsChanged) |
86 | Q_PROPERTY(QVariantMap hints READ getHints WRITE setHints NOTIFY hintsChanged) |
87 | + Q_PROPERTY(bool fullscreen READ fullscreen WRITE setFullscreen NOTIFY fullscreenChanged) // only in mock |
88 | |
89 | private: |
90 | QScopedPointer<MockNotificationPrivate> p; |
91 | @@ -64,6 +65,8 @@ |
92 | void completed(int nid); |
93 | void actionInvoked(const QString &action); |
94 | |
95 | + void fullscreenChanged(bool fullscreen); |
96 | + |
97 | public: |
98 | MockNotification(QObject *parent=nullptr); |
99 | virtual ~MockNotification(); |
100 | @@ -87,6 +90,8 @@ |
101 | void setActions(const QStringList &actions); |
102 | QVariantMap getHints() const; |
103 | void setHints(const QVariantMap& hints); |
104 | + bool fullscreen() const; |
105 | + void setFullscreen(bool fullscreen); |
106 | |
107 | Q_INVOKABLE void invokeAction(const QString &action); |
108 | Q_INVOKABLE void close(); |
109 | |
110 | === modified file 'tests/qmltests/Notifications/Notification.qml' |
111 | --- tests/qmltests/Notifications/Notification.qml 2015-01-26 12:03:19 +0000 |
112 | +++ tests/qmltests/Notifications/Notification.qml 2016-08-19 13:41:38 +0000 |
113 | @@ -28,4 +28,5 @@ |
114 | secondaryIcon: "" |
115 | value: 0 |
116 | rawActions: [] |
117 | + fullscreen: false |
118 | } |
119 | |
120 | === modified file 'tests/qmltests/Notifications/tst_Notifications.qml' |
121 | --- tests/qmltests/Notifications/tst_Notifications.qml 2016-08-02 10:28:28 +0000 |
122 | +++ tests/qmltests/Notifications/tst_Notifications.qml 2016-08-19 13:41:38 +0000 |
123 | @@ -141,6 +141,18 @@ |
124 | mockModel.append(n) |
125 | } |
126 | |
127 | + function addFullscreenNotification() { |
128 | + var component = Qt.createComponent("Notification.qml") |
129 | + var n = component.createObject("notification", {"nid": index++, |
130 | + "type": Notification.SnapDecision, |
131 | + "hints": {"x-canonical-private-affirmative-tint": "true"}, |
132 | + "summary": "SIM PIN screen", |
133 | + "body": "Enter your PIN to unlock the SIM", |
134 | + "fullscreen": true}) |
135 | + n.completed.connect(mockModel.onCompleted) |
136 | + mockModel.append(n) |
137 | + } |
138 | + |
139 | function clearNotifications() { |
140 | while(mockModel.count > 0) { |
141 | remove1stNotification(); |
142 | @@ -230,6 +242,12 @@ |
143 | |
144 | Button { |
145 | width: parent.width |
146 | + text: "add a fullscreen notification" |
147 | + onClicked: rootRow.addFullscreenNotification() |
148 | + } |
149 | + |
150 | + Button { |
151 | + width: parent.width |
152 | text: "remove 1st notification" |
153 | onClicked: rootRow.remove1stNotification() |
154 | } |
155 | @@ -371,6 +389,14 @@ |
156 | rawActions: ["ok_id", "Ok", |
157 | "snooze_id", "Snooze", |
158 | "view_id", "View"] |
159 | + }, |
160 | + Notification { |
161 | + nid: 10 |
162 | + type: Notification.SnapDecision |
163 | + hints: {"x-canonical-private-affirmative-tint": "true"} |
164 | + summary: "SIM PIN screen" |
165 | + body: "Enter your PIN to unlock the SIM" |
166 | + fullscreen: true |
167 | } |
168 | ] |
169 | |
170 | @@ -538,15 +564,12 @@ |
171 | signalName: "actionInvoked" |
172 | } |
173 | |
174 | - function init() { |
175 | - while (mockModel.count > 0) { |
176 | - mockModel.remove(0); |
177 | - } |
178 | - } |
179 | - |
180 | function cleanup() { |
181 | clickThroughSpy.clear() |
182 | actionSpy.clear() |
183 | + while (mockModel.count > 0) { |
184 | + mockModel.removeFirst(); |
185 | + } |
186 | } |
187 | |
188 | function test_NotificationRenderer(data) { |
189 | @@ -797,6 +820,50 @@ |
190 | tryCompareFunction(function() { return notification2.expanded; }, undefined); |
191 | } |
192 | |
193 | + function topmostIsFullscreen_data() { |
194 | + return [{ |
195 | + tag: "Confirmation notification with value", |
196 | + n: nlist[6] |
197 | + }, |
198 | + { |
199 | + tag: "SIM PIN fullscreen notification", |
200 | + n: nlist[9] |
201 | + }, |
202 | + { |
203 | + tag: "Snap Decision without secondary icon and no button-tint", |
204 | + n: nlist[3] |
205 | + }] |
206 | + } |
207 | + |
208 | + function test_topmostIsFullscreen() { |
209 | + var data = topmostIsFullscreen_data(); |
210 | + |
211 | + // fill the model |
212 | + data.forEach(function(notification) { |
213 | + mockModel.append(notification.n); |
214 | + notification.n.completed.connect(mockModel.onCompleted); |
215 | + }) |
216 | + |
217 | + // make sure the view is properly updated before going on |
218 | + notifications.forceLayout(); |
219 | + waitForRendering(notifications); |
220 | + |
221 | + // initially, topmost is not fullscreen |
222 | + verify(!notifications.topmostIsFullscreen) |
223 | + |
224 | + // close the 1st one, 2nd one should be fullscreen |
225 | + var notification0 = findChild(notifications, "notification0") |
226 | + notification0.closeNotification(); |
227 | + waitForRendering(notifications); |
228 | + verify(notifications.topmostIsFullscreen) |
229 | + |
230 | + // close the 1st one (SIM PIN) again, topmost should no longer be fullscreen |
231 | + notification0 = findChild(notifications, "notification0") |
232 | + notification0.closeNotification(); |
233 | + waitForRendering(notifications); |
234 | + verify(!notifications.topmostIsFullscreen) |
235 | + } |
236 | + |
237 | function cleanupTestCase() { |
238 | notifications.hasMouse = false; |
239 | } |
FAILED: Continuous integration, rev:2597 /unity8- jenkins. ubuntu. com/job/ lp-unity8- ci/1983/ /unity8- jenkins. ubuntu. com/job/ build/2602/ console /unity8- jenkins. ubuntu. com/job/ build-0- fetch/2630/ console
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild: /unity8- jenkins. ubuntu. com/job/ lp-unity8- ci/1983/ rebuild
https:/