Unity 8

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

lvwphnoprocessevents
Merge into trunk

Proposed by Albert Astals Cid on 2014-03-28

Status:

Merged

Approved by:

Michael Zanetti on 2014-03-31

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

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Zanetti (community)		2014-03-28	Approve on 2014-03-31
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2014-03-31
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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-28:

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 on 2014-03-30

796. By Albert Astals Cid on 2014-03-30: whitespace

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-30:

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 on 2014-03-31

797. By Albert Astals Cid on 2014-03-31: Merge
798. By Albert Astals Cid on 2014-03-31: Make test work both under xvfb and non xvfb

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-31:

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 on 2014-03-31:

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 on 2014-03-31:

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

Subscribers

People subscribed via source and target branches

to all changes:

Albert Astals Cid

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'plugins/DashViews/listviewwithpageheader.cpp'
2	--- plugins/DashViews/listviewwithpageheader.cpp 2014-03-24 16:10:08 +0000
3	+++ plugins/DashViews/listviewwithpageheader.cpp 2014-03-31 09:04:06 +0000
4	@@ -1097,7 +1097,7 @@
5	{
6	const qreal heightDiff = newGeometry.height() - oldGeometry.height();
7	if (heightDiff != 0) {
8	- if (oldGeometry.y() + oldGeometry.height() + m_clipItem->y() <= contentY() && !m_visibleItems.isEmpty()) {
9	+ if (!m_inContentHeightKeepHeaderShown && oldGeometry.y() + oldGeometry.height() + m_clipItem->y() <= contentY() && !m_visibleItems.isEmpty()) {
10	ListItem *firstItem = m_visibleItems.first();
11	firstItem->setY(firstItem->y() - heightDiff);
12	adjustMinYExtent();
13	@@ -1293,11 +1293,6 @@
14	refill();
15
16	if (m_contentHeightDirty) {
17	- // We need to make sure all bindings have updated otherwise
18	- // we may end up with bindings updating when we call setContentHeight
19	- // and then everything gets out of sync, i.e. testHeaderPositionBug1240118
20	- QCoreApplication::instance()->processEvents();
21	-
22	qreal contentHeight;
23	if (m_visibleItems.isEmpty()) {
24	contentHeight = m_headerItem ? m_headerItem->height() : 0;
25
26	=== modified file 'tests/plugins/DashViews/listviewwithpageheadertest.cpp'
27	--- tests/plugins/DashViews/listviewwithpageheadertest.cpp 2014-03-24 15:23:39 +0000
28	+++ tests/plugins/DashViews/listviewwithpageheadertest.cpp 2014-03-31 09:04:06 +0000
29	@@ -1855,7 +1855,6 @@
30	scrollToBottom();
31	lvwph->showHeader();
32	QTRY_VERIFY(!lvwph->m_contentYAnimation->isRunning());
33	- QTest::qWait(100); // Make sure stuff is stable
34	QMetaObject::invokeMethod(model, "insertItem", Q_ARG(QVariant, 0), Q_ARG(QVariant, 100));
35	model->setProperty(3, "size", 10);
36	model->setProperty(4, "size", 10);
37
38	=== modified file 'tests/plugins/DashViews/listviewwithpageheadertestsection.cpp'
39	--- tests/plugins/DashViews/listviewwithpageheadertestsection.cpp 2014-03-06 17:30:53 +0000
40	+++ tests/plugins/DashViews/listviewwithpageheadertestsection.cpp 2014-03-31 09:04:06 +0000
41	@@ -1680,12 +1680,11 @@
42	verifyItem(18, 700, 50., false, QString(), true);
43	verifyItem(19, 750, 50., false, QString(), true);
44	verifyItem(20, 800, 50., false, QString(), true);
45	- QCOMPARE(lvwph->m_minYExtent, 5 * 1510./21. + 660 - 1970 + 50);
46	- QCOMPARE(lvwph->m_clipItem->y(), 1970.);
47	+ QCOMPARE(lvwph->m_clipItem->y(), lvwph->contentY());
48	+ QCOMPARE(lvwph->m_minYExtent, 5 * 1510./21. + 660 - lvwph->contentY() + 50);
49	QCOMPARE(lvwph->m_clipItem->clip(), false);
50	QCOMPARE(lvwph->m_headerItem->y(), 0.);
51	QCOMPARE(lvwph->m_headerItem->height(), 50.);
52	- QCOMPARE(lvwph->contentY(), 1970.);
53	QCOMPARE(lvwph->m_headerItemShownHeight, 0.);
54	}
55
56	@@ -2108,6 +2107,7 @@
57	QVERIFY(QQuickItemPrivate::get(lvwph->m_topSectionItem)->culled);
58
59	QTest::qWait(1);
60	+ QCoreApplication::instance()->processEvents();
61
62	changeContentY(-15);
63