Merge lp:~aacid/unity8/dash_ranges_on_move into lp:unity8

[Albert Astals Cid]

Make sure changing a scope doesn't trigger creation/destruction of delegates until it's finished

 * Are there any related MPs required for this MP to build/function as expected?
No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * Did you make sure that your branch does not contain spurious tags?
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

[Michał Sawicz (saviq)]

This test fails for me:

119 + verify(compareArrays(buttons2, buttons));

I've looked at the arrays and buttons kept about twice (~1200) as many items as buttons2 (~600).

It seems like a timing issue - i.e. we're creating too many items on startup?

review: Needs Fixing

[Michał Sawicz (saviq)]

Testing this manually (patched a live phone) and this doesn't seem to be doing what it advertises. I added some debug lines in onCompleted and onDestruction and I'm still getting a lot of those when switching scopes to the sides (and I can see stuttering, too).

What's also interesting is that I've seen at least a few destructions on startup, which we should avoid if at all possible.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1545
http://jenkins.qa.ubuntu.com/job/unity8-ci/5142/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/782
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/306
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/681
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/780
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/780/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17026

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5142/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

OK there was some misunderstanding, it does indeed keep the margins and caches static *while* swiping. The creations I was seeing were due to the plugin querying next-to-adjacent scopes.

This does, however, fix the case when all the scopes are already loaded and you're swiping between them.

review: Approve

[Michał Sawicz (saviq)]

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Y
 * Did CI run pass? If not, please explain why.
N, unstable tests :/
 * Did you make sure that the branch does not contain spurious tags?
Y

review: Approve

[lp:~aacid/unity8/dash_ranges_on_move]

1546. By Albert Astals Cid on 2015-01-13

Increase the busy loop a bit

Busy loops are nasty, but can't think of another way to test this at the moment

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1546
http://jenkins.qa.ubuntu.com/job/unity8-ci/5143/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/786
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/308
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/308
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/784
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/784/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17035

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5143/rebuild

review: Needs Fixing (continuous-integration)

=== modified file 'qml/Dash/GenericScopeView.qml'
--- qml/Dash/GenericScopeView.qml	2015-01-09 10:40:33 +0000
+++ qml/Dash/GenericScopeView.qml	2015-01-13 14:37:22 +0000
@@ -41,6 +41,7 @@
     property bool visibleToParent: false
     property alias pageHeaderTotallyVisible: categoryView.pageHeaderTotallyVisible
     property var holdingList: null
+    property bool wasCurrentOnMoveStart: false
 
     property var scopeStyle: ScopeStyle {
         style: scope ? scope.customizations : {}
@@ -116,6 +117,9 @@
     }
 
     onIsCurrentChanged: {
+        if (!holdingList || !holdingList.moving) {
+            wasCurrentOnMoveStart = scopeView.isCurrent;
+        }
         if (pageHeaderLoader.item && showPageHeader) {
             pageHeaderLoader.item.resetSearch();
         }
@@ -142,6 +146,15 @@
         onHideDash: subPageLoader.closeSubPage()
     }
 
+    Connections {
+        target: holdingList
+        onMovingChanged: {
+            if (!moving) {
+                wasCurrentOnMoveStart = scopeView.isCurrent;
+            }
+        }
+    }
+
     Rectangle {
         anchors.fill: parent
         color: scopeView.scopeStyle ? scopeView.scopeStyle.background : "transparent"
@@ -367,7 +380,11 @@
 
                 function updateRanges() {
                     // Don't want to create stress by requesting more items during scope
-                    // changes so unless you're not part of the visible scopes just return
+                    // changes so unless you're not part of the visible scopes just return.
+                    // For the visible scopes we need to do some work, the previously non visible
+                    // scope needs to adjust its ranges so that we define the new visible range,
+                    // that still means no creation/destruction of delegates, it's just about changing
+                    // the culling of the items so they are actually visible
                     if (holdingList && holdingList.moving && !scopeView.visibleToParent) {
                         return;
                     }
@@ -412,7 +429,18 @@
                         if (scopeView.isCurrent || scopeView.visibleToParent) {
                             item.displayMarginBeginning = displayMarginBeginning;
                             item.displayMarginEnd = displayMarginEnd;
-                            item.cacheBuffer = scopeView.isCurrent ? categoryView.height * 1.5 : 0;
+                            if (holdingList && holdingList.moving) {
+                                // If we are moving we need to reset the cache buffer of the
+                                // view that was not visible (i.e. !wasCurrentOnMoveStart) to 0 since
+                                // otherwise the cache buffer we had set to preload the items of the
+                                // visible range will trigger some item creations and we want move to
+                                // be as smooth as possible meaning no need creations
+                                if (!wasCurrentOnMoveStart) {
+                                    item.cacheBuffer = 0;
+                                }
+                            } else {
+                                item.cacheBuffer = categoryView.height * 1.5;
+                            }
                         } else {
                             var visibleRange = baseItem.height + displayMarginEnd + displayMarginBeginning;
                             if (visibleRange < 0) {

=== modified file 'tests/qmltests/Dash/tst_DashContent.qml'
--- tests/qmltests/Dash/tst_DashContent.qml	2015-01-09 10:42:42 +0000
+++ tests/qmltests/Dash/tst_DashContent.qml	2015-01-13 14:37:22 +0000
@@ -537,5 +537,62 @@
 
             compare(categoryListView.pageHeader.item.searchHint, "Search People");
         }
+
+        function compareArrays(a, b) {
+            if (a.length != b.length) return false;
+            for (var i in a) {
+                if (a[i] != b[i]) return false;
+            }
+            return true;
+        }
+
+        function getSettledButtons() {
+            var buttons = findChildsByType(dashContent, "AbstractButton");
+            wait(2500);
+            var aux = findChildsByType(dashContent, "AbstractButton");
+            while (!compareArrays(aux, buttons)) {
+                buttons = aux;
+                wait(2500);
+                aux = findChildsByType(dashContent, "AbstractButton");
+            }
+            return buttons;
+        }
+
+        function test_noDelegateCreationDestructionOnMove() {
+            // Our cards are of type AbstractButton as defined in CardCreator.js
+            // This gives also other things that are not cards but for our purpose it
+            // does not matter
+
+            // Wait for the buttons to settle
+            var buttons = getSettledButtons();
+
+            // Move the scopes so that the item on the right is the current one
+            // without releasing the button
+            mouseFlick(dashContent, dashContent.width - units.gu(1), units.gu(1), units.gu(1), units.gu(1), true, false);
+
+            // Make sure we have changed to a new scope
+            compare(dashContent.currentIndex, 1);
+
+            // Wait for the buttons to settle
+            var buttons2 = getSettledButtons();
+
+            // Verify we have exactly the same buttons as before starting to move
+            verify(compareArrays(buttons2, buttons));
+
+            // Release the mouse
+            mouseRelease(dashContent, units.gu(1), units.gu(1));
+
+            // Wait for the scopes list to stop moving
+            var dashContentList = findChild(dashContent, "dashContentList");
+            tryCompare(dashContentList, "moving", false);
+            compare(dashContent.currentIndex, 1);
+
+            // Wait for the buttons to settle
+            var buttons3 = getSettledButtons();
+
+            // Verify we have a different set of buttons now
+            expectFail("", "There has to be new cards after releasing the list is not moving anymore");
+            verify(compareArrays(buttons3, buttons));
+        }
     }
 }

=== modified file 'tests/utils/modules/Unity/Test/UnityTestCase.qml'
--- tests/utils/modules/Unity/Test/UnityTestCase.qml	2015-01-09 10:42:42 +0000
+++ tests/utils/modules/Unity/Test/UnityTestCase.qml	2015-01-13 14:37:22 +0000
@@ -170,6 +170,18 @@
         return null;
     }
 
+    function findChildsByType(obj, typeName) {
+        var res = new Array(0);
+        for (var i in obj.children) {
+            var c = obj.children[i];
+            if (UT.Util.isInstanceOf(c, typeName)) {
+                res.push(c)
+            }
+            res = res.concat(findChildsByType(c, typeName));
+        }
+        return res;
+    }
+
     // Type a full string instead of keyClick letter by letter
     // TODO: this is not ugly, this is uber-ugly and does not support
     // any special character. Remove the keyMap once keyClick(obj, char)