Position in updatePolish instead of using single shot timer

Related to QTBUG-18919, but that was not reproduceable.

Rewrites a fair bit of the autotest to no longer rely on repositionings
happening immediately.

Change-Id: I80683d6e03a5543fc838cdd55b2d53e49a8a68bd
Reviewed-on: http://codereview.qt.nokia.com/3690
Reviewed-by: Qt Sanity Bot <qt_sanity_bot@ovi.com>
Reviewed-by: Michael Brasser <michael.brasser@nokia.com>

diff --git a/src/declarative/items/qsgpositioners.cpp b/src/declarative/items/qsgpositioners.cpp
index fca0aa1..0401494 100644 (file)
--- a/src/declarative/items/qsgpositioners.cpp
+++ b/src/declarative/items/qsgpositioners.cpp
@@ -93,6 +93,9 @@ QSGBasePositioner::QSGBasePositioner(PositionerType at, QSGItem *parent)
     for the child items. Depending on the chosen type, only x or y changes will be applied.
 
     Note that the subclass is responsible for adding the spacing in between items.
+
+    Positioning is usually delayed until before a frame is rendered, to batch multiple repositioning
+    changes into one calculation.
 */
 
 QSGBasePositioner::QSGBasePositioner(QSGBasePositionerPrivate &dd, PositionerType at, QSGItem *parent)
@@ -110,6 +113,13 @@ QSGBasePositioner::~QSGBasePositioner()
     positionedItems.clear();
 }
 
+void QSGBasePositioner::updatePolish()
+{
+    Q_D(QSGBasePositioner);
+    if (d->positioningDirty)
+        prePositioning();
+}
+
 int QSGBasePositioner::spacing() const
 {
     Q_D(const QSGBasePositioner);
@@ -122,7 +132,7 @@ void QSGBasePositioner::setSpacing(int s)
     if (s==d->spacing)
         return;
     d->spacing = s;
-    prePositioning();
+    d->setPositioningDirty();
     emit spacingChanged();
 }
 
@@ -169,7 +179,7 @@ void QSGBasePositioner::itemChange(ItemChange change, const ItemChangeData &valu
 {
     Q_D(QSGBasePositioner);
     if (change == ItemChildAddedChange){
-        prePositioning();
+        d->setPositioningDirty();
     } else if (change == ItemChildRemovedChange) {
         QSGItem *child = value.item;
         QSGBasePositioner::PositionedItem posItem(child);
@@ -178,7 +188,7 @@ void QSGBasePositioner::itemChange(ItemChange change, const ItemChangeData &valu
             d->unwatchChanges(child);
             positionedItems.remove(idx);
         }
-        prePositioning();
+        d->setPositioningDirty();
     }
 
     QSGItem::itemChange(change, value);
@@ -193,7 +203,7 @@ void QSGBasePositioner::prePositioning()
     if (d->doingPositioning)
         return;
 
-    d->queuedPositioning = false;
+    d->positioningDirty = false;
     d->doingPositioning = true;
     //Need to order children by creation order modified by stacking order
     QList<QSGItem *> children = childItems();
@@ -463,6 +473,10 @@ void QSGPositionerAttached::setIsLastItem(bool isLastItem)
 
   Items with a width or height of 0 will not be positioned.
 
+  Positioning is batched and syncronized with painting to reduce the number of
+  calculations needed. This means that positioners may not reposition items immediately
+  when changes occur, but it will have moved by the next frame.
+
   \sa Row, Grid, Flow, Positioner, {declarative/positioners}{Positioners example}
 */
 /*!
@@ -607,6 +621,10 @@ void QSGColumn::reportConflictingAnchors()
 
   Items with a width or height of 0 will not be positioned.
 
+  Positioning is batched and syncronized with painting to reduce the number of
+  calculations needed. This means that positioners may not reposition items immediately
+  when changes occur, but it will have moved by the next frame.
+
   \sa Column, Grid, Flow, Positioner, {declarative/positioners}{Positioners example}
 */
 /*!
@@ -838,6 +856,10 @@ void QSGRow::reportConflictingAnchors()
 
   Items with a width or height of 0 will not be positioned.
 
+  Positioning is batched and syncronized with painting to reduce the number of
+  calculations needed. This means that positioners may not reposition items immediately
+  when changes occur, but it will have moved by the next frame.
+
   \sa Flow, Row, Column, Positioner, {declarative/positioners}{Positioners example}
 */
 /*!
@@ -1265,6 +1287,10 @@ void QSGGrid::reportConflictingAnchors()
 
   Items with a width or height of 0 will not be positioned.
 
+  Positioning is batched and syncronized with painting to reduce the number of
+  calculations needed. This means that positioners may not reposition items immediately
+  when changes occur, but it will have moved by the next frame.
+
   \sa Column, Row, Grid, Positioner, {declarative/positioners}{Positioners example}
 */
 /*!

diff --git a/src/declarative/items/qsgpositioners_p.h b/src/declarative/items/qsgpositioners_p.h
index 7200b6d..f871125 100644 (file)
--- a/src/declarative/items/qsgpositioners_p.h
+++ b/src/declarative/items/qsgpositioners_p.h
@@ -121,6 +121,8 @@ protected:
     virtual void itemChange(ItemChange, const ItemChangeData &);
     void finishApplyTransitions();
 
+    virtual void updatePolish();
+
 Q_SIGNALS:
     void spacingChanged();
     void moveChanged();

diff --git a/src/declarative/items/qsgpositioners_p_p.h b/src/declarative/items/qsgpositioners_p_p.h
index a29982b..3c11853 100644 (file)
--- a/src/declarative/items/qsgpositioners_p_p.h
+++ b/src/declarative/items/qsgpositioners_p_p.h
@@ -74,7 +74,7 @@ class QSGBasePositionerPrivate : public QSGImplicitSizeItemPrivate, public QSGIt
 public:
     QSGBasePositionerPrivate()
         : spacing(0), type(QSGBasePositioner::None)
-        , moveTransition(0), addTransition(0), queuedPositioning(false)
+        , moveTransition(0), addTransition(0), positioningDirty(false)
         , doingPositioning(false), anchorConflict(false), layoutDirection(Qt::LeftToRight)
     {
     }
@@ -97,25 +97,23 @@ public:
 
     void watchChanges(QSGItem *other);
     void unwatchChanges(QSGItem* other);
-    bool queuedPositioning : 1;
+    void setPositioningDirty() {
+        Q_Q(QSGBasePositioner);
+        if (!positioningDirty) {
+            positioningDirty = true;
+            q->polish();
+        }
+    }
+
+    bool positioningDirty : 1;
     bool doingPositioning : 1;
     bool anchorConflict : 1;
 
     Qt::LayoutDirection layoutDirection;
 
-    void schedulePositioning()
-    {
-        Q_Q(QSGBasePositioner);
-        if(!queuedPositioning){
-            QTimer::singleShot(0,q,SLOT(prePositioning()));
-            queuedPositioning = true;
-        }
-    }
-
     void mirrorChange() {
-        Q_Q(QSGBasePositioner);
         if (type != QSGBasePositioner::Vertical)
-            q->prePositioning();
+            setPositioningDirty();
     }
     bool isLeftToRight() const {
         if (type == QSGBasePositioner::Vertical)
@@ -127,26 +125,18 @@ public:
     virtual void itemSiblingOrderChanged(QSGItem* other)
     {
         Q_UNUSED(other);
-        //Delay is due to many children often being reordered at once
-        //And we only want to reposition them all once
-        schedulePositioning();
+        setPositioningDirty();
     }
 
     void itemGeometryChanged(QSGItem *, const QRectF &newGeometry, const QRectF &oldGeometry)
     {
-        Q_Q(QSGBasePositioner);
         if (newGeometry.size() != oldGeometry.size())
-            q->prePositioning();
+            setPositioningDirty();
     }
 
     virtual void itemVisibilityChanged(QSGItem *)
     {
-        schedulePositioning();
-    }
-    virtual void itemOpacityChanged(QSGItem *)
-    {
-        Q_Q(QSGBasePositioner);
-        q->prePositioning();
+        setPositioningDirty();
     }
 
     void itemDestroyed(QSGItem *item)

diff --git a/tests/auto/declarative/qsglistview/tst_qsglistview.cpp b/tests/auto/declarative/qsglistview/tst_qsglistview.cpp
index 0cd10f6..45a32da 100644 (file)
--- a/tests/auto/declarative/qsglistview/tst_qsglistview.cpp
+++ b/tests/auto/declarative/qsglistview/tst_qsglistview.cpp
@@ -1124,6 +1124,7 @@ void tst_QSGListView::enforceRange_withoutHighlight()
 
     QSGView *canvas = createView();
     canvas->show();
+    QTest::qWaitForWindowShown(canvas);
 
     TestModel model;
     model.addItem("Item 0", "a");

diff --git a/tests/auto/declarative/qsgpositioners/tst_qsgpositioners.cpp b/tests/auto/declarative/qsgpositioners/tst_qsgpositioners.cpp
index 137e6ac..0a1c4df 100644 (file)
--- a/tests/auto/declarative/qsgpositioners/tst_qsgpositioners.cpp
+++ b/tests/auto/declarative/qsgpositioners/tst_qsgpositioners.cpp
@@ -160,7 +160,7 @@ void tst_qsgpositioners::test_horizontal_rtl()
 
     // Change the width of the row and check that items stay to the right
     row->setWidth(200);
-    QCOMPARE(one->x(), 150.0);
+    QTRY_COMPARE(one->x(), 150.0);
     QCOMPARE(one->y(), 0.0);
     QCOMPARE(two->x(), 130.0);
     QCOMPARE(two->y(), 0.0);
@@ -580,7 +580,7 @@ void tst_qsgpositioners::test_grid_rightToLeft()
 
     // Change the width of the grid and check that items stay to the right
     grid->setWidth(200);
-    QCOMPARE(one->x(), 150.0);
+    QTRY_COMPARE(one->x(), 150.0);
     QCOMPARE(one->y(), 0.0);
     QCOMPARE(two->x(), 130.0);
     QCOMPARE(two->y(), 0.0);
@@ -1086,16 +1086,16 @@ void tst_qsgpositioners::test_flow_resize()
     QSGRectangle *five = canvas->rootObject()->findChild<QSGRectangle*>("five");
     QVERIFY(five != 0);
 
-    QCOMPARE(one->x(), 0.0);
-    QCOMPARE(one->y(), 0.0);
-    QCOMPARE(two->x(), 50.0);
-    QCOMPARE(two->y(), 0.0);
-    QCOMPARE(three->x(), 70.0);
-    QCOMPARE(three->y(), 0.0);
-    QCOMPARE(four->x(), 0.0);
-    QCOMPARE(four->y(), 50.0);
-    QCOMPARE(five->x(), 50.0);
-    QCOMPARE(five->y(), 50.0);
+    QTRY_COMPARE(one->x(), 0.0);
+    QTRY_COMPARE(one->y(), 0.0);
+    QTRY_COMPARE(two->x(), 50.0);
+    QTRY_COMPARE(two->y(), 0.0);
+    QTRY_COMPARE(three->x(), 70.0);
+    QTRY_COMPARE(three->y(), 0.0);
+    QTRY_COMPARE(four->x(), 0.0);
+    QTRY_COMPARE(four->y(), 50.0);
+    QTRY_COMPARE(five->x(), 50.0);
+    QTRY_COMPARE(five->y(), 50.0);
 
     delete canvas;
 }
@@ -1110,7 +1110,7 @@ void tst_qsgpositioners::test_flow_resize_rightToLeft()
     root->setProperty("testRightToLeft", true);
 
     QSGRectangle *one = canvas->rootObject()->findChild<QSGRectangle*>("one");
-    QVERIFY(one != 0);
+    QTRY_VERIFY(one != 0);
     QSGRectangle *two = canvas->rootObject()->findChild<QSGRectangle*>("two");
     QVERIFY(two != 0);
     QSGRectangle *three = canvas->rootObject()->findChild<QSGRectangle*>("three");
@@ -1294,7 +1294,7 @@ void tst_qsgpositioners::test_mirroring()
                 break;
             QSGItem *itemA = rootA->findChild<QSGItem*>(objectName);
             QSGItem *itemB = rootB->findChild<QSGItem*>(objectName);
-            QVERIFY(itemA->x() != itemB->x());
+            QTRY_VERIFY(itemA->x() != itemB->x());
         }
 
         QSGItemPrivate* rootPrivateB = QSGItemPrivate::get(rootB);
@@ -1311,7 +1311,7 @@ void tst_qsgpositioners::test_mirroring()
                 break;
             QSGItem *itemA = rootA->findChild<QSGItem*>(objectName);
             QSGItem *itemB = rootB->findChild<QSGItem*>(objectName);
-            QCOMPARE(itemA->x(), itemB->x());
+            QTRY_COMPARE(itemA->x(), itemB->x());
         }
 
         rootA->setProperty("testRightToLeft", false); // layoutDirection: Qt.LeftToRight
@@ -1324,7 +1324,7 @@ void tst_qsgpositioners::test_mirroring()
                 break;
             QSGItem *itemA = rootA->findChild<QSGItem*>(objectName);
             QSGItem *itemB = rootB->findChild<QSGItem*>(objectName);
-            QCOMPARE(itemA->x(), itemB->x());
+            QTRY_COMPARE(itemA->x(), itemB->x());
         }
         delete canvasA;
         delete canvasB;
@@ -1428,12 +1428,9 @@ void tst_qsgpositioners::test_attachedproperties_dynamic()
 
     row->metaObject()->invokeMethod(row, "createSubRect");
 
-    posIndex = rect1->property("index").toInt();
-    QVERIFY(posIndex == 1);
-    isFirst = rect1->property("firstItem").toBool();
-    QVERIFY(isFirst == false);
-    isLast = rect1->property("lastItem").toBool();
-    QVERIFY(isLast == false);
+    QTRY_VERIFY(rect1->property("index").toInt() == 1);
+    QTRY_VERIFY(rect1->property("firstItem").toBool() == false);
+    QTRY_VERIFY(rect1->property("lastItem").toBool() == false);
 
     QSGRectangle *rect2 = canvas->rootObject()->findChild<QSGRectangle *>("rect2");
     QVERIFY(rect2 != 0);
@@ -1449,12 +1446,9 @@ void tst_qsgpositioners::test_attachedproperties_dynamic()
 
     qApp->processEvents(QEventLoop::DeferredDeletion);
 
-    posIndex = rect1->property("index").toInt();
-    QVERIFY(posIndex == 1);
-    isFirst = rect1->property("firstItem").toBool();
-    QVERIFY(isFirst == false);
-    isLast = rect1->property("lastItem").toBool();
-    QVERIFY(isLast == true);
+    QTRY_VERIFY(rect1->property("index").toInt() == 1);
+    QTRY_VERIFY(rect1->property("firstItem").toBool() == false);
+    QTRY_VERIFY(rect1->property("lastItem").toBool() == true);
 
     delete canvas;
 }
@@ -1464,6 +1458,8 @@ QSGView *tst_qsgpositioners::createView(const QString &filename)
     QSGView *canvas = new QSGView(0);
 
     canvas->setSource(QUrl::fromLocalFile(filename));
+    canvas->show();
+    QTest::qWaitForWindowShown(canvas); //It may not relayout until the next frame, so it needs to be drawn
 
     return canvas;
 }