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

Proposed by Zsombor Egri
Status: Merged
Approved by: Cris Dywan
Approved revision: 1668
Merged at revision: 1673
Proposed branch: lp:~zsombi/ubuntu-ui-toolkit/aplReplacePrimaryPage
Merge into: lp:ubuntu-ui-toolkit/staging
Prerequisite: lp:~zsombi/ubuntu-ui-toolkit/aplPrimaryPageAsync
Diff against target: 466 lines (+179/-138)
2 files modified
src/Ubuntu/Components/1.3/AdaptivePageLayout.qml (+52/-30)
tests/unit_x11/tst_components/tst_adaptivepagelayout.qml (+127/-108)
To merge this branch: bzr merge lp:~zsombi/ubuntu-ui-toolkit/aplReplacePrimaryPage
Reviewer Review Type Date Requested Status
Cris Dywan Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+272868@code.launchpad.net

Commit message

Make primaryPage and primaryPageSource replaceable after component completion.

Description of the change

Make primaryPage and primaryPageSource replaceable after component completion.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
1666. By Zsombor Egri

documentation fixed

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

+ The property can only hold a Page instance. When changed runtime (not by the

...changed at runtime...

+ be cerated asynchronously and the instance will be reported through

...be created asynchronously...

+ if (source === null) {
+ if (source === undefined) {

These conditionals are hard to make sense of for me even after checking the two places the function can be called from. I'm thinking it might make more sense to dissolve the function so it's easier to understand given there is very little code to be shared.

+ function findPageFromLayoutWithProperty(apl, property, value) {

How about making this objectName only? That'd be more consistent and there's no chance of killing time by retroactively investigating localization-related failures because of labels.
Also, may be worth adding to UbuntuTestCase?

review: Needs Fixing
Revision history for this message
Zsombor Egri (zsombi) wrote :

> + The property can only hold a Page instance. When changed runtime (not by the
>
> ...changed at runtime...
>
> + be cerated asynchronously and the instance will be reported through
>
> ...be created asynchronously...
>
> + if (source === null) {
> + if (source === undefined) {
>
> These conditionals are hard to make sense of for me even after checking the
> two places the function can be called from. I'm thinking it might make more
> sense to dissolve the function so it's easier to understand given there is
> very little code to be shared.
>
> + function findPageFromLayoutWithProperty(apl, property, value) {
>
> How about making this objectName only? That'd be more consistent and there's
> no chance of killing time by retroactively investigating localization-related
> failures because of labels.

Yep, makes sense.

> Also, may be worth adding to UbuntuTestCase?

Well... should we have APL specific function in UbuntuTestCase? The function itself looks in the body of the APL and not in the APL children itself. Pages can be added as child to APL (which are then forwarded to the internal hiddenPages) so we are not interested of those...

1667. By Zsombor Egri

review comments applied

1668. By Zsombor Egri

findPageFromLayoutWithProperty renamed into findPageFromLayout

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

Thanks for the update!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/Ubuntu/Components/1.3/AdaptivePageLayout.qml'
--- src/Ubuntu/Components/1.3/AdaptivePageLayout.qml 2015-09-30 09:46:03 +0000
+++ src/Ubuntu/Components/1.3/AdaptivePageLayout.qml 2015-10-02 05:22:37 +0000
@@ -42,13 +42,11 @@
42 add the page to the leftmost column of the view.42 add the page to the leftmost column of the view.
4343
44 The primary page, the very first page must be specified either through the44 The primary page, the very first page must be specified either through the
45 \l primaryPage or \l primaryPageSource properties. The properties cannot be45 \l primaryPage or \l primaryPageSource properties. \l primaryPage can only
46 changed after component completion. \l primaryPage can only hold a Page instance,46 hold a Page instance, \l primaryPageSource can either be a Component or a
47 \l primaryPageSource can either be a Component or a url to a document defining47 url to a document defining a Page. \l primaryPageSource has precedence over
48 a Page. This page cannot be removed from the view. \l primaryPageSource has48 \l primaryPage, and when set it will report the loaded Page through \l primaryPage
49 precedence over \l primaryPage and will create the Page asynchronously. The49 property, and will replace any value set into that property.
50 page instance will be reported through \l primaryPage property and will replace
51 any previous value set to that property.
5250
53 \qml51 \qml
54 import QtQuick 2.452 import QtQuick 2.4
@@ -188,17 +186,18 @@
188 /*!186 /*!
189 The property holds the first Page which will be added to the view. If the187 The property holds the first Page which will be added to the view. If the
190 view has more than one column, the page will be added to the leftmost column.188 view has more than one column, the page will be added to the leftmost column.
191 The property can hold only a Page instance. The property cannot be changed after189 The property can only hold a Page instance. When changed runtime (not by the
192 component completion.190 AdaptivePageLayout component itself), the \l primaryPageSource property
191 will be reset.
193 */192 */
194 property Page primaryPage193 property Page primaryPage
195194
196 /*!195 /*!
197 The property specifies the source of the primaryPage in case the primary196 The property specifies the source of the primaryPage in case the primary
198 page is created from a Component or loaded from an external document. It197 page is created from a Component or loaded from an external document. It
199 has precedence over \l primaryPage and cannot be changed after component198 has precedence over \l primaryPage. The page specified in this way will
200 completion. The page specified in this way will be cerated asynchronously199 be cerated asynchronously and the instance will be reported through
201 and the instance will be reported through \l primaryPage property.200 \l primaryPage property.
202 */201 */
203 property var primaryPageSource202 property var primaryPageSource
204203
@@ -322,12 +321,7 @@
322 pages will be removed.321 pages will be removed.
323 */322 */
324 function removePages(page) {323 function removePages(page) {
325 var nodeToRemove = d.getWrapper(page);324 d.removeAllPages(page, page != layout.primaryPage);
326 var removedNodes = d.tree.chop(nodeToRemove, page != layout.primaryPage);
327 for (var i = removedNodes.length-1; i >= 0; i--) {
328 var node = removedNodes[i];
329 d.updatePageForColumn(node.column);
330 }
331 }325 }
332326
333 /*327 /*
@@ -345,26 +339,34 @@
345 d.createPrimaryPage(primaryPageSource);339 d.createPrimaryPage(primaryPageSource);
346 } else if (primaryPage) {340 } else if (primaryPage) {
347 d.createPrimaryPage(primaryPage);341 d.createPrimaryPage(primaryPage);
348 } else {
349 console.warn("No primary page set. No pages can be added without a primary page.");
350 }342 }
351 d.completed = true;343 d.completed = true;
352 }344 }
353 onPrimaryPageChanged: {345 onPrimaryPageChanged: {
354 if (d.completed && !d.internalUpdate) {346 if (!d.completed || d.internalUpdate) {
355 console.warn("Cannot change primaryPage after completion.");
356 d.internalPropertyUpdate("primaryPage", d.lastPrimaryPage);
357 return;347 return;
358 }348 }
359 d.lastPrimaryPage = primaryPage;349 // reset the primaryPageSource
350 d.internalPropertyUpdate("primaryPageSource", undefined);
351 // clear the layout
352 d.purgeLayout();
353 // add the new page if valid
354 if (primaryPage !== null) {
355 d.createPrimaryPage(primaryPage);
356 }
360 }357 }
361 onPrimaryPageSourceChanged: {358 onPrimaryPageSourceChanged: {
362 if (d.completed && !d.internalUpdate) {359 if (!d.completed || d.internalUpdate) {
363 console.warn("Cannot change primaryPageSource after completion.");
364 d.internalPropertyUpdate("primaryPageSource", d.lastPrimaryPageSource);
365 return;360 return;
366 }361 }
367 d.lastPrimaryPageSource = primaryPageSource;362 // remove all pages first
363 d.purgeLayout();
364 // create the new primary page if a valid component is specified
365 if (primaryPageSource) {
366 d.createPrimaryPage(primaryPageSource);
367 } else {
368 d.internalPropertyUpdate("primaryPage", null);
369 }
368 }370 }
369371
370 onLayoutsChanged: {372 onLayoutsChanged: {
@@ -389,8 +391,7 @@
389 (activeLayout ? activeLayout.data.length : 1)391 (activeLayout ? activeLayout.data.length : 1)
390 property PageColumnsLayout activeLayout: null392 property PageColumnsLayout activeLayout: null
391 property list<PageColumnsLayout> prevLayouts393 property list<PageColumnsLayout> prevLayouts
392 property Page lastPrimaryPage394 property Page prevPrimaryPage
393 property var lastPrimaryPageSource
394395
395 /*! internal */396 /*! internal */
396 onColumnsChanged: {397 onColumnsChanged: {
@@ -416,10 +417,20 @@
416 wrapper.incubator.onStatusChanged = function (status) {417 wrapper.incubator.onStatusChanged = function (status) {
417 if (status == Component.Ready) {418 if (status == Component.Ready) {
418 internalPropertyUpdate("primaryPage", wrapper.incubator.object);419 internalPropertyUpdate("primaryPage", wrapper.incubator.object);
420 prevPrimaryPage = wrapper.incubator.object;
419 }421 }
420 }422 }
421 } else {423 } else {
422 finalizeAddingPage(wrapper);424 finalizeAddingPage(wrapper);
425 prevPrimaryPage = wrapper.object;
426 }
427 }
428
429 // remove all pages, including primaryPage
430 function purgeLayout() {
431 if (prevPrimaryPage) {
432 removeAllPages(prevPrimaryPage, true);
433 prevPrimaryPage = null;
423 }434 }
424 }435 }
425436
@@ -514,6 +525,17 @@
514 return newWrapper.incubator;525 return newWrapper.incubator;
515 }526 }
516527
528 // removes all pages from the layout, and may include the page itself
529 function removeAllPages(page, inclusive) {
530 inclusive = typeof inclusive !== 'undefined' ? inclusive : true;
531 var nodeToRemove = d.getWrapper(page);
532 var removedNodes = d.tree.chop(nodeToRemove, inclusive);
533 for (var i = removedNodes.length-1; i >= 0; i--) {
534 var node = removedNodes[i];
535 updatePageForColumn(node.column);
536 }
537 }
538
517 // update the page for the specified column539 // update the page for the specified column
518 function updatePageForColumn(column) {540 function updatePageForColumn(column) {
519 var effectiveColumn = MathUtils.clamp(column, 0, d.columns - 1);541 var effectiveColumn = MathUtils.clamp(column, 0, d.columns - 1);
520542
=== modified file 'tests/unit_x11/tst_components/tst_adaptivepagelayout.qml'
--- tests/unit_x11/tst_components/tst_adaptivepagelayout.qml 2015-09-29 10:37:46 +0000
+++ tests/unit_x11/tst_components/tst_adaptivepagelayout.qml 2015-10-02 05:22:37 +0000
@@ -27,83 +27,79 @@
27 // 2 on desktop, 1 on phone.27 // 2 on desktop, 1 on phone.
28 property int columns: width >= units.gu(80) ? 2 : 128 property int columns: width >= units.gu(80) ? 2 : 1
2929
30 AdaptivePageLayout {30 Column {
31 id: layout31 anchors.fill: parent
32 width: parent.width32 AdaptivePageLayout {
33 height: parent.height33 id: layout
3434 width: parent.width
35 primaryPage: page135 height: parent.height / 2
3636
37 Page {37 primaryPage: page1
38 id: page138
39 title: "Page1"39 Page {
4040 id: page1
41 Column {41 objectName: title
42 anchors.centerIn: parent42 title: "Page1"
43 width: childrenRect.width43
44 Button {44 Column {
45 text: "Page 2 left"45 anchors.centerIn: parent
46 onTriggered: layout.addPageToCurrentColumn(page1, page2)46 width: childrenRect.width
47 }47 Button {
48 Button {48 text: "Page 2 left"
49 text: "Page 3 right"49 onTriggered: layout.addPageToCurrentColumn(page1, page2)
50 onTriggered: layout.addPageToNextColumn(page1, page3);50 }
51 }51 Button {
52 }52 text: "Page 3 right"
53 }53 onTriggered: layout.addPageToNextColumn(page1, page3);
54 Page {54 }
55 id: page255 }
56 title: "Page2"56 }
57 }57 Page {
58 Page {58 id: page2
59 id: page359 objectName: title
60 title: "Page3"60 title: "Page2"
61 }61 }
62 Page {62 Page {
63 id: page463 id: page3
64 title: "Page4"64 objectName: title
65 }65 title: "Page3"
66 }66 }
6767 Page {
68 AdaptivePageLayout {68 id: page4
69 id: defaults69 objectName: title
70 title: "Page4"
71 }
72 }
73 AdaptivePageLayout {
74 id: defaults
75 width: parent.width
76 height: parent.height / 2
77 Page {
78 id: otherPage1
79 objectName: title
80 title: "Page1"
81 }
82 Page {
83 id: otherPage2
84 objectName: title
85 title: "Page2"
86 }
87 Page {
88 id: otherPage3
89 objectName: title
90 title: "Page3"
91 }
92 }
70 }93 }
7194
72 Component {95 Component {
73 id: pageComponent96 id: pageComponent
74 Page {97 Page {
98 objectName: title
75 title: "DynamicPage"99 title: "DynamicPage"
76 }100 }
77 }101 }
78102
79 Component {
80 id: aplComponent
81 AdaptivePageLayout {
82 width: units.gu(40)
83 height: units.gu(50)
84 primaryPageSource: pageComponent
85 }
86 }
87
88 Component {
89 id: aplDocument
90 AdaptivePageLayout {
91 width: units.gu(40)
92 height: units.gu(50)
93 primaryPageSource: Qt.resolvedUrl("MyExternalPage.qml")
94 }
95 }
96
97 Component {
98 id: aplPrecedence
99 AdaptivePageLayout {
100 width: units.gu(40)
101 height: units.gu(50)
102 primaryPage: page1
103 primaryPageSource: Qt.resolvedUrl("MyExternalPage.qml")
104 }
105 }
106
107 UbuntuTestCase {103 UbuntuTestCase {
108 id: testCase104 id: testCase
109 when: windowShown105 when: windowShown
@@ -115,6 +111,10 @@
115 target: testCase111 target: testCase
116 signalName: "pageLoaded"112 signalName: "pageLoaded"
117 }113 }
114 SignalSpy {
115 id: primaryPageSpy
116 signalName: "primaryPageChanged"
117 }
118118
119 function resize_single_column() {119 function resize_single_column() {
120 layout.width = units.gu(40);120 layout.width = units.gu(40);
@@ -135,29 +135,38 @@
135 return apl.__d.getWrapper(page);135 return apl.__d.getWrapper(page);
136 }136 }
137137
138 function findPageFromLayout(apl, objectName) {
139 var body = findChild(apl, "body");
140 verify(body);
141 return findChild(body, objectName);
142 }
143
138 function cleanup() {144 function cleanup() {
139 page1.title = "Page1";145 page1.title = "Page1";
140 page2.title = "Page2";146 page2.title = "Page2";
141 page3.title = "Page3";147 page3.title = "Page3";
142 page4.title = "Page4";148 page4.title = "Page4";
143 loadedSpy.clear();149 loadedSpy.clear();
150 primaryPageSpy.clear();
151 primaryPageSpy.target = null;
144 resize_multiple_columns();152 resize_multiple_columns();
145 layout.removePages(page1);153 layout.removePages(layout.primaryPage);
154 defaults.primaryPage = null;
155 wait(200);
146 }156 }
147157
148 function initTestCase() {158 function initTestCase() {
149 compare(defaults.primaryPage, null, "primaryPage not null by default");159 compare(defaults.primaryPage, null, "primaryPage not null by default");
150 compare(defaults.primaryPageSource, undefined, "primaryPageSource not set by default");160 compare(defaults.primaryPageSource, undefined, "primaryPageSource not set by default");
151 compare(defaults.layouts.length, 0, "no layouts by default");161 compare(defaults.layouts.length, 0, "no layouts by default");
152 compare(defaults.columns, 1, "1 column as default");162 compare(defaults.columns, columns, columns + " column(s) as default");
153 }163 }
154164
155 function test_change_primaryPage() {165 function test_change_primaryPage() {
156 // this prints the warning but still changes the primary page,166 defaults.primaryPage = otherPage1;
157 // so the test must be executed last not to mess up the other tests.167 defaults.addPageToCurrentColumn(defaults.primaryPage, otherPage2);
158 ignoreWarning("Cannot change primaryPage after completion.");168 defaults.primaryPage = otherPage3;
159 layout.primaryPage = page3;169 verify(!findPageFromLayout(defaults, "Page2"), "Page2 still in the view!");
160 verify(layout.primaryPage != page3, "primaryPage value was changed");
161 }170 }
162171
163 function test_add_page_when_source_page_not_in_stack() {172 function test_add_page_when_source_page_not_in_stack() {
@@ -333,10 +342,10 @@
333 }342 }
334343
335 var testPage = testHolder.pageWrapper.object;344 var testPage = testHolder.pageWrapper.object;
336 var prevPageActive = false345 var prevPageActive = false;
337 var incubator = data.nextColumn346 var incubator = data.nextColumn
338 ? layout.addPageToNextColumn(data.sourcePage, data.page)347 ? layout.addPageToNextColumn(data.sourcePage, data.page)
339 : layout.addPageToCurrentColumn(data.sourcePage, data.page);;348 : layout.addPageToCurrentColumn(data.sourcePage, data.page);
340 verify(incubator);349 verify(incubator);
341 compare(testHolder.pageWrapper.object, testPage);350 compare(testHolder.pageWrapper.object, testPage);
342 incubator.onStatusChanged = function (status) {351 incubator.onStatusChanged = function (status) {
@@ -351,48 +360,58 @@
351360
352 function test_primaryPageSource_bug1499179_data() {361 function test_primaryPageSource_bug1499179_data() {
353 return [362 return [
354 {tag: "Component", test: aplComponent},363 {tag: "Component", test: pageComponent},
355 {tag: "Document", test: aplDocument},364 {tag: "Document", test: Qt.resolvedUrl("MyExternalPage.qml")},
356 ];365 ];
357 }366 }
358 function test_primaryPageSource_bug1499179(data) {367 function test_primaryPageSource_bug1499179(data) {
359 var apl = data.test.createObject(root);368 primaryPageSpy.target = defaults;
360 verify(apl);369 defaults.primaryPageSource = data.test;
361 tryCompareFunction(function () { return apl.primaryPage != null }, true, 1500);370 primaryPageSpy.wait();
362 apl.visible = false;
363 apl = null;
364 }371 }
365372
366 function test_primaryPageSource_not_set_twice_data() {373 function test_change_primaryPageSource_data() {
367 return [374 return [
368 {tag: "Component", test: aplComponent, nextValue: null},375 {tag: "Component", test: pageComponent, nextValue: Qt.resolvedUrl("MyExternalPage.qml")},
369 {tag: "Document", test: aplDocument, nextValue: null},376 {tag: "Document", test: Qt.resolvedUrl("MyExternalPage.qml"), nextValue: pageComponent},
370 ];377 ];
371 }378 }
372 function test_primaryPageSource_not_set_twice(data) {379 function test_change_primaryPageSource(data) {
373 var apl = data.test.createObject(root);380 primaryPageSpy.target = defaults;
374 verify(apl);381 verify(defaults.primaryPage == null);
375 tryCompareFunction(function () { return apl.primaryPage != null }, true, 1500);382 verify(defaults.primaryPageSource == undefined);
376 ignoreWarning("Cannot change primaryPageSource after completion.");383 defaults.primaryPageSource = data.test;
377 apl.primaryPageSource = data.nextValue;384 primaryPageSpy.wait(400);
378 verify(apl.primaryPageSource != data.nextValue, "property value changed!");385 // add some pages
379 apl.visible = false;386 defaults.addPageToCurrentColumn(defaults.primaryPage, otherPage2);
380 apl = null;387 // then replace the primaryPageSource
381 }388 primaryPageSpy.clear();
382389 defaults.primaryPageSource = data.nextValue;
383 SignalSpy {390 primaryPageSpy.wait(400);
384 id: primaryPageSpy391 // look after page2
385 signalName: "primaryPageChanged"392 verify(!findPageFromLayout(defaults, "Page2"), "Page2 still in the view!");
386 }393 }
387394
388 function test_primaryPageSource_precedence_over_primaryPage() {395 function test_primaryPageSource_precedence_over_primaryPage() {
389 var apl = aplPrecedence.createObject(root);396 primaryPageSpy.target = defaults;
390 primaryPageSpy.target = apl;397 defaults.primaryPage = otherPage1;
391 verify(apl);398 primaryPageSpy.wait(400);
392 primaryPageSpy.wait();399 // now set a value to primaryPageSource
393 verify(apl.primaryPage.title != page1.title, "primaryPage has not been overloaded by primaryPageSource");400 primaryPageSpy.clear();
394 apl.visible = false;401 defaults.primaryPageSource = pageComponent;
395 apl = null;402 primaryPageSpy.wait(400);
403 }
404
405 function test_primaryPage_change_clears_primaryPageSource() {
406 primaryPageSpy.target = defaults;
407 defaults.primaryPageSource = pageComponent;
408 primaryPageSpy.wait(400);
409 compare(defaults.primaryPage.title, "DynamicPage", "DynamicPage not set as primaryPage");
410 // now set a value to primaryPage
411 primaryPageSpy.clear();
412 defaults.primaryPage = otherPage1;
413 primaryPageSpy.wait(400);
414 compare(defaults.primaryPageSource, undefined, "primaryPageSource must be cleared");
396 }415 }
397 }416 }
398}417}

Subscribers

People subscribed via source and target branches