Merge lp:~feng-kylin/unity8/fix-lp1413791 into lp:unity8
- fix-lp1413791
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Albert Astals Cid |
Approved revision: | 1613 |
Merged at revision: | 1892 |
Proposed branch: | lp:~feng-kylin/unity8/fix-lp1413791 |
Merge into: | lp:unity8 |
Diff against target: |
94 lines (+44/-1) 4 files modified
qml/Dash/Dash.qml (+1/-1) qml/Dash/DashContent.qml (+1/-0) qml/Dash/GenericScopeView.qml (+5/-0) tests/qmltests/tst_Shell.qml (+37/-0) |
To merge this branch: | bzr merge lp:~feng-kylin/unity8/fix-lp1413791 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Albert Astals Cid (community) | Approve | ||
PS Jenkins bot (community) | continuous-integration | Needs Fixing | |
Michael Zanetti (community) | Needs Fixing | ||
Ubuntu design | Pending | ||
Review via email: mp+249452@code.launchpad.net |
Commit message
makes left swip reset the search string.
Description of the change
Added a series of signal and slot to deal with resetSearch when left swip.
* Are there any related MPs required for this MP to build/function as expected? Please list.
no
* 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?
n/a
Albert Astals Cid (aacid) wrote : | # |
Albert Astals Cid (aacid) wrote : | # |
And by this i mean the bug itself.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1596
http://
Executed test runs:
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
file://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1596
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
Why did you introduce the resetAll if it's always used when dash.setCurrent
handsome_feng (feng-kylin) wrote : | # |
> Why did you introduce the resetAll if it's always used when
> dash.setCurrent
Because I think long left swipe/BFB may reset something else more than the search string in the future. but I think you are right, the signal is not necessary here, I will modify this.Thank you!
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1596
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1600
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
- dash.setCurrent
+ dash.setCurrent
Why this change?
handsome_feng (feng-kylin) wrote : | # |
> - dash.setCurrent
> + dash.setCurrent
>
> Why this change?
To reset the search query and position the list in the beginning when click home button on the launcher.
Notice that in setCurrentScope
if (reset) {
+ dashContentList
}
Albert Astals Cid (aacid) wrote : | # |
The change of behaviour in "swipe over dash" is not wanted, the launcher should not be hidden if we are just on the dash and do a long swipe
Albert Astals Cid (aacid) wrote : | # |
Do we need those clear() in tst_Shell.qml?
I removed them and the test still passes fine.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1603
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
Text conflict in qml/Dash/
1 conflicts encountered.
Please re-merge your branch with lp:untiy8
handsome_feng (feng-kylin) wrote : | # |
> Text conflict in qml/Dash/
> 1 conflicts encountered.
>
> Please re-merge your branch with lp:untiy8
done
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1604
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
Breaks in this case
* Be on non first scope
* Do a long left swipe
* See how you changed to the first scope
That should not happen
Albert Astals Cid (aacid) wrote : | # |
Also please add a test for the thing you're fixing
Albert Astals Cid (aacid) wrote : | # |
retriggered CI
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1605
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
I think maybe i'm not explaining myself correctly, this needs to be fixed:
* Make sure you have at least two scopes
* Be on the second scope
* Do a long left swipe that shows the launhcher
* Release the finger
* See how the scope has changed to the first scope
It should stay in the second scope like it does if when not using this branch.
Albert Astals Cid (aacid) wrote : | # |
Retriggered CI
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1606
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1606
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
There's another regression. Steps to reproduce:
* Have various scopes in dash
* Go to the last one
* Start app from launcher
* Do a long swipe from the left
* See that until you release the shown scope is the last scope instead of the first one
This is most likely due to the removal of the code of onDashSwipeChanged
Albert Astals Cid (aacid) wrote : | # |
Please move the launcherShowDas
Albert Astals Cid (aacid) wrote : | # |
* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes
* Did CI run pass?
As much as they pass lately.
* Did you make sure that the branch does not contain spurious tags?
Yes
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1610
http://
Executed test runs:
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Michael Zanetti (mzanetti) wrote : | # |
Sorry. This branch failed QA verification.
It breaks going to the "home" scope when you're on another scope by pressing the BFB.
- 1611. By handsome_feng
-
merge trunk
- 1612. By handsome_feng
-
BFB and left swipe both should reset the dash
- 1613. By handsome_feng
-
modified the test
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1613
http://
Executed test runs:
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
Re-approve the issue Michael mentioned has been fixed and can't find it having any other regression.
Preview Diff
1 | === modified file 'qml/Dash/Dash.qml' |
2 | --- qml/Dash/Dash.qml 2015-05-18 23:04:12 +0000 |
3 | +++ qml/Dash/Dash.qml 2015-07-20 08:36:50 +0000 |
4 | @@ -41,7 +41,7 @@ |
5 | onSetCurrentScopeRequested: { |
6 | if (!isSwipe || !window.active || bottomEdgeController.progress != 0 || scopeItem.scope || dashContent.subPageShown) { |
7 | if (bottomEdgeController.progress != 0 && window.active) animate = false; |
8 | - dashContent.setCurrentScopeAtIndex(index, animate, isSwipe) |
9 | + dashContent.setCurrentScopeAtIndex(index, animate, true) |
10 | backToDashContent() |
11 | } |
12 | } |
13 | |
14 | === modified file 'qml/Dash/DashContent.qml' |
15 | --- qml/Dash/DashContent.qml 2015-03-19 15:25:45 +0000 |
16 | +++ qml/Dash/DashContent.qml 2015-07-20 08:36:50 +0000 |
17 | @@ -84,6 +84,7 @@ |
18 | |
19 | if (reset) { |
20 | dashContentList.currentItem.item.positionAtBeginning() |
21 | + dashContentList.currentItem.item.resetSearch() |
22 | } |
23 | } |
24 | |
25 | |
26 | === modified file 'qml/Dash/GenericScopeView.qml' |
27 | --- qml/Dash/GenericScopeView.qml 2015-07-03 14:24:08 +0000 |
28 | +++ qml/Dash/GenericScopeView.qml 2015-07-20 08:36:50 +0000 |
29 | @@ -67,6 +67,11 @@ |
30 | subPageLoader.closeSubPage() |
31 | } |
32 | |
33 | + function resetSearch() { |
34 | + if(pageHeaderLoader.item && showPageHeader) |
35 | + pageHeaderLoader.item.resetSearch() |
36 | + } |
37 | + |
38 | function itemClicked(index, result, item, itemModel, resultsModel, limitedCategoryItemCount, categoryId) { |
39 | if (itemModel.uri.indexOf("scope://") === 0 || scope.id === "clickscope" || (scope.id === "videoaggregator" && categoryId === "myvideos-getstarted")) { |
40 | // TODO Technically it is possible that calling activate() will make the scope emit |
41 | |
42 | === modified file 'tests/qmltests/tst_Shell.qml' |
43 | --- tests/qmltests/tst_Shell.qml 2015-06-23 17:40:03 +0000 |
44 | +++ tests/qmltests/tst_Shell.qml 2015-07-20 08:36:50 +0000 |
45 | @@ -472,6 +472,41 @@ |
46 | confirmLoggedIn(data.loggedIn) |
47 | } |
48 | |
49 | + function test_longLeftEdgeSwipeTakesToAppsAndResetSearchString() { |
50 | + loadShell("phone"); |
51 | + swipeAwayGreeter(); |
52 | + dragLauncherIntoView(); |
53 | + dashCommunicatorSpy.clear(); |
54 | + |
55 | + tapOnAppIconInLauncher(); |
56 | + waitUntilApplicationWindowIsFullyVisible(); |
57 | + |
58 | + verify(ApplicationManager.focusedApplicationId !== "unity8-dash") |
59 | + |
60 | + //Long left swipe |
61 | + swipeFromLeftEdge(units.gu(30)); |
62 | + |
63 | + tryCompare(ApplicationManager, "focusedApplicationId", "unity8-dash"); |
64 | + |
65 | + compare(dashCommunicatorSpy.count, 1); |
66 | + } |
67 | + |
68 | + function test_ClickUbuntuIconInLauncherTakesToAppsAndResetSearchString() { |
69 | + loadShell("phone"); |
70 | + swipeAwayGreeter(); |
71 | + dragLauncherIntoView(); |
72 | + dashCommunicatorSpy.clear(); |
73 | + |
74 | + var launcher = findChild(shell, "launcher"); |
75 | + var dashIcon = findChild(launcher, "dashItem"); |
76 | + verify(dashIcon != undefined); |
77 | + mouseClick(dashIcon); |
78 | + |
79 | + tryCompare(ApplicationManager, "focusedApplicationId", "unity8-dash"); |
80 | + |
81 | + compare(dashCommunicatorSpy.count, 1); |
82 | + } |
83 | + |
84 | function test_suspend() { |
85 | loadShell("phone"); |
86 | swipeAwayGreeter(); |
87 | @@ -1082,6 +1117,8 @@ |
88 | } |
89 | |
90 | function test_tapUbuntuIconInLauncherOverAppSpread() { |
91 | + launcherShowDashHomeSpy.clear(); |
92 | + |
93 | loadShell("phone"); |
94 | swipeAwayGreeter(); |
95 |
Thanks for the patch, as you can see on the bug (affects Ubuntu UX as NEW), this is waiting for input by the design team.