Merge lp:~mzanetti/unity8/edge-hinting-tweaks into lp:unity8
- edge-hinting-tweaks
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | MichaĆ Sawicz |
Approved revision: | 63 |
Merged at revision: | 67 |
Proposed branch: | lp:~mzanetti/unity8/edge-hinting-tweaks |
Merge into: | lp:unity8 |
Diff against target: |
193 lines (+91/-8) 5 files modified
Greeter/Greeter.qml (+2/-2) Launcher/Launcher.qml (+3/-4) Shell.qml (+6/-1) tests/qmltests/Greeter/tst_Phone.qml (+40/-1) tests/qmltests/Launcher/tst_Launcher.qml (+40/-0) |
To merge this branch: | bzr merge lp:~mzanetti/unity8/edge-hinting-tweaks |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve | |
Daniel d'Andrada (community) | Approve | ||
Review via email: mp+172014@code.launchpad.net |
Commit message
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
Description of the change
PS Jenkins bot (ps-jenkins) wrote : | # |
Daniel d'Andrada (dandrader) wrote : | # |
> FAILED: Continuous integration, rev:56
> [...]
Jenkins crashed. Asked for a rebuild.
Daniel d'Andrada (dandrader) wrote : | # |
> - 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) wrote : | # |
PASS : qmltestrunner:
PASS : qmltestrunner:
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) wrote : | # |
FAILED: Continuous integration, rev:56
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Daniel d'Andrada (dandrader) wrote : | # |
tests/qmltests/
Daniel d'Andrada (dandrader) wrote : | # |
tst_Launcher.
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) wrote : | # |
Other than those coding style issues, the Launcher test looks good.
Michael Zanetti (mzanetti) wrote : | # |
> > - 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) wrote : | # |
> PASS : qmltestrunner:
> PASS : qmltestrunner:
>
> 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) wrote : | # |
> tst_Launcher.
> tryCompareFunct
> -launcher.
>
> 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) wrote : | # |
FAILED: Continuous integration, rev:58
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Daniel d'Andrada (dandrader) wrote : | # |
> > tst_Launcher.
> > tryCompareFunct
> > -launcher.
> >
> > 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) wrote : | # |
> 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_noTeaseIfL
But I won't insist if you wanna keep it like that. It's already good as it is.
Michael Zanetti (mzanetti) wrote : | # |
> > 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_noTeaseIfL
> Including a comment explaining what the test wanna check also wouldn't hurt
> (tests/
> 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_teaseLocke
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:61
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:63
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
1 | === modified file 'Greeter/Greeter.qml' |
2 | --- Greeter/Greeter.qml 2013-06-19 08:29:34 +0000 |
3 | +++ Greeter/Greeter.qml 2013-06-28 16:41:39 +0000 |
4 | @@ -48,7 +48,7 @@ |
5 | signal unlocked(int uid) |
6 | |
7 | onRightTeaserPressedChanged: { |
8 | - if (rightTeaserPressed) { |
9 | + if (rightTeaserPressed && !locked) { |
10 | teasingTimer.start(); |
11 | } |
12 | } |
13 | @@ -81,7 +81,7 @@ |
14 | states: [ |
15 | State { |
16 | name: "teasing" |
17 | - when: greeter.rightTeaserPressed || teasingTimer.running |
18 | + when: teasingTimer.running |
19 | PropertyChanges { |
20 | target: greeter |
21 | x: -units.gu(2) |
22 | |
23 | === modified file 'Launcher/Launcher.qml' |
24 | --- Launcher/Launcher.qml 2013-06-26 11:25:43 +0000 |
25 | +++ Launcher/Launcher.qml 2013-06-28 16:41:39 +0000 |
26 | @@ -25,7 +25,6 @@ |
27 | |
28 | property bool available: true // can be used to disable all interactions |
29 | |
30 | - property bool teasing: false |
31 | property int panelWidth: units.gu(8.5) |
32 | property int dragAreaWidth: units.gu(1) |
33 | property real progress: dragArea.dragging && dragArea.touchX > panel.width ? dragArea.touchX : 0 |
34 | @@ -58,8 +57,8 @@ |
35 | animateTimer.start(); |
36 | } |
37 | |
38 | - onTeasingChanged: { |
39 | - if (teasing) { |
40 | + function tease() { |
41 | + if (available) { |
42 | teaseTimer.start(); |
43 | } |
44 | } |
45 | @@ -244,7 +243,7 @@ |
46 | }, |
47 | State { |
48 | name: "teasing" |
49 | - when: root.teasing || teaseTimer.running |
50 | + when: teaseTimer.running |
51 | PropertyChanges { |
52 | target: panel |
53 | x: -root.panelWidth + units.gu(2) |
54 | |
55 | === modified file 'Shell.qml' |
56 | --- Shell.qml 2013-06-27 16:25:18 +0000 |
57 | +++ Shell.qml 2013-06-28 16:41:39 +0000 |
58 | @@ -439,6 +439,12 @@ |
59 | var bgPath = greeter.model.data(uid, LightDM.UserRoles.BackgroundPathRole) |
60 | shell.background = bgPath ? bgPath : default_background |
61 | } |
62 | + |
63 | + onLeftTeaserPressedChanged: { |
64 | + if (leftTeaserPressed) { |
65 | + launcher.tease(); |
66 | + } |
67 | + } |
68 | } |
69 | |
70 | InputFilterArea { |
71 | @@ -532,7 +538,6 @@ |
72 | width: parent.width |
73 | dragAreaWidth: shell.edgeSize |
74 | available: !greeter.locked |
75 | - teasing: available && greeter.leftTeaserPressed |
76 | onDashItemSelected: { |
77 | greeter.hide() |
78 | // Animate if moving between application and dash |
79 | |
80 | === modified file 'tests/qmltests/Greeter/tst_Phone.qml' |
81 | --- tests/qmltests/Greeter/tst_Phone.qml 2013-06-13 14:24:52 +0000 |
82 | +++ tests/qmltests/Greeter/tst_Phone.qml 2013-06-28 16:41:39 +0000 |
83 | @@ -28,7 +28,17 @@ |
84 | |
85 | Greeter { |
86 | id: greeter |
87 | - anchors.fill: parent |
88 | + width: parent.width |
89 | + height: parent.height |
90 | + x: 0; y: 0 |
91 | + |
92 | + property int minX: 0 |
93 | + |
94 | + onXChanged: { |
95 | + if (x < minX) { |
96 | + minX = x; |
97 | + } |
98 | + } |
99 | } |
100 | |
101 | SignalSpy { |
102 | @@ -63,5 +73,34 @@ |
103 | tryCompare(greeter, "leftTeaserPressed", false) |
104 | tryCompare(greeter, "rightTeaserPressed", false) |
105 | } |
106 | + |
107 | + function test_teaseLockedUnlocked_data() { |
108 | + return [ |
109 | + {tag: "unlocked", locked: false}, |
110 | + {tag: "locked", locked: true} |
111 | + ]; |
112 | + } |
113 | + |
114 | + function test_teaseLockedUnlocked(data) { |
115 | + tryCompare(greeter, "rightTeaserPressed", false); |
116 | + tryCompare(greeter, "x", 0); |
117 | + greeter.locked = data.locked; |
118 | + |
119 | + mouseClick(greeter, greeter.width - units.gu(5), greeter.height - units.gu(1)); |
120 | + greeter.minX = 0; // This is needed because the transition actually makes x jump once before animating |
121 | + |
122 | + if (!data.locked) { |
123 | + // Check if it has been moved over by 2 GUs. Give it a 2 pixel grace area |
124 | + // because animation duration and teaseTimer are the same duration and |
125 | + // might cause slight offsets |
126 | + tryCompareFunction(function() { return greeter.minX <= -units.gu(2) + 2}, true); |
127 | + } else { |
128 | + // waiting 100ms to make sure nothing moves |
129 | + wait(100); |
130 | + compare(greeter.minX, 0, "Greeter moved even tho its locked"); |
131 | + } |
132 | + // Wait until we're back to 0 |
133 | + tryCompareFunction(function() { return greeter.x;}, 0); |
134 | + } |
135 | } |
136 | } |
137 | |
138 | === modified file 'tests/qmltests/Launcher/tst_Launcher.qml' |
139 | --- tests/qmltests/Launcher/tst_Launcher.qml 2013-06-05 22:03:08 +0000 |
140 | +++ tests/qmltests/Launcher/tst_Launcher.qml 2013-06-28 16:41:39 +0000 |
141 | @@ -43,6 +43,18 @@ |
142 | onDashItemSelected: { |
143 | dashItemSelected_count++; |
144 | } |
145 | + |
146 | + property int maxPanelX: 0 |
147 | + } |
148 | + |
149 | + Connections { |
150 | + target: testCase.findChild(launcher, "launcherPanel") |
151 | + |
152 | + onXChanged: { |
153 | + if (target.x > launcher.maxPanelX) { |
154 | + launcher.maxPanelX = target.x; |
155 | + } |
156 | + } |
157 | } |
158 | |
159 | UT.UnityTestCase { |
160 | @@ -126,5 +138,33 @@ |
161 | var panel = findChild(launcher, "launcherPanel") |
162 | tryCompare(panel, "x", -panel.width, 1000) |
163 | } |
164 | + |
165 | + |
166 | + function test_teaseLauncher_data() { |
167 | + return [ |
168 | + {tag: "available", available: true}, |
169 | + {tag: "not available", available: false} |
170 | + ]; |
171 | + } |
172 | + |
173 | + function test_teaseLauncher(data) { |
174 | + launcher.available = data.available; |
175 | + launcher.maxPanelX = -launcher.panelWidth; |
176 | + launcher.tease(); |
177 | + |
178 | + if (data.available) { |
179 | + // Check if the launcher slides in for units.gu(2). However, as the animation is 200ms |
180 | + // and the teaseTimer's timeout too, give it a 2 pixels grace distance |
181 | + tryCompareFunction( |
182 | + function(){ |
183 | + return launcher.maxPanelX >= -launcher.panelWidth + units.gu(2) - 2; |
184 | + }, |
185 | + true) |
186 | + } else { |
187 | + wait(100) |
188 | + compare(launcher.maxPanelX, -launcher.panelWidth, "Launcher moved even if it shouldn't") |
189 | + } |
190 | + waitUntilLauncherDisappears(); |
191 | + } |
192 | } |
193 | } |
FAILED: Continuous integration, rev:56 jenkins. qa.ubuntu. com/job/ unity8- ci/12/ jenkins. qa.ubuntu. com/job/ generic- mediumtests- saucy/460/ console jenkins. qa.ubuntu. com/job/ unity-phablet- qmluitests- saucy/253 jenkins. qa.ubuntu. com/job/ unity8- saucy-armhf- ci/12 jenkins. qa.ubuntu. com/job/ unity8- saucy-armhf- ci/12/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ unity8- saucy-i386- ci/12 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- saucy/462 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- saucy/462/ artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ generic- mediumtests- runner- saucy/420/ console
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins: 8080/job/ unity8- ci/12/rebuild
http://