Unity 8

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

categoryDelegateRangeFixOvershootDetection
Merge into trunk

Proposed by Albert Astals Cid on 2014-04-08

Status:	Merged
Approved by:	Michael Zanetti on 2014-04-17
Approved revision:	789
Merged at revision:	856
Proposed branch:	lp:~aacid/unity8/categoryDelegateRangeFixOvershootDetection
Merge into:	lp:unity8
Prerequisite:	lp:~aacid/unity8/delegateRangeNeedsOriginY
Diff against target:	25 lines (+9/-6) 1 file modified qml/Dash/GenericScopeView.qml (+9/-6)
To merge this branch:	bzr merge lp:~aacid/unity8/categoryDelegateRangeFixOvershootDetection
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Zanetti (community)		2014-04-08	Approve on 2014-04-17
PS Jenkins bot (community)	continuous-integration	2014-04-08	Needs Fixing on 2014-04-17
Review via email: mp+214764@code.launchpad.net

This proposal supersedes a proposal from 2014-03-26.

Commit message

CategoryDelegateRange: Fix condition for detecting overshooting

Description of the change

This is hard to autotest since what was happening is that all the delegates where being created and then immediately destroyed, so at the end the number of delegates we'd have in a tryCompare would be the same.

For manually testing this:
* Add to Card.qml root
+ Component.onCompleted: console.log("Created", root)
+ Component.onDestruction: console.log("Destroyed", root)
* And then start the shell and uncollapse the Available category of the Apps scope

Without it you'll see that there's lots of created and destroyed, without it there's only a few created.

* Are there any related MPs required for this MP to build/function as expected?
Yes, see prerequisite

* 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-03-26: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2014-04-04: Posted in a previous version of this proposal

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

Yes. Everything still works. The code change makes sense to me.

* Did CI run pass? If not, please explain why.

Yip

review: Approve

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-08: Posted in a previous version of this proposal

Just realzied a bug in here

lp:~aacid/unity8/categoryDelegateRangeFixOvershootDetection updated on 2014-04-08

788. By Albert Astals Cid on 2014-04-08: Fixes

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

lp:~aacid/unity8/categoryDelegateRangeFixOvershootDetection updated on 2014-04-17

789. By Albert Astals Cid on 2014-04-17: Merge

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2014-04-17:

> This is hard to autotest since what was happening is that all the delegates where being created and then immediately > destroyed, so at the end the number of delegates we'd have in a tryCompare would be the same.

Should be quite easy to detect I think...

Get the pointer to the first item that would be destroyed without this patch, do the overshoot and verify that the pointer didn't change. In case its destroyed and recreated, you'd have a different pointer, no?

Revision history for this message

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

So it is not hard in principle but it's actually hard in practice, if we want to exercise the new condition of taking into account originY we need a way to modify it, but this only happens internally when modifying adding items on the list, but unfortunately at the moment our tests for GenericScopeView depend on a static fake Scopes that returns the same scope list all the time.

I agree it would be great having a test for this, but at the moment modifying the test to do that is probably too much work.

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2014-04-17:

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

Nope, but verified that the offending test passes after merging https://code.launchpad.net/~aacid/unity8/card_optimizations/+merge/213660

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/GenericScopeView.qml'
 --- qml/Dash/GenericScopeView.qml	2014-04-16 13:44:46 +0000
 +++ qml/Dash/GenericScopeView.qml	2014-04-17 08:07:23 +0000
@@ -238,12 +238,15 @@
+                 }
                  function updateDelegateCreationRange() {
--                    // 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 (categoryView.contentY < categoryView.originY) {
--                        return;
--                    } else if (categoryView.contentHeight > categoryView.height && categoryView.contentY + categoryView.height > categoryView.contentHeight) {
--                        return;
++                    if (categoryView.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 (categoryView.contentY < categoryView.originY) {
++                            return;
++                        } else if (categoryView.contentHeight - categoryView.originY > categoryView.height &&
++                                   categoryView.contentY + categoryView.height > categoryView.contentHeight) {
++                            return;
++                        }
+                     }
                      if (item && item.hasOwnProperty("delegateCreationBegin")) {