Merge lp:~mzanetti/unity8/edge-hinting-tweaks into lp:unity8

[Michael Zanetti]

edge hinting tweaks

  - change edge hinting behavior to only happen on onPressed and immediately snap back
  - fix edge hinting to not happen if the Greeter is locked

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:56
http://jenkins.qa.ubuntu.com/job/unity8-ci/12/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/460/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/253
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/12/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/462
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/462/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/420/console

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

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

> FAILED: Continuous integration, rev:56
> [...]

Jenkins crashed. Asked for a rebuild.

[Daniel d'Andrada (dandrader)]

> - fix edge hinting to not happen if the Greeter is locked

Would it make sense to test it?

eg:
 tap on greeter when not locked -> check that greeter bounces somehow (track its x coord)
 tap on greeter when locked -> check that greeter stays still (its x coord doesn't change at all)

Or it's not worth the effort?

[Daniel d'Andrada (dandrader)]

PASS : qmltestrunner::Launcher::test_teaseLauncher(tease - available)
PASS : qmltestrunner::Launcher::test_teaseLauncher(tease - not available)

I think the tags can say just "available" and "not available". We can already see that it's about a "tease" test from the test name itself.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:56
http://jenkins.qa.ubuntu.com/job/unity8-ci/14/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/464
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/256/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/14
        deb: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/14/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-ci/14
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/466
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/466/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/424

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

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

tests/qmltests/Launcher/tst_Launcher.qml: bad whitespace in line 156

[Daniel d'Andrada (dandrader)]

tst_Launcher.qml:159
                tryCompareFunction(function(){return launcher.maxPanelX >= -launcher.panelWidth + units.gu(2) - 2;}, true)

That's 122 chars long. Our coding style sets a limit of 120 (hmm, I'm starting to think we should have an automatic checker for that, like we do for the whitespace)

[Daniel d'Andrada (dandrader)]

Other than those coding style issues, the Launcher test looks good.

[Michael Zanetti (mzanetti)]

> > - fix edge hinting to not happen if the Greeter is locked
>
> Would it make sense to test it?
>
> eg:
> tap on greeter when not locked -> check that greeter bounces somehow (track
> its x coord)
> tap on greeter when locked -> check that greeter stays still (its x coord
> doesn't change at all)
>
> Or it's not worth the effort?

done

[Michael Zanetti (mzanetti)]

> PASS : qmltestrunner::Launcher::test_teaseLauncher(tease - available)
> PASS : qmltestrunner::Launcher::test_teaseLauncher(tease - not available)
>
> I think the tags can say just "available" and "not available". We can already
> see that it's about a "tease" test from the test name itself.

done

[Michael Zanetti (mzanetti)]

> tst_Launcher.qml:159
> tryCompareFunction(function(){return launcher.maxPanelX >=
> -launcher.panelWidth + units.gu(2) - 2;}, true)
>
> That's 122 chars long. Our coding style sets a limit of 120 (hmm, I'm starting
> to think we should have an automatic checker for that, like we do for the
> whitespace)

now it looks like shit and did not contribute to readability the smallest bit, but as long as the guideline is happy...

done

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:58
http://jenkins.qa.ubuntu.com/job/unity8-ci/17/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/470/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/260/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/17/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-ci/17/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/472/console

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

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

> > tst_Launcher.qml:159
> > tryCompareFunction(function(){return launcher.maxPanelX >=
> > -launcher.panelWidth + units.gu(2) - 2;}, true)
> >
> > That's 122 chars long. Our coding style sets a limit of 120 (hmm, I'm
> starting
> > to think we should have an automatic checker for that, like we do for the
> > whitespace)
>
> now it looks like shit and did not contribute to readability the smallest bit,
> but as long as the guideline is happy...
>
> done

"Beauty is in the eye of the beholder". And it's not only the guideline that is happy. I am too. :D

[Daniel d'Andrada (dandrader)]

> function test_tease(data) {

That's a very inclusive name (could include even test_teasingArea). But what you're testing there is much more specific. You're testing the interaction between greeter being locked and its tease animation. If later you add a test about something else involving the tease animation you might have to rename this one. So I would call it like test_noTeaseIfLocked or test_teaseVsLock. Including a comment explaining what the test wanna check also wouldn't hurt (tests/qmltests/README!) but it's perfectly fine if you don't (test is rather small after all).

But I won't insist if you wanna keep it like that. It's already good as it is.

review: Approve

[Michael Zanetti (mzanetti)]

> > function test_tease(data) {
>
> That's a very inclusive name (could include even test_teasingArea). But what
> you're testing there is much more specific. You're testing the interaction
> between greeter being locked and its tease animation. If later you add a test
> about something else involving the tease animation you might have to rename
> this one. So I would call it like test_noTeaseIfLocked or test_teaseVsLock.
> Including a comment explaining what the test wanna check also wouldn't hurt
> (tests/qmltests/README!) but it's perfectly fine if you don't (test is rather
> small after all).
>

Yep. I agree on this. I used test_tease mainly because test_teasingArea was already there, which checks if both teasingAreas work. As this one really does a tease, I named it lake that. Anyways, you're absolutely right on this and I renamed it to test_teaseLockedUnlocked().

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:61
http://jenkins.qa.ubuntu.com/job/unity8-ci/19/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/476/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/265/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/19/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-ci/19/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/478/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:63
http://jenkins.qa.ubuntu.com/job/unity8-ci/22/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/485
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/487
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/487/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/439

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity8-autolanding/9/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/525
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/304
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-autolanding/9
        deb: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-autolanding/9/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-autolanding/9
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/527
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/527/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/474

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity8-autolanding/12/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/533
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-saucy/308/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-autolanding/12
        deb: http://jenkins.qa.ubuntu.com/job/unity8-saucy-armhf-autolanding/12/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-saucy-i386-autolanding/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/535
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/535/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/482

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'Greeter/Greeter.qml'
--- Greeter/Greeter.qml	2013-06-19 08:29:34 +0000
+++ Greeter/Greeter.qml	2013-06-28 16:41:39 +0000
@@ -48,7 +48,7 @@
     signal unlocked(int uid)
 
     onRightTeaserPressedChanged: {
-        if (rightTeaserPressed) {
+        if (rightTeaserPressed && !locked) {
             teasingTimer.start();
         }
     }
@@ -81,7 +81,7 @@
     states: [
         State {
             name: "teasing"
-            when: greeter.rightTeaserPressed || teasingTimer.running
+            when: teasingTimer.running
             PropertyChanges {
                 target: greeter
                 x: -units.gu(2)

=== modified file 'Launcher/Launcher.qml'
--- Launcher/Launcher.qml	2013-06-26 11:25:43 +0000
+++ Launcher/Launcher.qml	2013-06-28 16:41:39 +0000
@@ -25,7 +25,6 @@
 
     property bool available: true // can be used to disable all interactions
 
-    property bool teasing: false
     property int panelWidth: units.gu(8.5)
     property int dragAreaWidth: units.gu(1)
     property real progress: dragArea.dragging  && dragArea.touchX > panel.width ? dragArea.touchX : 0
@@ -58,8 +57,8 @@
         animateTimer.start();
     }
 
-    onTeasingChanged: {
-        if (teasing) {
+    function tease() {
+        if (available) {
             teaseTimer.start();
         }
     }
@@ -244,7 +243,7 @@
         },
         State {
             name: "teasing"
-            when: root.teasing || teaseTimer.running
+            when: teaseTimer.running
             PropertyChanges {
                 target: panel
                 x: -root.panelWidth + units.gu(2)

=== modified file 'Shell.qml'
--- Shell.qml	2013-06-27 16:25:18 +0000
+++ Shell.qml	2013-06-28 16:41:39 +0000
@@ -439,6 +439,12 @@
             var bgPath = greeter.model.data(uid, LightDM.UserRoles.BackgroundPathRole)
             shell.background = bgPath ? bgPath : default_background
         }
+
+        onLeftTeaserPressedChanged: {
+            if (leftTeaserPressed) {
+                launcher.tease();
+            }
+        }
     }
 
     InputFilterArea {
@@ -532,7 +538,6 @@
             width: parent.width
             dragAreaWidth: shell.edgeSize
             available: !greeter.locked
-            teasing: available && greeter.leftTeaserPressed
             onDashItemSelected: {
                 greeter.hide()
                 // Animate if moving between application and dash

=== modified file 'tests/qmltests/Greeter/tst_Phone.qml'
--- tests/qmltests/Greeter/tst_Phone.qml	2013-06-13 14:24:52 +0000
+++ tests/qmltests/Greeter/tst_Phone.qml	2013-06-28 16:41:39 +0000
@@ -28,7 +28,17 @@
 
     Greeter {
         id: greeter
-        anchors.fill: parent
+        width: parent.width
+        height: parent.height
+        x: 0; y: 0
+
+        property int minX: 0
+
+        onXChanged: {
+            if (x < minX) {
+                minX = x;
+            }
+        }
     }
 
     SignalSpy {
@@ -63,5 +73,34 @@
             tryCompare(greeter, "leftTeaserPressed", false)
             tryCompare(greeter, "rightTeaserPressed", false)
         }
+
+        function test_teaseLockedUnlocked_data() {
+            return [
+                {tag: "unlocked", locked: false},
+                {tag: "locked", locked: true}
+            ];
+        }
+
+        function test_teaseLockedUnlocked(data) {
+            tryCompare(greeter, "rightTeaserPressed", false);
+            tryCompare(greeter, "x", 0);
+            greeter.locked = data.locked;
+
+            mouseClick(greeter, greeter.width - units.gu(5), greeter.height - units.gu(1));
+            greeter.minX = 0; // This is needed because the transition actually makes x jump once before animating
+
+            if (!data.locked) {
+                // Check if it has been moved over by 2 GUs. Give it a 2 pixel grace area
+                // because animation duration and teaseTimer are the same duration and
+                // might cause slight offsets
+                tryCompareFunction(function() { return greeter.minX <= -units.gu(2) + 2}, true);
+            } else {
+                // waiting 100ms to make sure nothing moves
+                wait(100);
+                compare(greeter.minX, 0, "Greeter moved even tho its locked");
+            }
+            // Wait until we're back to 0
+            tryCompareFunction(function() { return greeter.x;},  0);
+        }
     }
 }

=== modified file 'tests/qmltests/Launcher/tst_Launcher.qml'
--- tests/qmltests/Launcher/tst_Launcher.qml	2013-06-05 22:03:08 +0000
+++ tests/qmltests/Launcher/tst_Launcher.qml	2013-06-28 16:41:39 +0000
@@ -43,6 +43,18 @@
         onDashItemSelected: {
             dashItemSelected_count++;
         }
+
+        property int maxPanelX: 0
+    }
+
+    Connections {
+        target: testCase.findChild(launcher, "launcherPanel")
+
+        onXChanged: {
+            if (target.x > launcher.maxPanelX) {
+                launcher.maxPanelX = target.x;
+            }
+        }
     }
 
     UT.UnityTestCase {
@@ -126,5 +138,33 @@
             var panel = findChild(launcher, "launcherPanel")
             tryCompare(panel, "x", -panel.width, 1000)
         }
+
+
+        function test_teaseLauncher_data() {
+            return [
+                {tag: "available", available: true},
+                {tag: "not available", available: false}
+            ];
+        }
+
+        function test_teaseLauncher(data) {
+            launcher.available = data.available;
+            launcher.maxPanelX = -launcher.panelWidth;
+            launcher.tease();
+
+            if (data.available) {
+                // Check if the launcher slides in for units.gu(2). However, as the animation is 200ms
+                // and the teaseTimer's timeout too, give it a 2 pixels grace distance
+                tryCompareFunction(
+                    function(){
+                        return launcher.maxPanelX >= -launcher.panelWidth + units.gu(2) - 2;
+                    },
+                    true)
+            } else {
+                wait(100)
+                compare(launcher.maxPanelX, -launcher.panelWidth, "Launcher moved even if it shouldn't")
+            }
+            waitUntilLauncherDisappears();
+        }
     }
 }