Unity 8

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

listviewforreviews
Merge into trunk

Proposed by Albert Astals Cid on 2015-11-13

Status:

Merged

Approved by:

Michael Zanetti on 2015-12-01

Approved revision:

2045

Merged at revision:

2084

Proposed branch:

lp:~aacid/unity8/listviewforreviews

Merge into:

lp:unity8

Diff against target:

293 lines (+179/-25)

8 files modified

qml/Dash/Previews/Preview.qml (+2/-0)
qml/Dash/Previews/PreviewRatingDisplay.qml (+72/-21)
qml/Dash/Previews/PreviewWidget.qml (+3/-0)
qml/Dash/Previews/PreviewWidgetFactory.qml (+4/-0)
tests/qmltests/CMakeLists.txt (+1/-0)
tests/qmltests/Dash/Previews/tst_PreviewRatingDisplay.qml (+1/-3)
tests/qmltests/Dash/Previews/tst_PreviewRatingDisplayCreationRanges.qml (+94/-0)
tests/utils/modules/Unity/Test/UnityTestCase.qml (+2/-1)

To merge this branch:

bzr merge lp:~aacid/unity8/listviewforreviews

Related bugs:

Bug #1492214: All preview comments are created at the same time	High	Fix Released
Bug #1519898: Slow scrolling and high cpu usage in app preview	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Zanetti (community)		2015-11-13	Approve on 2015-12-01
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-11-27
Review via email: mp+277428@code.launchpad.net

Commit Message

Create ratings on demand instead of all at the same time

Description of the Change

* 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

PS Jenkins bot (ps-jenkins) wrote on 2015-11-16:

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

review: Needs Fixing (continuous-integration)

Michael Zanetti (mzanetti) wrote on 2015-11-25:

Improves the issue a lot. Code looks reasonable.

Do you think it is worth writing some test for this? Shouldn't be too hard to open a fake preview with tons of entries, making sure only very few are created, flicking to the bottom and making sure all of them are created. Would at least check if noone breaks it by making it perform bad again, or by preventing things from being built up when required.

Michael Zanetti (mzanetti) on 2015-11-25:

review: Needs Information

lp:~aacid/unity8/listviewforreviews updated on 2015-11-26

2040. By Albert Astals Cid on 2015-11-26: Merge
2041. By Albert Astals Cid on 2015-11-26: Do not use 5 max iterations hardcoded in flickToYEnd, makes no sense
2042. By Albert Astals Cid on 2015-11-26: Rename parentList to parentFlickable
2043. By Albert Astals Cid on 2015-11-26: Add test for the creation ranges on rating display creation

Albert Astals Cid (aacid) wrote on 2015-11-26:

Test added

PS Jenkins bot (ps-jenkins) wrote on 2015-11-26:

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

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-11-26:

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

review: Needs Fixing (continuous-integration)

lp:~aacid/unity8/listviewforreviews updated on 2015-11-27

2044. By Albert Astals Cid on 2015-11-27: beware of scary eol

PS Jenkins bot (ps-jenkins) wrote on 2015-11-27:

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

review: Needs Fixing (continuous-integration)

lp:~aacid/unity8/listviewforreviews updated on 2015-11-27

2045. By Albert Astals Cid on 2015-11-27

Make test pass again

There's no need to check the repeater.count (that doesn't exist anymore)
since we're checking the existance of "reviewItem" + index later in the loop anyway

PS Jenkins bot (ps-jenkins) wrote on 2015-11-27:

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

review: Needs Fixing (continuous-integration)

Michael Zanetti (mzanetti) wrote on 2015-12-01:

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

yes

* Did you make sure that the branch does not contain spurious tags?

yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Albert Astals Cid

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Dash/Previews/Preview.qml'
 --- qml/Dash/Previews/Preview.qml	2015-11-04 14:57:13 +0000
 +++ qml/Dash/Previews/Preview.qml	2015-11-27 10:34:17 +0000
@@ -90,6 +90,8 @@
                      widgetData: model.properties
                      isCurrentPreview: root.isCurrent
                      scopeStyle: root.scopeStyle
++                    parentFlickable: column
++
                      anchors {
                          left: parent.left
                          right: parent.right
 === modified file 'qml/Dash/Previews/PreviewRatingDisplay.qml'
 --- qml/Dash/Previews/PreviewRatingDisplay.qml	2015-08-13 09:08:15 +0000
 +++ qml/Dash/Previews/PreviewRatingDisplay.qml	2015-11-27 10:34:17 +0000
@@ -40,28 +40,79 @@
      id: root
      implicitHeight: childrenRect.height
--    Column {
++    onIsCurrentPreviewChanged: ratingsList.updateRanges();
++    onParentFlickableChanged: ratingsList.updateRanges();
++
++    Connections {
++        target: parentFlickable
++        onOriginYChanged: ratingsList.updateRanges();
++        onContentYChanged: ratingsList.updateRanges();
++        onHeightChanged: ratingsList.updateRanges();
++        onContentHeightChanged: ratingsList.updateRanges();
++    }
++
++    ListView {
++        id: ratingsList
          anchors { left: parent.left; right: parent.right; }
--        visible: reviewsRepeater.count > 0
--
--        Repeater {
--            id: reviewsRepeater
--            objectName: "reviewsRepeater"
--            model: root.widgetData["reviews"]
--
--            delegate: PreviewRatingSingleDisplay {
--                objectName: "reviewItem" + index
--
--                anchors { left: parent.left; right: parent.right; }
--
--                rating: modelData["rating"] || -1
--                author: modelData["author"] || ""
--                review: modelData["review"] || ""
--                urlIconEmpty: widgetData["rating-icon-empty"]
--                urlIconFull: widgetData["rating-icon-full"]
--                urlIconHalf: widgetData["rating-icon-half"]
--                labelColor: scopeStyle ? scopeStyle.foreground : theme.palette.normal.baseText
--            }
++        height: contentHeight
++        interactive: false
++
++        model: root.widgetData["reviews"]
++
++        delegate: PreviewRatingSingleDisplay {
++            id: prsd
++            objectName: "reviewItem" + index
++
++            anchors { left: parent.left; right: parent.right; }
++
++            rating: modelData["rating"] || -1
++            author: modelData["author"] || ""
++            review: modelData["review"] || ""
++            urlIconEmpty: widgetData["rating-icon-empty"]
++            urlIconFull: widgetData["rating-icon-full"]
++            urlIconHalf: widgetData["rating-icon-half"]
++            labelColor: scopeStyle ? scopeStyle.foreground : theme.palette.normal.baseText
++        }
++
++        onContentHeightChanged: ratingsList.updateRanges();
++
++        function updateRanges() {
++            var baseItem = root.parent;
++            if (!parentFlickable || !baseItem) {
++                ratingsList.displayMarginBeginning = 0;
++                ratingsList.displayMarginEnd = 0;
++                return;
++            }
++
++            if (parentFlickable.moving) {
++                // Do not update the range if we are overshooting up or down, since we'll come back
++                // to the stable position and delete/create items without any reason
++                if (parentFlickable.contentY < parentFlickable.originY) {
++                    return;
++                } else if (parentFlickable.contentHeight - parentFlickable.originY > parentFlickable.height &&
++                            parentFlickable.contentY + parentFlickable.height > parentFlickable.contentHeight) {
++                    return;
++                }
++            }
++
++            // A item view creates its delegates synchronously from
++            //     -displayMarginBeginning
++            // to
++            //     height + displayMarginEnd
++            // Around that area it adds the cacheBuffer area where delegates are created async
++            //
++            // We adjust displayMarginEnd to be negative so that the range of created delegates matches
++            // from the beginning of the list to the end of the viewport.
++            // Ideally we would also use displayMarginBeginning
++            // so that delegates at the beginning get destroyed but that causes issues with
++            // listview and is not really necessary to provide the better experience we're after
++            var itemYOnViewPort = baseItem.y - parentFlickable.contentY;
++            var displayMarginEnd = -ratingsList.contentHeight + parentFlickable.height - itemYOnViewPort;
++            displayMarginEnd = -Math.max(-displayMarginEnd, 0);
++            displayMarginEnd = -Math.min(-displayMarginEnd, ratingsList.contentHeight);
++            displayMarginEnd = Math.round(displayMarginEnd);
++
++            ratingsList.displayMarginEnd = displayMarginEnd;
+         }
+     }
+ }
 === modified file 'qml/Dash/Previews/PreviewWidget.qml'
 --- qml/Dash/Previews/PreviewWidget.qml	2015-11-04 14:57:13 +0000
 +++ qml/Dash/Previews/PreviewWidget.qml	2015-11-27 10:34:17 +0000
@@ -37,6 +37,9 @@
      //! Set margins width.
      property real widgetMargins: units.gu(1)
++    /// The parent (vertical) flickable this widget is in (if any)
++    property var parentFlickable: null
++
      /*! \brief This signal should be emitted when a preview action was triggered.
+      *
       *  \param widgetId, actionId Respective identifiers from widgetData.
 === modified file 'qml/Dash/Previews/PreviewWidgetFactory.qml'
 --- qml/Dash/Previews/PreviewWidgetFactory.qml	2015-11-04 14:57:13 +0000
 +++ qml/Dash/Previews/PreviewWidgetFactory.qml	2015-11-27 10:34:17 +0000
@@ -43,6 +43,9 @@
      //! Set margins width.
      property real widgetMargins: status === Loader.Ready ? item.widgetMargins : units.gu(1)
++    /// The parent (vertical) flickable this widget is in (if any)
++    property var parentFlickable: null
++
      //! Triggered signal forwarded from the widgets.
      signal triggered(string widgetId, string actionId, var data)
@@ -82,6 +85,7 @@
          item.isCurrentPreview = Qt.binding(function() { return root.isCurrentPreview } )
          item.expanded = Qt.binding(function() { return root.expanded } )
          item.scopeStyle = Qt.binding(function() { return root.scopeStyle } )
++        item.parentFlickable = Qt.binding(function() { return root.parentFlickable } )
+     }
      Connections {
 === modified file 'tests/qmltests/CMakeLists.txt'
 --- tests/qmltests/CMakeLists.txt	2015-11-06 10:06:58 +0000
 +++ tests/qmltests/CMakeLists.txt	2015-11-27 10:34:17 +0000
@@ -41,6 +41,7 @@
  add_unity8_qmltest(Dash/Previews PreviewPayments)
  add_unity8_qmltest(Dash/Previews PreviewProgress)
  add_unity8_qmltest(Dash/Previews PreviewRatingDisplay)
++add_unity8_qmltest(Dash/Previews PreviewRatingDisplayCreationRanges)
  add_unity8_qmltest(Dash/Previews PreviewRatingEdit)
  add_unity8_qmltest(Dash/Previews PreviewRatingInput)
  add_unity8_qmltest(Dash/Previews PreviewSharing)
 === modified file 'tests/qmltests/Dash/Previews/tst_PreviewRatingDisplay.qml'
 --- tests/qmltests/Dash/Previews/tst_PreviewRatingDisplay.qml	2015-07-15 15:07:19 +0000
 +++ tests/qmltests/Dash/Previews/tst_PreviewRatingDisplay.qml	2015-11-27 10:34:17 +0000
@@ -70,9 +70,7 @@
          function test_reviews(data) {
              previewRatingDisplay.widgetData = data.reviewsModel;
--
--            var reviewsRepeater = findChild(previewRatingDisplay, "reviewsRepeater");
--            compare(reviewsRepeater.count, data.reviewsModel["reviews"].length);
++            waitForRendering(previewRatingDisplay);
              for (var i = 0; i < data.reviewsModel["reviews"].length; ++i) {
                  var reviewItem = findChild(previewRatingDisplay, "reviewItem" + i);
 === added file 'tests/qmltests/Dash/Previews/tst_PreviewRatingDisplayCreationRanges.qml'
 --- tests/qmltests/Dash/Previews/tst_PreviewRatingDisplayCreationRanges.qml	1970-01-01 00:00:00 +0000
 +++ tests/qmltests/Dash/Previews/tst_PreviewRatingDisplayCreationRanges.qml	2015-11-27 10:34:17 +0000
@@ -0,0 +1,94 @@
++/*
++ * Copyright 2014,2015 Canonical Ltd.
++ *
++ * This program is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++import QtQuick 2.4
++import QtTest 1.0
++import Ubuntu.Components 1.3
++import "../../../../qml/Dash/Previews"
++import Unity.Test 0.1 as UT
++
++Flickable {
++    id: root
++    width: units.gu(40)
++    height: units.gu(80)
++    contentHeight: factory.implicitHeight
++
++    property var reviewsEmpty: {
++        "reviews": [ ]
++    }
++
++    Item {
++        id: widgetData500
++        property var reviews: []
++    }
++
++    PreviewWidgetFactory {
++        id: factory
++        anchors.fill: parent
++        widgetType: "reviews"
++        parentFlickable: root
++    }
++
++    Component.onCompleted: {
++        for (var i = 0; i < 500; ++i) {
++            widgetData500.reviews.push({ author: "Some dude", review: "Very cool app" + i });
++        }
++        factory.widgetData = widgetData500;
++    }
++
++    UT.UnityTestCase {
++        name: "PreviewRatingDisplayTest"
++        when: windowShown
++
++        function test_creation_speed() {
++            factory.parentFlickable = null;
++            factory.widgetData = reviewsEmpty;
++            waitForRendering(root);
++
++            var start = new Date().getTime();
++            factory.widgetData = widgetData500;
++            waitForRendering(root);
++            var end = new Date().getTime();
++            var time500 = end - start;
++
++            factory.widgetData = reviewsEmpty;
++            waitForRendering(root);
++
++            factory.parentFlickable = root;
++            start = new Date().getTime();
++            factory.widgetData = widgetData500;
++            waitForRendering(root);
++            end = new Date().getTime();
++            var timeRanges500 = end - start;
++
++            // Measurements show it's usually like 20 times faster
++            // but we set the range at 8 times fast to make sure it's not an unstable test
++            verify(timeRanges500 * 8 < time500);
++        }
++
++        function test_check_indexes() {
++            // Check that item 499 isn't there on startup but if we scroll down
++            // it will be there
++            var reviewItem499 = findChild(factory, "reviewItem499");
++            verify(reviewItem499 === null);
++
++            flickToYEnd(root);
++
++            var reviewItem499 = findChild(factory, "reviewItem499");
++            verify(reviewItem499 !== null);
++        }
++    }
++}
 === modified file 'tests/utils/modules/Unity/Test/UnityTestCase.qml'
 --- tests/utils/modules/Unity/Test/UnityTestCase.qml	2015-08-13 09:08:15 +0000
 +++ tests/utils/modules/Unity/Test/UnityTestCase.qml	2015-11-27 10:34:17 +0000
@@ -239,7 +239,8 @@
          var x = item.width / 2;
          var y = item.height - units.gu(1);
          var toY = units.gu(1);
--        while (i < 5 && !item.atYEnd) {
++        var maxIterations = 5 + item.contentHeight / item.height;
++        while (i < maxIterations && !item.atYEnd) {
              touchFlick(item, x, y, x, toY);
              tryCompare(item, "moving", false);
              ++i;