Fix binding loop and polish issue in quick layout

The quick layout item caused a binding loop issue when layout item sizes
were updated in between polish. This has been fixed by not allowing
rearrange during geometry change, if the layout is already dirty due to
polish event.

There is also a polish issue due to the child item not being invalidated
and it can lead to skipping the corresponding item box size calculation.
This patch invalidates all the items in the rearrange list of the layout
and finally, invalidates the engine and layout.

The threshold limit to trigger layout polish has also been removed now.

Fixes: QTBUG-117899
Fixes: QTBUG-118511
Pick-to: 6.7 6.6 6.5
Change-Id: Ie5713f51ed9e428786046bb06a822e5668beebb0
Reviewed-by: Lars Schmertmann <lars.schmertmann@governikus.de>
Reviewed-by: Jan Arve Sæther <jan-arve.saether@qt.io>

3 files changed, 93 insertions, 27 deletions

diff --git a/src/quicklayouts/qquicklayout.cpp b/src/quicklayouts/qquicklayout.cpp
index eb4a8f0c51..f38bdfd396 100644
--- a/src/quicklayouts/qquicklayout.cpp
+++ b/src/quicklayouts/qquicklayout.cpp
@@ -835,20 +835,16 @@ void QQuickLayout::invalidate(QQuickItem * /*childItem*/)
     d->m_dirtyArrangement = true;
 
     if (!qobject_cast<QQuickLayout *>(parentItem())) {
-
-        if (m_inUpdatePolish)
-            ++m_polishInsideUpdatePolish;
-        else
-            m_polishInsideUpdatePolish = 0;
-
-        if (m_polishInsideUpdatePolish <= 2) {
-            // allow at most two consecutive loops in order to respond to height-for-width
-            // (e.g QQuickText changes implicitHeight when its width gets changed)
-            qCDebug(lcQuickLayouts) << "QQuickLayout::invalidate(), polish()";
-            polish();
+        polish();
+
+        if (m_inUpdatePolish) {
+            if (++m_polishInsideUpdatePolish > 2)
+                // allow at most two consecutive loops in order to respond to height-for-width
+                // (e.g QQuickText changes implicitHeight when its width gets changed)
+                qCDebug(lcQuickLayouts) << "Layout polish loop detected for " << this
+                            << ". The polish request will still be scheduled.";
         } else {
-            qmlWarning(this).nospace() << "Layout polish loop detected for " << this
-                << ". Aborting after two iterations.";
+            m_polishInsideUpdatePolish = 0;
         }
     }
 }
@@ -927,7 +923,7 @@ void QQuickLayout::geometryChange(const QRectF &newGeometry, const QRectF &oldGe
 {
     Q_D(QQuickLayout);
     QQuickItem::geometryChange(newGeometry, oldGeometry);
-    if (d->m_disableRearrange || !isReady())
+    if (invalidated() || d->m_disableRearrange || !isReady())
         return;
 
     qCDebug(lcQuickLayouts) << "QQuickLayout::geometryChange" << newGeometry << oldGeometry;
diff --git a/src/quicklayouts/qquicklinearlayout.cpp b/src/quicklayouts/qquicklinearlayout.cpp
index de243af8f3..25c5e62632 100644
--- a/src/quicklayouts/qquicklinearlayout.cpp
+++ b/src/quicklayouts/qquicklinearlayout.cpp
@@ -361,26 +361,24 @@ void QQuickGridLayoutBase::invalidate(QQuickItem *childItem)
     if (!isReady())
         return;
     qCDebug(lcQuickLayouts) << "QQuickGridLayoutBase::invalidate()" << this << ", invalidated:" << invalidated();
-    if (invalidated()) {
-        return;
-    }
-    qCDebug(lcQuickLayouts) << "d->m_rearranging:" << d->m_rearranging;
-    if (d->m_rearranging) {
-        d->m_invalidateAfterRearrange << childItem;
-        return;
-    }
-
     if (childItem) {
-        if (QQuickGridLayoutItem *layoutItem = d->engine.findLayoutItem(childItem))
+        if (d->m_rearranging) {
+            if (!d->m_invalidateAfterRearrange.contains(childItem))
+                d->m_invalidateAfterRearrange << childItem;
+            return;
+        }
+        if (QQuickGridLayoutItem *layoutItem = d->engine.findLayoutItem(childItem)) {
             layoutItem->invalidate();
+        }
     }
+
     // invalidate engine
     d->engine.invalidate();
 
     qCDebug(lcQuickLayouts) << "calling QQuickLayout::invalidate();";
     QQuickLayout::invalidate();
 
-    if (QQuickLayout *parentLayout = qobject_cast<QQuickLayout *>(parentItem()))
+    if (auto *parentLayout = qobject_cast<QQuickLayout *>(parentItem()))
         parentLayout->invalidate(this);
     qCDebug(lcQuickLayouts) << "QQuickGridLayoutBase::invalidate() LEAVING" << this;
 }
@@ -479,8 +477,8 @@ void QQuickGridLayoutBase::rearrange(const QSizeF &size)
     d->engine.setGeometries(QRectF(QPointF(0,0), size), d->styleInfo);
     d->m_rearranging = false;
 
-    for (QQuickItem *invalid : std::as_const(d->m_invalidateAfterRearrange))
-        invalidate(invalid);
+    for (auto childItem : std::as_const(d->m_invalidateAfterRearrange))
+        invalidate(childItem);
     d->m_invalidateAfterRearrange.clear();
 }
 
diff --git a/tests/auto/quick/qquicklayouts/data/tst_rowlayout.qml b/tests/auto/quick/qquicklayouts/data/tst_rowlayout.qml
index 99da499e08..63734d9d14 100644
--- a/tests/auto/quick/qquicklayouts/data/tst_rowlayout.qml
+++ b/tests/auto/quick/qquicklayouts/data/tst_rowlayout.qml
@@ -1329,6 +1329,78 @@ Item {
         }
 
         Component {
+            id: sizeHintBindingLoopComp
+            Item {
+                id: root
+                anchors.fill: parent
+                property var customWidth: 100
+                RowLayout {
+                    id: col
+                    Item {
+                        id: item
+                        implicitHeight: 80
+                        implicitWidth: Math.max(col2.implicitWidth, root.customWidth + 20)
+                        ColumnLayout {
+                            id: col2
+                            width: parent.width
+                            Item {
+                                id: rect
+                                implicitWidth: root.customWidth
+                                implicitHeight: 80
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        function test_sizeHintBindingLoopIssue() {
+            var item = createTemporaryObject(sizeHintBindingLoopComp, container)
+            waitForRendering(item)
+            item.customWidth += 10
+            waitForRendering(item)
+            verify(!LayoutSetup.bindingLoopDetected, "Detected binding loop")
+            LayoutSetup.resetBindingLoopDetectedFlag()
+        }
+
+        Component {
+            id: polishLayoutItemComp
+            Item {
+                anchors.fill: parent
+                implicitHeight: contentLayout.implicitHeight
+                implicitWidth: contentLayout.implicitWidth
+                property alias textLayout: contentLayout
+                RowLayout {
+                    width: parent.width
+                    height: parent.height
+                    ColumnLayout {
+                        id: contentLayout
+                        Layout.alignment: Qt.AlignHCenter | Qt.AlignTop
+                        Layout.maximumWidth: 200
+                        Repeater {
+                            model: 2
+                            Text {
+                                Layout.fillWidth: true
+                                text: "This is a long text causing line breaks to show the bug."
+                                wrapMode: Text.Wrap
+                            }
+                        }
+                        Item {
+                            Layout.fillHeight: true
+                        }
+                    }
+                }
+            }
+        }
+
+        function test_polishLayoutItemIssue() {
+            var rootItem = createTemporaryObject(polishLayoutItemComp, container)
+            waitForRendering(rootItem)
+            var textItem = rootItem.textLayout.children[1]
+            verify(textItem.y >= rootItem.textLayout.children[0].height)
+        }
+
+        Component {
             id: rearrangeNestedLayouts_Component
             RowLayout {
                 id: layout