Merge lp:~aacid/unity8/lvwph_bad_header_position_1240118 into lp:unity8

Proposed by Albert Astals Cid on 2013-10-30
Status: Merged
Approved by: Nick Dedekind on 2013-11-13
Approved revision: 485
Merged at revision: 527
Proposed branch: lp:~aacid/unity8/lvwph_bad_header_position_1240118
Merge into: lp:unity8
Diff against target: 70 lines (+22/-1)
3 files modified
plugins/ListViewWithPageHeader/listviewwithpageheader.cpp (+4/-1)
plugins/ListViewWithPageHeader/listviewwithpageheader.h (+1/-0)
tests/plugins/ListViewWithPageHeader/listviewwithpageheadertest.cpp (+17/-0)
To merge this branch: bzr merge lp:~aacid/unity8/lvwph_bad_header_position_1240118
Reviewer Review Type Date Requested Status
Nick Dedekind (community) 2013-10-30 Approve on 2013-11-13
PS Jenkins bot (community) continuous-integration Approve on 2013-11-04
Michal Hruby (community) 2013-10-30 Approve on 2013-10-31
Review via email: mp+193200@code.launchpad.net

Commit message

LVWPH: Fix header going bad when setContentHeight ends up moving the viewport

How to reproduce the bug easily without the patch:
 * In the Dash Home, search for london
 * Scroll to the bottom
 * Start moving to the apps scope very slowly
 * At around 3/4 of the move you'll see the header in the home scope went to a bad position
 * Go back to the Dash Home

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:482
http://jenkins.qa.ubuntu.com/job/unity8-ci/1521/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/187
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/181/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/45
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/45
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/176
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/187
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/187/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/181
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/181/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-maguro/2804/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/2855
    SUCCESS: http://10.97.0.26:8080/job/touch-flash-device/648
    SUCCESS: http://10.97.0.26:8080/job/touch-flash-device/647

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1521/rebuild

review: Needs Fixing (continuous-integration)
Michal Hruby (mhr3) wrote :

Fixes the incorrect positioning for me, although the code looks a bit fragile :)

review: Approve
483. By Albert Astals Cid on 2013-11-04

Merge lp:unity8

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
484. By Albert Astals Cid on 2013-11-04

booo

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
485. By Albert Astals Cid on 2013-11-04

Enable the check later?

I'm pretty convinced it's unstability has to do with the non great qml engine loop in 5.0

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Nick Dedekind (nick-dedekind) wrote :

Ok, don't really understand the code, but it fixes the issue.
Test successfully tests for regressions.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/ListViewWithPageHeader/listviewwithpageheader.cpp'
2--- plugins/ListViewWithPageHeader/listviewwithpageheader.cpp 2013-10-03 11:48:24 +0000
3+++ plugins/ListViewWithPageHeader/listviewwithpageheader.cpp 2013-11-04 12:34:19 +0000
4@@ -155,6 +155,7 @@
5 , m_topSectionItem(nullptr)
6 , m_forceNoClip(false)
7 , m_inLayout(false)
8+ , m_inContentHeightKeepHeaderShown(false)
9 {
10 m_clipItem = new QQuickItem(contentItem());
11 // m_clipItem = new QQuickRectangle(contentItem());
12@@ -494,7 +495,7 @@
13 if (!scrolledUp && contentY() == -m_minYExtent) {
14 m_headerItemShownHeight = 0;
15 m_headerItem->setY(contentY());
16- } else if ((scrolledUp && notRebounding && notShownByItsOwn && !maximizeVisibleAreaRunning) || (m_headerItemShownHeight > 0)) {
17+ } else if ((scrolledUp && notRebounding && notShownByItsOwn && !maximizeVisibleAreaRunning) || (m_headerItemShownHeight > 0) || m_inContentHeightKeepHeaderShown) {
18 if (maximizeVisibleAreaRunning && diff > 0) // If we are maximizing and the header was shown, make sure we hide it
19 m_headerItemShownHeight -= diff;
20 else
21@@ -1267,7 +1268,9 @@
22
23 m_contentHeightDirty = false;
24 adjustMinYExtent();
25+ m_inContentHeightKeepHeaderShown = m_headerItem && m_headerItem->y() == contentY();
26 setContentHeight(contentHeight);
27+ m_inContentHeightKeepHeaderShown = false;
28 }
29 }
30
31
32=== modified file 'plugins/ListViewWithPageHeader/listviewwithpageheader.h'
33--- plugins/ListViewWithPageHeader/listviewwithpageheader.h 2013-09-02 13:22:33 +0000
34+++ plugins/ListViewWithPageHeader/listviewwithpageheader.h 2013-11-04 12:34:19 +0000
35@@ -202,6 +202,7 @@
36
37 bool m_forceNoClip;
38 bool m_inLayout;
39+ bool m_inContentHeightKeepHeaderShown;
40
41 // Qt 5.0 doesn't like releasing the items just after itemCreated
42 // so we delay the releasing until the next updatePolish
43
44=== modified file 'tests/plugins/ListViewWithPageHeader/listviewwithpageheadertest.cpp'
45--- tests/plugins/ListViewWithPageHeader/listviewwithpageheadertest.cpp 2013-10-02 13:20:56 +0000
46+++ tests/plugins/ListViewWithPageHeader/listviewwithpageheadertest.cpp 2013-11-04 12:34:19 +0000
47@@ -1824,6 +1824,23 @@
48 lvwph->positionAtBeginning();
49 }
50
51+ void testHeaderPositionBug1240118()
52+ {
53+ scrollToBottom();
54+ lvwph->showHeader();
55+ QTRY_VERIFY(!lvwph->m_contentYAnimation->isRunning());
56+ QTest::qWait(100); // Make sure stuff is stable
57+ QMetaObject::invokeMethod(model, "insertItem", Q_ARG(QVariant, 0), Q_ARG(QVariant, 100));
58+ model->setProperty(3, "size", 10);
59+ model->setProperty(4, "size", 10);
60+ model->setProperty(5, "size", 10);
61+ model->setProperty(6, "size", 10);
62+#if (QT_VERSION > QT_VERSION_CHECK(5, 0, 3))
63+ QTRY_COMPARE(lvwph->m_minYExtent, 210.);
64+#endif
65+ QTRY_COMPARE(lvwph->m_headerItem->y(), -lvwph->m_minYExtent);
66+ }
67+
68 private:
69 QQuickView *view;
70 ListViewWithPageHeader *lvwph;

Subscribers

People subscribed via source and target branches