From 0790b899ca640e2612dad9fb95b5fc7de336d9b1 Mon Sep 17 00:00:00 2001 From: Santhosh Kumar Date: Tue, 16 Jan 2024 12:56:27 +0100 Subject: Fix binding loop and polish issue in quick layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.6 6.5 Change-Id: Ie5713f51ed9e428786046bb06a822e5668beebb0 Reviewed-by: Lars Schmertmann Reviewed-by: Jan Arve Sæther (cherry picked from commit 3fa4719789921ff8be440ec760d8b9ab569758eb) Reviewed-by: Qt Cherry-pick Bot --- src/quicklayouts/qquicklayout.cpp | 24 +++----- src/quicklayouts/qquicklinearlayout.cpp | 24 ++++---- .../quick/qquicklayouts/data/tst_rowlayout.qml | 72 ++++++++++++++++++++++ 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(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 31c1857d9f..e10725da1c 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(parentItem())) + if (auto *parentLayout = qobject_cast(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 @@ -1328,6 +1328,78 @@ Item { compare(row.implicitWidth, 2); } + 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 { -- cgit v1.2.3