Unity 8

Merge lp:~mzanetti/unity8/fix-appdelegate-jumping into lp:unity8

fix-appdelegate-jumping
Merge into trunk

Proposed by Michael Zanetti on 2014-11-17

Status:

Merged

Approved by:

Michał Sawicz on 2014-11-25

Approved revision:

1440

Merged at revision:

1455

Proposed branch:

lp:~mzanetti/unity8/fix-appdelegate-jumping

Merge into:

lp:unity8

Diff against target:

48 lines (+17/-4)

2 files modified

qml/Stages/SpreadDelegate.qml (+13/-3)
tests/qmltests/Stages/tst_SpreadDelegate.qml (+4/-1)

To merge this branch:

bzr merge lp:~mzanetti/unity8/fix-appdelegate-jumping

Related bugs:

Bug #1350803: [Spread] Threashold in spread for closing apps should be increased	High	Fix Committed
Bug #1352842: [Spread] Item jumps when drag/swipe to close is activated	Medium	Fix Committed

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2014-11-24
Daniel d'Andrada (community)			Abstain on 2014-11-20
Michał Sawicz			Needs Fixing on 2014-11-20
Albert Astals Cid (community)		2014-11-17	Approve on 2014-11-18
Review via email: mp+241930@code.launchpad.net

Commit message

increase threshold and avoid jumping in SpreadDelegate dragging

Description of the change

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

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

fixing bugs reported by design

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-11-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.
Waiting for it for top approval

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

review: Approve

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

FAILED: Continuous integration, rev:1437
http://jenkins.qa.ubuntu.com/job/unity8-ci/4923/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/248
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/88
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/88
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/223
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/248
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/248/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15915

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-11-18:

qmluitest got broken by this.

review: Needs Fixing

Revision history for this message

Albert Astals Cid (aacid) on 2014-11-18:

review: Approve

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2014-11-18:

35 + verify(Math.abs(Math.abs(appWindowWithShadow.y) - dragDistance) == units.gu(2));

You should make the test access the threshold variable instead of duplicating its value here, which has the danger or getting out of sync. It would also hopefully make this expression more self-explanatory

review: Needs Fixing

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-11-20:

This introduced jumping in the spread when dragging the app up'n'down and crossing the middle of the screen.

review: Needs Fixing

lp:~mzanetti/unity8/fix-appdelegate-jumping updated on 2014-11-20

1439. By Michael Zanetti on 2014-11-20: fix jumping when changing swipe direction

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2014-11-20:

> 35 + verify(Math.abs(Math.abs(appWindowWithShadow.y) - dragDistance) ==
> units.gu(2));
>
> You should make the test access the threshold variable instead of duplicating
> its value here, which has the danger or getting out of sync. It would also
> hopefully make this expression more self-explanatory

fixed

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2014-11-20:

> This introduced jumping in the spread when dragging the app up'n'down and
> crossing the middle of the screen.

fixed

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2014-11-20:

> > 35 + verify(Math.abs(Math.abs(appWindowWithShadow.y) - dragDistance) ==
> > units.gu(2));
> >
> > You should make the test access the threshold variable instead of
> duplicating
> > its value here, which has the danger or getting out of sync. It would also
> > hopefully make this expression more self-explanatory
>
> fixed

Thanks!

review: Abstain

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

lp:~mzanetti/unity8/fix-appdelegate-jumping updated on 2014-11-24

1440. By Michael Zanetti on 2014-11-24: verfiy -> compare

Revision history for this message

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

FAILED: Continuous integration, rev:1440
http://jenkins.qa.ubuntu.com/job/unity8-ci/4958/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/326
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/123
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/123
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/298
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/326
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/326/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16163

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

review: Needs Fixing (continuous-integration)

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:

Michael Zanetti

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Stages/SpreadDelegate.qml'
 --- qml/Stages/SpreadDelegate.qml	2014-09-19 11:20:07 +0000
 +++ qml/Stages/SpreadDelegate.qml	2014-11-24 10:27:18 +0000
@@ -75,6 +75,8 @@
          property bool moving: false
          property real distance: 0
++        readonly property int threshold: units.gu(2)
++        property int offset: 0
          readonly property real minSpeedToClose: units.gu(40)
@@ -82,9 +84,17 @@
              if (!dragging) {
                  return;
+             }
--            moving = moving || Math.abs(dragValue) > units.gu(1)
--            if (moving) {
--                distance = dragValue;
++            moving = moving || Math.abs(dragValue) > threshold;
++            if (moving) {
++                distance = dragValue + offset;
++            }
++        }
++
++        onMovingChanged: {
++            if (moving) {
++                offset = (dragValue > 0 ? -threshold: threshold)
++            } else {
++                offset = 0;
+             }
+         }
 === modified file 'tests/qmltests/Stages/tst_SpreadDelegate.qml'
 --- tests/qmltests/Stages/tst_SpreadDelegate.qml	2014-08-13 19:50:09 +0000
 +++ tests/qmltests/Stages/tst_SpreadDelegate.qml	2014-11-24 10:27:18 +0000
@@ -137,7 +137,10 @@
              if (data.swipeToClose) {
                  verify(appWindowWithShadow.y < 0);
--                verify(Math.abs(Math.abs(appWindowWithShadow.y) - dragDistance) < units.gu(1));
++                var threshold = findChild(spreadDelegateLoader.item, "dragArea").threshold
++                // Verify that the delegate started moving exactly "threshold" after the finger movement
++                // and did not jump up to the finger, but lags the threshold behind
++                compare(Math.abs(Math.abs(appWindowWithShadow.y) - dragDistance), threshold);
                  touchRelease(spreadDelegateLoader.item, touchX, toY - units.gu(1));