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

[Zsombor Egri]

Make primaryPage and primaryPageSource replaceable after component completion.

Make primaryPage and primaryPageSource replaceable after component completion.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1665
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2334/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1062/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1064/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1061/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2334/rebuild

review: Needs Fixing (continuous-integration)

[lp:~zsombi/ubuntu-ui-toolkit/aplReplacePrimaryPage]

1666. By Zsombor Egri on 2015-09-30

documentation fixed

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1666
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2336/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1064
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1066
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1066/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1063

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2336/rebuild

review: Approve (continuous-integration)

[Cris Dywan (kalikiana)]

+ 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

[Zsombor Egri (zsombi)]

> + 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...

[lp:~zsombi/ubuntu-ui-toolkit/aplReplacePrimaryPage]

1667. By Zsombor Egri on 2015-10-02

review comments applied

1668. By Zsombor Egri on 2015-10-02

findPageFromLayoutWithProperty renamed into findPageFromLayout

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1668
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2357/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1085
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1087
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1087/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1084

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2357/rebuild

review: Approve (continuous-integration)

[Cris Dywan (kalikiana)]

Thanks for the update!

review: Approve

=== 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 @@
   add the page to the leftmost column of the view.
 
   The primary page, the very first page must be specified either through the
-  \l primaryPage or \l primaryPageSource properties. The properties cannot be
-  changed after component completion. \l primaryPage can only hold a Page instance,
-  \l primaryPageSource can either be a Component or a url to a document defining
-  a Page. This page cannot be removed from the view. \l primaryPageSource has
-  precedence over \l primaryPage and will create the Page asynchronously. The
-  page instance will be reported through \l primaryPage property and will replace
-  any previous value set to that property.
+  \l primaryPage or \l primaryPageSource properties. \l primaryPage can only
+  hold a Page instance, \l primaryPageSource can either be a Component or a
+  url to a document defining a Page. \l primaryPageSource has precedence over
+  \l primaryPage, and when set it will report the loaded Page through \l primaryPage
+  property, and will replace any value set into that property.
 
   \qml
     import QtQuick 2.4
@@ -188,17 +186,18 @@
     /*!
       The property holds the first Page which will be added to the view. If the
       view has more than one column, the page will be added to the leftmost column.
-      The property can hold only a Page instance. The property cannot be changed after
-      component completion.
+      The property can only hold a Page instance. When changed runtime (not by the
+      AdaptivePageLayout component itself), the \l primaryPageSource property
+      will be reset.
       */
     property Page primaryPage
 
     /*!
       The property specifies the source of the primaryPage in case the primary
       page is created from a Component or loaded from an external document. It
-      has precedence over \l primaryPage and cannot be changed after component
-      completion. The page specified in this way will be cerated asynchronously
-      and the instance will be reported through \l primaryPage property.
+      has precedence over \l primaryPage. The page specified in this way will
+      be cerated asynchronously and the instance will be reported through
+      \l primaryPage property.
       */
     property var primaryPageSource
 
@@ -322,12 +321,7 @@
       pages will be removed.
       */
     function removePages(page) {
-        var nodeToRemove = d.getWrapper(page);
-        var removedNodes = d.tree.chop(nodeToRemove, page != layout.primaryPage);
-        for (var i = removedNodes.length-1; i >= 0; i--) {
-            var node = removedNodes[i];
-            d.updatePageForColumn(node.column);
-        }
+        d.removeAllPages(page, page != layout.primaryPage);
     }
 
     /*
@@ -345,26 +339,34 @@
             d.createPrimaryPage(primaryPageSource);
         } else if (primaryPage) {
             d.createPrimaryPage(primaryPage);
-        } else {
-            console.warn("No primary page set. No pages can be added without a primary page.");
         }
         d.completed = true;
     }
     onPrimaryPageChanged: {
-        if (d.completed && !d.internalUpdate) {
-            console.warn("Cannot change primaryPage after completion.");
-            d.internalPropertyUpdate("primaryPage", d.lastPrimaryPage);
+        if (!d.completed || d.internalUpdate) {
             return;
         }
-        d.lastPrimaryPage = primaryPage;
+        // reset the primaryPageSource
+        d.internalPropertyUpdate("primaryPageSource", undefined);
+        // clear the layout
+        d.purgeLayout();
+        // add the new page if valid
+        if (primaryPage !== null) {
+            d.createPrimaryPage(primaryPage);
+        }
     }
     onPrimaryPageSourceChanged: {
-        if (d.completed && !d.internalUpdate) {
-            console.warn("Cannot change primaryPageSource after completion.");
-            d.internalPropertyUpdate("primaryPageSource", d.lastPrimaryPageSource);
+        if (!d.completed || d.internalUpdate) {
             return;
         }
-        d.lastPrimaryPageSource = primaryPageSource;
+        // remove all pages first
+        d.purgeLayout();
+        // create the new primary page if a valid component is specified
+        if (primaryPageSource) {
+            d.createPrimaryPage(primaryPageSource);
+        } else {
+            d.internalPropertyUpdate("primaryPage", null);
+        }
     }
 
     onLayoutsChanged: {
@@ -389,8 +391,7 @@
                                   (activeLayout ? activeLayout.data.length : 1)
         property PageColumnsLayout activeLayout: null
         property list<PageColumnsLayout> prevLayouts
-        property Page lastPrimaryPage
-        property var lastPrimaryPageSource
+        property Page prevPrimaryPage
 
         /*! internal */
         onColumnsChanged: {
@@ -416,10 +417,20 @@
                 wrapper.incubator.onStatusChanged = function (status) {
                     if (status == Component.Ready) {
                         internalPropertyUpdate("primaryPage", wrapper.incubator.object);
+                        prevPrimaryPage = wrapper.incubator.object;
                     }
                 }
             } else {
                 finalizeAddingPage(wrapper);
+                prevPrimaryPage = wrapper.object;
+            }
+        }
+
+        // remove all pages, including primaryPage
+        function purgeLayout() {
+            if (prevPrimaryPage) {
+                removeAllPages(prevPrimaryPage, true);
+                prevPrimaryPage = null;
             }
         }
 
@@ -514,6 +525,17 @@
             return newWrapper.incubator;
         }
 
+        // removes all pages from the layout, and may include the page itself
+        function removeAllPages(page, inclusive) {
+            inclusive = typeof inclusive !== 'undefined' ? inclusive : true;
+            var nodeToRemove = d.getWrapper(page);
+            var removedNodes = d.tree.chop(nodeToRemove, inclusive);
+            for (var i = removedNodes.length-1; i >= 0; i--) {
+                var node = removedNodes[i];
+                updatePageForColumn(node.column);
+            }
+        }
+
         // update the page for the specified column
         function updatePageForColumn(column) {
             var effectiveColumn = MathUtils.clamp(column, 0, d.columns - 1);

=== 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 @@
     // 2 on desktop, 1 on phone.
     property int columns: width >= units.gu(80) ? 2 : 1
 
-    AdaptivePageLayout {
-        id: layout
-        width: parent.width
-        height: parent.height
-
-        primaryPage: page1
-
-        Page {
-            id: page1
-            title: "Page1"
-
-            Column {
-                anchors.centerIn: parent
-                width: childrenRect.width
-                Button {
-                    text: "Page 2 left"
-                    onTriggered: layout.addPageToCurrentColumn(page1, page2)
-                }
-                Button {
-                    text: "Page 3 right"
-                    onTriggered: layout.addPageToNextColumn(page1, page3);
-                }
-            }
-        }
-        Page {
-            id: page2
-            title: "Page2"
-        }
-        Page {
-            id: page3
-            title: "Page3"
-        }
-        Page {
-            id: page4
-            title: "Page4"
-        }
-    }
-
-    AdaptivePageLayout {
-        id: defaults
+    Column {
+        anchors.fill: parent
+        AdaptivePageLayout {
+            id: layout
+            width: parent.width
+            height: parent.height / 2
+
+            primaryPage: page1
+
+            Page {
+                id: page1
+                objectName: title
+                title: "Page1"
+
+                Column {
+                    anchors.centerIn: parent
+                    width: childrenRect.width
+                    Button {
+                        text: "Page 2 left"
+                        onTriggered: layout.addPageToCurrentColumn(page1, page2)
+                    }
+                    Button {
+                        text: "Page 3 right"
+                        onTriggered: layout.addPageToNextColumn(page1, page3);
+                    }
+                }
+            }
+            Page {
+                id: page2
+                objectName: title
+                title: "Page2"
+            }
+            Page {
+                id: page3
+                objectName: title
+                title: "Page3"
+            }
+            Page {
+                id: page4
+                objectName: title
+                title: "Page4"
+            }
+        }
+        AdaptivePageLayout {
+            id: defaults
+            width: parent.width
+            height: parent.height / 2
+            Page {
+                id: otherPage1
+                objectName: title
+                title: "Page1"
+            }
+            Page {
+                id: otherPage2
+                objectName: title
+                title: "Page2"
+            }
+            Page {
+                id: otherPage3
+                objectName: title
+                title: "Page3"
+            }
+        }
     }
 
     Component {
         id: pageComponent
         Page {
+            objectName: title
             title: "DynamicPage"
         }
     }
 
-    Component {
-        id: aplComponent
-        AdaptivePageLayout {
-            width: units.gu(40)
-            height: units.gu(50)
-            primaryPageSource: pageComponent
-        }
-    }
-
-    Component {
-        id: aplDocument
-        AdaptivePageLayout {
-            width: units.gu(40)
-            height: units.gu(50)
-            primaryPageSource: Qt.resolvedUrl("MyExternalPage.qml")
-        }
-    }
-
-    Component {
-        id: aplPrecedence
-        AdaptivePageLayout {
-            width: units.gu(40)
-            height: units.gu(50)
-            primaryPage: page1
-            primaryPageSource: Qt.resolvedUrl("MyExternalPage.qml")
-        }
-    }
-
     UbuntuTestCase {
         id: testCase
         when: windowShown
@@ -115,6 +111,10 @@
             target: testCase
             signalName: "pageLoaded"
         }
+        SignalSpy {
+            id: primaryPageSpy
+            signalName: "primaryPageChanged"
+        }
 
         function resize_single_column() {
             layout.width = units.gu(40);
@@ -135,29 +135,38 @@
             return apl.__d.getWrapper(page);
         }
 
+        function findPageFromLayout(apl, objectName) {
+            var body = findChild(apl, "body");
+            verify(body);
+            return findChild(body, objectName);
+        }
+
         function cleanup() {
             page1.title = "Page1";
             page2.title = "Page2";
             page3.title = "Page3";
             page4.title = "Page4";
             loadedSpy.clear();
+            primaryPageSpy.clear();
+            primaryPageSpy.target = null;
             resize_multiple_columns();
-            layout.removePages(page1);
+            layout.removePages(layout.primaryPage);
+            defaults.primaryPage = null;
+            wait(200);
         }
 
         function initTestCase() {
             compare(defaults.primaryPage, null, "primaryPage not null by default");
             compare(defaults.primaryPageSource, undefined, "primaryPageSource not set by default");
             compare(defaults.layouts.length, 0, "no layouts by default");
-            compare(defaults.columns, 1, "1 column as default");
+            compare(defaults.columns, columns, columns + " column(s) as default");
         }
 
         function test_change_primaryPage() {
-            // this prints the warning but still changes the primary page,
-            //  so the test must be executed last not to mess up the other tests.
-            ignoreWarning("Cannot change primaryPage after completion.");
-            layout.primaryPage = page3;
-            verify(layout.primaryPage != page3, "primaryPage value was changed");
+            defaults.primaryPage = otherPage1;
+            defaults.addPageToCurrentColumn(defaults.primaryPage, otherPage2);
+            defaults.primaryPage = otherPage3;
+            verify(!findPageFromLayout(defaults, "Page2"), "Page2 still in the view!");
         }
 
         function test_add_page_when_source_page_not_in_stack() {
@@ -333,10 +342,10 @@
             }
 
             var testPage = testHolder.pageWrapper.object;
-            var prevPageActive = false
+            var prevPageActive = false;
             var incubator = data.nextColumn
                         ? layout.addPageToNextColumn(data.sourcePage, data.page)
-                        : layout.addPageToCurrentColumn(data.sourcePage, data.page);;
+                        : layout.addPageToCurrentColumn(data.sourcePage, data.page);
             verify(incubator);
             compare(testHolder.pageWrapper.object, testPage);
             incubator.onStatusChanged = function (status) {
@@ -351,48 +360,58 @@
 
         function test_primaryPageSource_bug1499179_data() {
             return [
-                {tag: "Component", test: aplComponent},
-                {tag: "Document", test: aplDocument},
+                {tag: "Component", test: pageComponent},
+                {tag: "Document", test: Qt.resolvedUrl("MyExternalPage.qml")},
             ];
         }
         function test_primaryPageSource_bug1499179(data) {
-            var apl = data.test.createObject(root);
-            verify(apl);
-            tryCompareFunction(function () { return apl.primaryPage != null }, true, 1500);
-            apl.visible = false;
-            apl = null;
+            primaryPageSpy.target = defaults;
+            defaults.primaryPageSource = data.test;
+            primaryPageSpy.wait();
         }
 
-        function test_primaryPageSource_not_set_twice_data() {
+        function test_change_primaryPageSource_data() {
             return [
-                {tag: "Component", test: aplComponent, nextValue: null},
-                {tag: "Document", test: aplDocument, nextValue: null},
+                {tag: "Component", test: pageComponent, nextValue: Qt.resolvedUrl("MyExternalPage.qml")},
+                {tag: "Document", test: Qt.resolvedUrl("MyExternalPage.qml"), nextValue: pageComponent},
             ];
         }
-        function test_primaryPageSource_not_set_twice(data) {
-            var apl = data.test.createObject(root);
-            verify(apl);
-            tryCompareFunction(function () { return apl.primaryPage != null }, true, 1500);
-            ignoreWarning("Cannot change primaryPageSource after completion.");
-            apl.primaryPageSource = data.nextValue;
-            verify(apl.primaryPageSource != data.nextValue, "property value changed!");
-            apl.visible = false;
-            apl = null;
-        }
-
-        SignalSpy {
-            id: primaryPageSpy
-            signalName: "primaryPageChanged"
+        function test_change_primaryPageSource(data) {
+            primaryPageSpy.target = defaults;
+            verify(defaults.primaryPage == null);
+            verify(defaults.primaryPageSource == undefined);
+            defaults.primaryPageSource = data.test;
+            primaryPageSpy.wait(400);
+            // add some pages
+            defaults.addPageToCurrentColumn(defaults.primaryPage, otherPage2);
+            // then replace the primaryPageSource
+            primaryPageSpy.clear();
+            defaults.primaryPageSource = data.nextValue;
+            primaryPageSpy.wait(400);
+            // look after page2
+            verify(!findPageFromLayout(defaults, "Page2"), "Page2 still in the view!");
         }
 
         function test_primaryPageSource_precedence_over_primaryPage() {
-            var apl = aplPrecedence.createObject(root);
-            primaryPageSpy.target = apl;
-            verify(apl);
-            primaryPageSpy.wait();
-            verify(apl.primaryPage.title != page1.title, "primaryPage has not been overloaded by primaryPageSource");
-            apl.visible = false;
-            apl = null;
+            primaryPageSpy.target = defaults;
+            defaults.primaryPage = otherPage1;
+            primaryPageSpy.wait(400);
+            // now set a value to primaryPageSource
+            primaryPageSpy.clear();
+            defaults.primaryPageSource = pageComponent;
+            primaryPageSpy.wait(400);
+        }
+
+        function test_primaryPage_change_clears_primaryPageSource() {
+            primaryPageSpy.target = defaults;
+            defaults.primaryPageSource = pageComponent;
+            primaryPageSpy.wait(400);
+            compare(defaults.primaryPage.title, "DynamicPage", "DynamicPage not set as primaryPage");
+            // now set a value to primaryPage
+            primaryPageSpy.clear();
+            defaults.primaryPage = otherPage1;
+            primaryPageSpy.wait(400);
+            compare(defaults.primaryPageSource, undefined, "primaryPageSource must be cleared");
         }
     }
 }