Unity 8

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

  1. lvwphnoprocessevents
  2. Merge into trunk
Proposed by Albert Astals Cid
Status: Merged
Approved by: Michael Zanetti
Approved revision: 798
Merged at revision: 802
Proposed branch: lp:~aacid/unity8/lvwphnoprocessevents
Merge into: lp:unity8
Diff against target: 62 lines (+4/-10)
3 files modified
plugins/DashViews/listviewwithpageheader.cpp (+1/-6)
tests/plugins/DashViews/listviewwithpageheadertest.cpp (+0/-1)
tests/plugins/DashViews/listviewwithpageheadertestsection.cpp (+3/-3)
To merge this branch: bzr merge lp:~aacid/unity8/lvwphnoprocessevents
Reviewer Review Type Date Requested Status
Michael Zanetti (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+213306@code.launchpad.net

Commit message

LVWPH: Remove processEvents() call from updatePolish()

It causes some reentrancy issues and in some times you end up in
polishItems() with items that have been deleted because you called processEvents()

This means i need a small tweak in itemGeometryChanged to not reposition items
if we are inside a setContentHeight call and two small tweaks to tests
since now things happen in a different order and numbers are different (though equivalent)

Description of the change

* 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

* 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

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:795
http://jenkins.qa.ubuntu.com/job/unity8-ci/2669/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/4350/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/3949/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/1539/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/1190/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1194/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/1190/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4449/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3954/console

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

review: Needs Fixing (continuous-integration)
lp:~aacid/unity8/lvwphnoprocessevents updated
796. By Albert Astals Cid

whitespace

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:796
http://jenkins.qa.ubuntu.com/job/unity8-ci/2672/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/4356
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/3954
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/1542
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/1193
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1197
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1197/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/1193
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/3774
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4455
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4455/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3970
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3970/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/6230
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5398

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

review: Needs Fixing (continuous-integration)
lp:~aacid/unity8/lvwphnoprocessevents updated
797. By Albert Astals Cid

Merge

798. By Albert Astals Cid

Make test work both under xvfb and non xvfb

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:797
http://jenkins.qa.ubuntu.com/job/unity8-ci/2673/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/4366
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/3959
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/1543
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/1194
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1198
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1198/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/1194
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/3784
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4465
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4465/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3980
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3980/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/6235
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5418

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:798
http://jenkins.qa.ubuntu.com/job/unity8-ci/2674/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/4368
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/3961
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/1544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/1195
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1199
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1199/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/1195
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/3786
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4467
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/4467/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3982
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3982/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/6237
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5421

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Zanetti (mzanetti) wrote :

I wasn't able to reproduce the crash it's supposed to fix. However, I've tested it with this fix too and everything seems to work fine. Doesn't break the thing why the processEvents() was initially introduced.

 * Did you perform an exploratory manual test run of the code change and any related functionality?

Yes.

 * Did CI run pass? If not, please explain why.

No. The qmltests failure should be resolved by now. The previously failed test is now passing here in both, normal run and xvfb-run.

I don't think the AP failure is related. However, seems to be a real failure we should investigate in.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/DashViews/listviewwithpageheader.cpp'
--- plugins/DashViews/listviewwithpageheader.cpp 2014-03-24 16:10:08 +0000
+++ plugins/DashViews/listviewwithpageheader.cpp 2014-03-31 09:04:06 +0000
@@ -1097,7 +1097,7 @@
1097{1097{
1098 const qreal heightDiff = newGeometry.height() - oldGeometry.height();1098 const qreal heightDiff = newGeometry.height() - oldGeometry.height();
1099 if (heightDiff != 0) {1099 if (heightDiff != 0) {
1100 if (oldGeometry.y() + oldGeometry.height() + m_clipItem->y() <= contentY() && !m_visibleItems.isEmpty()) {1100 if (!m_inContentHeightKeepHeaderShown && oldGeometry.y() + oldGeometry.height() + m_clipItem->y() <= contentY() && !m_visibleItems.isEmpty()) {
1101 ListItem *firstItem = m_visibleItems.first();1101 ListItem *firstItem = m_visibleItems.first();
1102 firstItem->setY(firstItem->y() - heightDiff);1102 firstItem->setY(firstItem->y() - heightDiff);
1103 adjustMinYExtent();1103 adjustMinYExtent();
@@ -1293,11 +1293,6 @@
1293 refill();1293 refill();
12941294
1295 if (m_contentHeightDirty) {1295 if (m_contentHeightDirty) {
1296 // We need to make sure all bindings have updated otherwise
1297 // we may end up with bindings updating when we call setContentHeight
1298 // and then everything gets out of sync, i.e. testHeaderPositionBug1240118
1299 QCoreApplication::instance()->processEvents();
1300
1301 qreal contentHeight;1296 qreal contentHeight;
1302 if (m_visibleItems.isEmpty()) {1297 if (m_visibleItems.isEmpty()) {
1303 contentHeight = m_headerItem ? m_headerItem->height() : 0;1298 contentHeight = m_headerItem ? m_headerItem->height() : 0;
13041299
=== modified file 'tests/plugins/DashViews/listviewwithpageheadertest.cpp'
--- tests/plugins/DashViews/listviewwithpageheadertest.cpp 2014-03-24 15:23:39 +0000
+++ tests/plugins/DashViews/listviewwithpageheadertest.cpp 2014-03-31 09:04:06 +0000
@@ -1855,7 +1855,6 @@
1855 scrollToBottom();1855 scrollToBottom();
1856 lvwph->showHeader();1856 lvwph->showHeader();
1857 QTRY_VERIFY(!lvwph->m_contentYAnimation->isRunning());1857 QTRY_VERIFY(!lvwph->m_contentYAnimation->isRunning());
1858 QTest::qWait(100); // Make sure stuff is stable
1859 QMetaObject::invokeMethod(model, "insertItem", Q_ARG(QVariant, 0), Q_ARG(QVariant, 100));1858 QMetaObject::invokeMethod(model, "insertItem", Q_ARG(QVariant, 0), Q_ARG(QVariant, 100));
1860 model->setProperty(3, "size", 10);1859 model->setProperty(3, "size", 10);
1861 model->setProperty(4, "size", 10);1860 model->setProperty(4, "size", 10);
18621861
=== modified file 'tests/plugins/DashViews/listviewwithpageheadertestsection.cpp'
--- tests/plugins/DashViews/listviewwithpageheadertestsection.cpp 2014-03-06 17:30:53 +0000
+++ tests/plugins/DashViews/listviewwithpageheadertestsection.cpp 2014-03-31 09:04:06 +0000
@@ -1680,12 +1680,11 @@
1680 verifyItem(18, 700, 50., false, QString(), true);1680 verifyItem(18, 700, 50., false, QString(), true);
1681 verifyItem(19, 750, 50., false, QString(), true);1681 verifyItem(19, 750, 50., false, QString(), true);
1682 verifyItem(20, 800, 50., false, QString(), true);1682 verifyItem(20, 800, 50., false, QString(), true);
1683 QCOMPARE(lvwph->m_minYExtent, 5 * 1510./21. + 660 - 1970 + 50);1683 QCOMPARE(lvwph->m_clipItem->y(), lvwph->contentY());
1684 QCOMPARE(lvwph->m_clipItem->y(), 1970.);1684 QCOMPARE(lvwph->m_minYExtent, 5 * 1510./21. + 660 - lvwph->contentY() + 50);
1685 QCOMPARE(lvwph->m_clipItem->clip(), false);1685 QCOMPARE(lvwph->m_clipItem->clip(), false);
1686 QCOMPARE(lvwph->m_headerItem->y(), 0.);1686 QCOMPARE(lvwph->m_headerItem->y(), 0.);
1687 QCOMPARE(lvwph->m_headerItem->height(), 50.);1687 QCOMPARE(lvwph->m_headerItem->height(), 50.);
1688 QCOMPARE(lvwph->contentY(), 1970.);
1689 QCOMPARE(lvwph->m_headerItemShownHeight, 0.);1688 QCOMPARE(lvwph->m_headerItemShownHeight, 0.);
1690 }1689 }
16911690
@@ -2108,6 +2107,7 @@
2108 QVERIFY(QQuickItemPrivate::get(lvwph->m_topSectionItem)->culled);2107 QVERIFY(QQuickItemPrivate::get(lvwph->m_topSectionItem)->culled);
21092108
2110 QTest::qWait(1);2109 QTest::qWait(1);
2110 QCoreApplication::instance()->processEvents();
21112111
2112 changeContentY(-15);2112 changeContentY(-15);
21132113

Subscribers

People subscribed via source and target branches