Ubuntu UI Toolkit

Merge lp:~tpeeters/ubuntu-ui-toolkit/lessHeaderDisabling into lp:ubuntu-ui-toolkit/staging

  1. lessHeaderDisabling
  2. Merge into staging
Proposed by Tim Peeters on 2015-11-17
Status: Merged
Approved by: Zsombor Egri on 2015-11-19
Approved revision: 1723
Merged at revision: 1720
Proposed branch: lp:~tpeeters/ubuntu-ui-toolkit/lessHeaderDisabling
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 126 lines (+61/-4)
2 files modified
src/Ubuntu/Components/plugin/ucheader.cpp (+7/-2)
tests/unit_x11/tst_components/tst_header.qml (+54/-2)
To merge this branch: bzr merge lp:~tpeeters/ubuntu-ui-toolkit/lessHeaderDisabling
Reviewer Review Type Date Requested Status
Zsombor Egri (community) 2015-11-17 Approve on 2015-11-19
Victor Thompson (community) Approve on 2015-11-19
PS Jenkins bot continuous-integration Approve on 2015-11-18
Andrew Hayzen (community) functional Approve on 2015-11-18
Review via email: mp+277713@code.launchpad.net

Commit Message

Don't set the header.moving property when flickable contents height changes but the header does not actually move.

Description of the Change

Please review, but don't top-approve yet. Let's discuss if this should be a hotfix that goes directly to trunk.

To post a comment you must log in.
Zsombor Egri (zsombi) wrote :

Are you sure this makes it working? Text input is made inactive while typed in it... was that disabled because the Flickable underneath was refilled?

review: Needs Information
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Tim Peeters (tpeeters) wrote :

Yes, I used this program to test it: http://paste.ubuntu.com/13319267/

When the contents height of the flickable is changed, and the new content height is smaller than the flickable height (so scrolling is no longer possible), header.show() is called in UCHeader.cpp in order to avoid getting in a state where the header is hidden while you cannot scroll the flickable to reveal it again. Only in this case that is not needed because the header was already visible, but still show() sets the moving property to true for just an instance, and when moving, the contents of the header is disabled (to avoid tapping a button by accident while the header moves). That is now avoided with my changes.

Andrew Hayzen (ahayzen) wrote :

When testing with the music-app revision 940 (before we added a workaround) this branch now does not disable the header while the view is changing.

However I have noticed if you scroll down the list, then perform a search, you sometimes get the header being disabled. This seems to be when many items are cleared at once, but is much better than before. Is there anyway this case can be fixed as well or is this not avoidable?

review: Needs Information (functional)
lp:~tpeeters/ubuntu-ui-toolkit/lessHeaderDisabling updated on 2015-11-18
1719. By Tim Peeters on 2015-11-18

fix second part of the bug (scrolling); add unit tests

1720. By Tim Peeters on 2015-11-18

remove new blank line

1721. By Tim Peeters on 2015-11-18

comment in unit test file

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Andrew Hayzen (ahayzen) wrote :

This now appears to fix the issue of the header being disabled when the view is resizing while typing in a search field.

review: Approve (functional)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~tpeeters/ubuntu-ui-toolkit/lessHeaderDisabling updated on 2015-11-18
1722. By Tim Peeters on 2015-11-18

kick jenkins. It passed for r1720 and failed for r1721, the difference was the text in a comment

1723. By Tim Peeters on 2015-11-18

formatting

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Victor Thompson (vthompson) wrote :

In order to source the built plugins I need to make the following change to the export_modules_dir.sh file (otherwise dirname $0 evaluated to /bin). It'd be nice to make this change if exporting the modules the way the README suggests doesn't work for others--could be user error:

bzr diff
=== modified file 'export_modules_dir.sh'
--- export_modules_dir.sh 2015-05-19 07:55:27 +0000
+++ export_modules_dir.sh 2015-11-19 02:03:27 +0000
@@ -15,7 +15,7 @@
 # along with this program. If not, see <http://www.gnu.org/licenses/>.
 #

-. `dirname $0`/build_paths.inc
+. ./build_paths.inc
 export QML_IMPORT_PATH=$BUILD_DIR/qml
 export QML2_IMPORT_PATH=$BUILD_DIR/qml
 export UBUNTU_UI_TOOLKIT_THEMES_PATH=$BUILD_DIR/qml

Otherwise, this works great!

review: Approve
Zsombor Egri (zsombi) wrote :

Ok, code looks good, and I got convinced now that it fixes the issues. Thanks!

review: Approve
Tim Peeters (tpeeters) wrote :

Thanks for the review zsombi.

Victor, I never encountered the issue that you have with export_modules_dir.sh. Can you report that as a separate bug? thanks.

Andrew Hayzen (ahayzen) wrote :

For reference, I have reported bug 1518106 to track the export_modules_dir.sh issue.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Ubuntu/Components/plugin/ucheader.cpp'
2--- src/Ubuntu/Components/plugin/ucheader.cpp 2015-10-01 21:23:04 +0000
3+++ src/Ubuntu/Components/plugin/ucheader.cpp 2015-11-18 19:52:42 +0000
4@@ -208,6 +208,7 @@
5 }
6
7 void UCHeader::show(bool animate) {
8+ if (m_exposed && !m_moving && y() == 0.0) return;
9 if (!m_exposed) {
10 m_exposed = true;
11 Q_EMIT exposedChanged();
12@@ -233,6 +234,7 @@
13 }
14
15 void UCHeader::hide(bool animate) {
16+ if (!m_exposed && !m_moving && y() == -1.0*height()) return;
17 if (m_exposed) {
18 m_exposed = false;
19 Q_EMIT exposedChanged();
20@@ -310,8 +312,11 @@
21 }
22 m_previous_contentY = m_flickable->contentY();
23 if (!m_moving) {
24- m_moving = true;
25- Q_EMIT movingChanged();
26+ bool move = m_exposed ? y() != 0.0 : y() != -height();
27+ if (move) {
28+ m_moving = true;
29+ Q_EMIT movingChanged();
30+ }
31 }
32 if (!m_flickable->isMoving()) {
33 // m_flickable.contentY was set directly, so no user flicking.
34
35=== modified file 'tests/unit_x11/tst_components/tst_header.qml'
36--- tests/unit_x11/tst_components/tst_header.qml 2015-10-01 21:23:04 +0000
37+++ tests/unit_x11/tst_components/tst_header.qml 2015-11-18 19:52:42 +0000
38@@ -42,6 +42,15 @@
39 width: 2
40 }
41 }
42+
43+ onMovingChanged: {
44+ // The value of movingLabel.text is used in:
45+ // - test_dont_move_when_flickable_shortens_bug1514143()
46+ // - test_dont_move_exposed_header_when_scrolling_down_bug1514143()
47+ // - test_dont_move_hidden_header_when_scrolling_up()
48+ movingLabel.text = "Moving changed to " + moving;
49+ movingLabel.color = moving ? "purple" : "red";
50+ }
51 }
52
53 Flickable {
54@@ -105,13 +114,22 @@
55 text: "Set contentY to " + newY
56 }
57 Label {
58+ id: flickLabel
59 anchors {
60- top : contentYButton.bottom
61+ top: contentYButton.bottom
62 horizontalCenter: parent.horizontalCenter
63 topMargin: units.gu(8)
64 }
65 text: "Flick me"
66 }
67+ Label {
68+ id: movingLabel
69+ anchors {
70+ top: flickLabel.bottom
71+ horizontalCenter: parent.horizontalCenter
72+ }
73+ text: "hmm"
74+ }
75 }
76
77 Rectangle {
78@@ -189,7 +207,7 @@
79 var p = centerOf(flickable);
80 // Use mouseWheel to scroll because mouseDrag is very unreliable
81 // and does not properly handle negative values for dy.
82- mouseWheel(flickable, p.x, p.y, 0,dy);
83+ mouseWheel(flickable, p.x, p.y, 0, dy);
84 }
85
86 function scroll_down() {
87@@ -368,5 +386,39 @@
88
89 header.flickable = flickable;
90 }
91+
92+ function test_dont_move_when_flickable_shortens_bug1514143() {
93+ var flickableContentHeight = flickable.contentHeight;
94+ movingLabel.text = "HEADER DID NOT MOVE";
95+ flickable.contentHeight = 200;
96+ compare(movingLabel.text, "HEADER DID NOT MOVE",
97+ "Reducing flickable contents height unneccessary sets header.moving.");
98+ flickable.contentHeight = flickableContentHeight;
99+ compare(movingLabel.text, "HEADER DID NOT MOVE",
100+ "Increasing flickable contents height unneccessary sets header.moving.");
101+ }
102+
103+ function test_dont_move_exposed_header_when_scrolling_down_bug1514143() {
104+ scroll_down(); scroll_down();
105+ wait_for_exposed(false, "Header doesn't hide when scrolling down.");
106+ header.exposed = true;
107+ wait_for_exposed(true, "Cannot expose header after scrolling down.");
108+ movingLabel.text = "HEADER DID NOT MOVE";
109+ scroll_up();
110+ wait(100);
111+ compare(movingLabel.text, "HEADER DID NOT MOVE",
112+ "Header moved when scrolling up while header was already exposed.");
113+ }
114+
115+ function test_dont_move_hidden_header_when_scrolling_up() {
116+ // flickable is at the top.
117+ header.exposed = false;
118+ wait_for_exposed(false, "Cannot hide header.");
119+ movingLabel.text = "HEADER DID NOT MOVE";
120+ scroll_down();
121+ wait(100);
122+ compare(movingLabel.text, "HEADER DID NOT MOVE",
123+ "Header moved when scrolling down while header was already hidden.");
124+ }
125 }
126 }

Subscribers

People subscribed via source and target branches