Merge lp:~zsombi/ubuntu-ui-toolkit/aplRemovePages into lp:ubuntu-ui-toolkit/staging

Proposed by Zsombor Egri
Status: Merged
Approved by: Tim Peeters
Approved revision: 1888
Merged at revision: 1898
Proposed branch: lp:~zsombi/ubuntu-ui-toolkit/aplRemovePages
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 259 lines (+107/-29)
3 files modified
src/Ubuntu/Components/1.3/AdaptivePageLayout.qml (+5/-4)
tests/unit_x11/tst_components/MyExternalPageWithNewHeader.qml (+26/-0)
tests/unit_x11/tst_components/tst_adaptivepagelayout.qml (+76/-25)
To merge this branch: bzr merge lp:~zsombi/ubuntu-ui-toolkit/aplRemovePages
Reviewer Review Type Date Requested Status
ubuntu-sdk-build-bot continuous-integration Approve
Tim Peeters Approve
Review via email: mp+288090@code.launchpad.net

Commit message

AdaptivePageLayout removes all pages pushed to next columns when addPageToNextColumn() is called

To post a comment you must log in.
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1885. By Zsombor Egri

roll back changes on MyExternalPage and add a new one to be used by the APL tests

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Tim Peeters (tpeeters) wrote :

Only one small comment (see below).

Also, the tst_tree tests fail. That is unrelated to this MR but those need to be fixed before we can land this.

Revision history for this message
Tim Peeters (tpeeters) :
review: Needs Fixing
1886. By Zsombor Egri

review comment applied

1887. By Zsombor Egri

staging sync

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Tim Peeters (tpeeters) wrote :

There is one failure in tst_adaptivepagelayout.qml. Is it this one? tst_adaptivepagelayout.qml: XPASS : components::test_hidden_page_keeps_geometry_bug1492343() 'wait for signal heightChanged' returned TRUE unexpectedly. ()

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Zsombor Egri (zsombi) wrote :

> There is one failure in tst_adaptivepagelayout.qml. Is it this one?
> tst_adaptivepagelayout.qml: XPASS :
> components::test_hidden_page_keeps_geometry_bug1492343() 'wait for signal
> heightChanged' returned TRUE unexpectedly. ()

Dear Tim, that was a test you were supposed to look at it >10 days ago...

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1888. By Zsombor Egri

staging sync

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Tim Peeters (tpeeters) wrote :

All the dependencies are in place now :) Let's get this one in too.

review: Approve
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
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/Ubuntu/Components/1.3/AdaptivePageLayout.qml'
2--- src/Ubuntu/Components/1.3/AdaptivePageLayout.qml 2016-03-15 14:39:45 +0000
3+++ src/Ubuntu/Components/1.3/AdaptivePageLayout.qml 2016-03-16 09:15:53 +0000
4@@ -339,11 +339,14 @@
5 */
6 function addPageToNextColumn(sourcePage, page, properties) {
7 var nextColumn = d.columnForPage(sourcePage) + 1;
8- d.tree.prune(nextColumn);
9+ var wrappers = d.tree.prune(nextColumn);
10 return d.addPageToColumn(nextColumn, sourcePage, page, properties, function() {
11 for (var i = nextColumn; i < d.columns; i++) {
12 d.updatePageForColumn(i);
13 }
14+ for (var i in wrappers) {
15+ wrappers[i].destroyObject();
16+ }
17 });
18 }
19
20@@ -591,9 +594,7 @@
21 if (newWrapper) {
22 columnHolder.attachPage(newWrapper);
23 }
24- if (oldWrapper.canDestroy) {
25- oldWrapper.destroyObject();
26- }
27+ oldWrapper.destroyObject();
28 }
29 }
30
31
32=== added file 'tests/unit_x11/tst_components/MyExternalPageWithNewHeader.qml'
33--- tests/unit_x11/tst_components/MyExternalPageWithNewHeader.qml 1970-01-01 00:00:00 +0000
34+++ tests/unit_x11/tst_components/MyExternalPageWithNewHeader.qml 2016-03-16 09:15:53 +0000
35@@ -0,0 +1,26 @@
36+/*
37+ * Copyright 2016 Canonical Ltd.
38+ *
39+ * This program is free software; you can redistribute it and/or modify
40+ * it under the terms of the GNU Lesser General Public License as published by
41+ * the Free Software Foundation; version 3.
42+ *
43+ * This program is distributed in the hope that it will be useful,
44+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
45+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
46+ * GNU Lesser General Public License for more details.
47+ *
48+ * You should have received a copy of the GNU Lesser General Public License
49+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
50+ */
51+
52+import QtQuick 2.4
53+import Ubuntu.Components 1.3
54+
55+Page {
56+ header: PageHeader { title: "Page from QML file" }
57+ Label {
58+ anchors.centerIn: parent
59+ text: "This page was created from MyExternalPageWithNewHeader.qml."
60+ }
61+}
62
63=== modified file 'tests/unit_x11/tst_components/tst_adaptivepagelayout.qml'
64--- tests/unit_x11/tst_components/tst_adaptivepagelayout.qml 2016-02-10 14:03:36 +0000
65+++ tests/unit_x11/tst_components/tst_adaptivepagelayout.qml 2016-03-16 09:15:53 +0000
66@@ -38,8 +38,8 @@
67
68 Page {
69 id: page1
70- objectName: title
71- title: "Page1"
72+ objectName: header.title
73+ header: PageHeader { title: "Page1" }
74
75 Column {
76 anchors.centerIn: parent
77@@ -56,18 +56,18 @@
78 }
79 Page {
80 id: page2
81- objectName: title
82- title: "Page2"
83+ objectName: header.title
84+ header: PageHeader { title: "Page2" }
85 }
86 Page {
87 id: page3
88- objectName: title
89- title: "Page3"
90+ objectName: header.title
91+ header: PageHeader { title: "Page3" }
92 }
93 Page {
94 id: page4
95- objectName: title
96- title: "Page4"
97+ objectName: header.title
98+ header: PageHeader { title: "Page4" }
99 }
100 }
101 AdaptivePageLayout {
102@@ -76,18 +76,18 @@
103 height: parent.height / 2
104 Page {
105 id: otherPage1
106- objectName: title
107- title: "Page1"
108+ objectName: header.title
109+ header: PageHeader { title: "Page1" }
110 }
111 Page {
112 id: otherPage2
113- objectName: title
114- title: "Page2"
115+ objectName: header.title
116+ header: PageHeader { title: "Page2" }
117 }
118 Page {
119 id: otherPage3
120- objectName: title
121- title: "Page3"
122+ objectName: header.title
123+ header: PageHeader { title: "Page3" }
124 }
125 }
126 }
127@@ -95,8 +95,10 @@
128 Component {
129 id: pageComponent
130 Page {
131- objectName: title
132- title: "DynamicPage"
133+ objectName: header.title
134+ header: PageHeader { title: "DynamicPage" }
135+ signal deleted()
136+ Component.onDestruction: deleted()
137 }
138 }
139
140@@ -116,6 +118,16 @@
141 signalName: "primaryPageChanged"
142 }
143
144+ SignalSpy {
145+ id: deletedSpy1
146+ signalName: "deleted"
147+ }
148+
149+ SignalSpy {
150+ id: deletedSpy2
151+ signalName: "deleted"
152+ }
153+
154 function resize_single_column() {
155 layout.width = units.gu(40);
156 }
157@@ -142,16 +154,20 @@
158 }
159
160 function cleanup() {
161- page1.title = "Page1";
162- page2.title = "Page2";
163- page3.title = "Page3";
164- page4.title = "Page4";
165+ page1.header.title = "Page1";
166+ page2.header.title = "Page2";
167+ page3.header.title = "Page3";
168+ page4.header.title = "Page4";
169 loadedSpy.clear();
170 primaryPageSpy.clear();
171 primaryPageSpy.target = null;
172 resize_multiple_columns();
173 layout.removePages(layout.primaryPage);
174 defaults.primaryPage = null;
175+ // restore binding on column number
176+ root.columns = Qt.binding(function () {return root.width >= units.gu(80) ? 2 : 1});
177+ // restore async
178+ layout.asynchronous = true;
179 wait(200);
180 }
181
182@@ -306,7 +322,7 @@
183 var testColumn = MathUtils.clamp(wrapper.column + (data.func == "addPageToCurrentColumn" ? 0 : 1),
184 0, layout.columns - 1);
185 var testHolder = getColumnHolder(layout, testColumn);
186- compare(testHolder.pageWrapper.object.title, data.expectedTitle, "page not found");
187+ compare(testHolder.pageWrapper.object.header.title, data.expectedTitle, "page not found");
188 }
189
190 function test_asynchronous_page_loading_incubator_forcecompletion() {
191@@ -383,7 +399,7 @@
192 function test_primaryPageSource_bug1499179_data() {
193 return [
194 {tag: "Component", test: pageComponent},
195- {tag: "Document", test: Qt.resolvedUrl("MyExternalPage.qml")},
196+ {tag: "Document", test: Qt.resolvedUrl("MyExternalPageWithNewHeader.qml")},
197 ];
198 }
199 function test_primaryPageSource_bug1499179(data) {
200@@ -394,8 +410,8 @@
201
202 function test_change_primaryPageSource_data() {
203 return [
204- {tag: "Component", test: pageComponent, nextValue: Qt.resolvedUrl("MyExternalPage.qml")},
205- {tag: "Document", test: Qt.resolvedUrl("MyExternalPage.qml"), nextValue: pageComponent},
206+ {tag: "Component", test: pageComponent, nextValue: Qt.resolvedUrl("MyExternalPageWithNewHeader.qml")},
207+ {tag: "Document", test: Qt.resolvedUrl("MyExternalPageWithNewHeader.qml"), nextValue: pageComponent},
208 ];
209 }
210 function test_change_primaryPageSource(data) {
211@@ -428,12 +444,47 @@
212 primaryPageSpy.target = defaults;
213 defaults.primaryPageSource = pageComponent;
214 primaryPageSpy.wait(400);
215- compare(defaults.primaryPage.title, "DynamicPage", "DynamicPage not set as primaryPage");
216+ compare(defaults.primaryPage.header.title, "DynamicPage", "DynamicPage not set as primaryPage");
217 // now set a value to primaryPage
218 primaryPageSpy.clear();
219 defaults.primaryPage = otherPage1;
220 primaryPageSpy.wait(400);
221 compare(defaults.primaryPageSource, undefined, "primaryPageSource must be cleared");
222 }
223+
224+ function test_add_page_to_next_column_doesnt_delete_prev_column_content_bug1544745() {
225+
226+ var incubator = layout.addPageToNextColumn(layout.primaryPage, pageComponent);
227+ verify(incubator);
228+ verify(incubator.status != Component.Error);
229+ var newPage = null;
230+ if (incubator.status == Component.Loading) {
231+ incubator.forceCompletion();
232+ newPage = incubator.object;
233+ } else if (incubator.status == Component.Ready) {
234+ newPage = incubator.object;
235+ }
236+ deletedSpy1.target = newPage;
237+
238+ // add one page on top of the page on second column
239+ incubator = layout.addPageToCurrentColumn(newPage, pageComponent);
240+ verify(incubator);
241+ newPage = null;
242+ if (incubator.status == Component.Loading) {
243+ incubator.forceCompletion();
244+ newPage = incubator.object;
245+ } else if (incubator.status == Component.Ready) {
246+ newPage = incubator.object;
247+ }
248+ deletedSpy2.target = newPage;
249+
250+ // add a new page relative to the primary page to the next column
251+ // this should remove the previously added two pages
252+ incubator = layout.addPageToNextColumn(layout.primaryPage, pageComponent);
253+ verify(incubator);
254+ incubator.forceCompletion();
255+ deletedSpy1.wait(500);
256+ deletedSpy2.wait(500);
257+ }
258 }
259 }

Subscribers

People subscribed via source and target branches