Unity 8

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

carouselLastItemClick
Merge into trunk

Proposed by Albert Astals Cid on 2014-04-04

Status:

Merged

Approved by:

Andrea Cimitan on 2014-04-11

Approved revision:

813

Merged at revision:

842

Proposed branch:

lp:~aacid/unity8/carouselLastItemClick

Merge into:

lp:unity8

Diff against target:

145 lines (+43/-25)

2 files modified

qml/Components/Carousel.qml (+16/-22)
tests/qmltests/Components/tst_Carousel.qml (+27/-3)

To merge this branch:

bzr merge lp:~aacid/unity8/carouselLastItemClick

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2014-04-11
Andrea Cimitan (community)			Approve on 2014-04-11
Michał Sawicz		2014-04-04	Needs Information on 2014-04-09
Review via email: mp+214230@code.launchpad.net

Commit message

Fix last item X position

Fixes clicking on the last item sometimes not working

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

* 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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-04:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-07:

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

review: Approve (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-09:

Should we maybe move the test to a separate suite so that the precondition of it "running first" is certain? I don't think alphabetical order is defined during test runs?

Or maybe we need to make it into a component and reload it in that test?

review: Needs Information

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-09:

I think it's pretty much to be defined to alphabetical yes.

I can move it to a different file, but we don't have that for anything and then it's not testCarousel anymore but testCarousel and testCarousel2?

I'd prefer not to move it into a component, to be honest the test conditions are pretty much strict, so we either keep the alphabetical change or make it a different file if you can suggest names for the two test files.

Revision history for this message

Andrea Cimitan (cimi) wrote on 2014-04-10:

+ wait(1000);

instead of this you can just wait for the flick to end or the velocity to decrease under a certain threshold.

review: Needs Fixing

lp:~aacid/unity8/carouselLastItemClick updated on 2014-04-11

813. By Albert Astals Cid on 2014-04-11: wait is evil

Revision history for this message

Andrea Cimitan (cimi) wrote on 2014-04-11:

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

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-11:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-14:

Please strip tags, you can use the script:

http://people.canonical.com/~msawicz/unity8/strip-u8-tags.sh

Pass lp:~aacid/unity8/carouselLastItemClick as an argument.

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/Components/Carousel.qml'
 --- qml/Components/Carousel.qml	2014-03-27 10:27:30 +0000
 +++ qml/Components/Carousel.qml	2014-04-11 08:33:00 +0000
@@ -75,6 +75,7 @@
         The scaling of the items is controlled by the variable continuousIndex, described below. */
      ListView {
          id: listView
++        objectName: "listView"
          property int highlightIndex: -1
          property real minimumTileWidth: 0
@@ -148,15 +149,19 @@
          maximumFlickVelocity: Math.max(2500 * Math.pow(realWidth / referenceWidth, 1.5), 2500) // 2500 is platform default
          orientation: ListView.Horizontal
++        function getXFromContinuousIndex(index) {
++            return CarouselJS.getXFromContinuousIndex(index,
++                                                       realWidth,
++                                                       footerItem.x,
++                                                       tileWidth,
++                                                       gapToMiddlePhase,
++                                                       gapToEndPhase,
++                                                       carousel.drawBuffer)
++        }
++
          function itemClicked(index, delegateItem) {
              listView.currentIndex = index
--            var x = CarouselJS.getXFromContinuousIndex(index,
--                                                       realWidth,
--                                                       realContentWidth,
--                                                       tileWidth,
--                                                       gapToMiddlePhase,
--                                                       gapToEndPhase,
--                                                       carousel.drawBuffer)
++            var x = getXFromContinuousIndex(index);
              if (Math.abs(x - contentX) < 1 && delegateItem !== undefined) {
                  /* We're clicking the selected item and
@@ -174,13 +179,7 @@
+         }
          function itemPressAndHold(index, delegateItem) {
--            var x = CarouselJS.getXFromContinuousIndex(index,
--                                                       realWidth,
--                                                       realContentWidth,
--                                                       tileWidth,
--                                                       gapToMiddlePhase,
--                                                       gapToEndPhase,
--                                                       carousel.drawBuffer);
++            var x = getXFromContinuousIndex(index);
              if (Math.abs(x - contentX) < 1 && delegateItem !== undefined) {
                  /* We're pressAndHold the selected item and
@@ -209,22 +208,17 @@
              newContentX = disabledNewContentX
+         }
          onMovementEnded: {
--            if (realContentX > 0 && realContentX < realContentWidth - realWidth)
++            if (realContentX > 0)
                  stepAnimation.start()
+         }
          SmoothedAnimation {
              id: stepAnimation
++            objectName: "stepAnimation"
              target: listView
              property: "contentX"
--            to: CarouselJS.getXFromContinuousIndex(listView.selectedIndex,
--                                                   listView.realWidth,
--                                                   listView.realContentWidth,
--                                                   listView.tileWidth,
--                                                   listView.gapToMiddlePhase,
--                                                   listView.gapToEndPhase,
--                                                   carousel.drawBuffer)
++            to: listView.getXFromContinuousIndex(listView.selectedIndex)
              duration: 450
              velocity: 200
              easing.type: Easing.InOutQuad
 === modified file 'tests/qmltests/Components/tst_Carousel.qml'
 --- tests/qmltests/Components/tst_Carousel.qml	2014-03-27 16:54:32 +0000
 +++ tests/qmltests/Components/tst_Carousel.qml	2014-04-11 08:33:00 +0000
@@ -18,6 +18,7 @@
  import QtTest 1.0
  import "../../../qml/Components/"
  import "../../../qml/Components/carousel.js" as Carousel
++import Unity.Test 0.1 as UT
  Item {
      width: 700
@@ -32,12 +33,13 @@
          property int destroyedDelegates: 0
          itemComponent: Rectangle {
++            border.color: "black"
              color: "red"
              Component.onCompleted: carousel.createdDelegates++
              Component.onDestruction: carousel.destroyedDelegates++
+         }
+     }
--    TestCase {
++    UT.UnityTestCase {
          id: root
          name: "Carousel"
          when: windowShown
@@ -55,8 +57,11 @@
          property real kGapEnd: kMiddleIndex * (1 - gapToEndPhase / gapToMiddlePhase)
          property real kXBeginningEnd: 1 / tileWidth + kMiddleIndex / gapToMiddlePhase
--        function test_onlyNeededItemsCreated() {
--            compare(carousel.createdDelegates, 10);
++        // This test needs to run first since it tests a corner case condition
++        // to where the carousel is just being created
++        // That is why it has a 1 in the name
++        function test_1onlyNeededItemsCreated() {
++            tryCompare(carousel, "createdDelegates", 10);
              compare(carousel.destroyedDelegates, 0);
+         }
@@ -216,5 +221,24 @@
                                                      data.maxTranslation)
              compare(scale, data.result)
+         }
++
++        function test_lastItemXPosition() {
++            var carouselList = findChild(carousel, "listView");
++            var stepAnimation = findInvisibleChild(carouselList, "stepAnimation");
++            var stepDiff = 0;
++            stepAnimation.onRunningChanged.connect(function() { if (stepAnimation.running) stepDiff = Math.abs(carouselList.contentX - stepAnimation.to); })
++            var overshootOnce = false;
++            while (!overshootOnce) {
++                overshootOnce = carouselList.realContentX > carouselList.realContentWidth - carouselList.realWidth;
++                mouseFlick(carousel, carousel.width - units.gu(1),
++                                            carousel.height / 2,
++                                            units.gu(2),
++                                            carousel.height / 2);
++                tryCompare(carouselList, "moving", false);
++            }
++            verify(stepDiff < 1);
++            var x = carouselList.getXFromContinuousIndex(carousel.model - 1);
++            verify(Math.abs(x - carouselList.contentX) < 1);
++        }
+     }
+ }