SplitView: schedule another layout if requested sizes changed during layout

As mentioned in the review of ed87e837, there could be a scenario where the
user sets the preferred size of an item inside the onWidthChanged handler
of another item:

    onWidthChanged: if (width < 10) secondItem.SplitView.preferredWidth = 100

Before this patch, this would result in the preferredWidth assignment being
ignored since it happened during a layout.

This patch adds some auto tests to ensure that this works, as the previous
patch (that converted layouts to be done in polish/updatePolish cycles)
already fixed the issue.

It also adds a check to avoid doing too many layouts in the case of
one of the split handles being dragged.

Change-Id: Ide519b33a2fa3bf746ae3793e0671fd1750c70d8
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

3 files changed, 119 insertions, 5 deletions

diff --git a/src/quicktemplates2/qquicksplitview.cpp b/src/quicktemplates2/qquicksplitview.cpp
index 64ccb6c7..502d2b3a 100644
--- a/src/quicktemplates2/qquicksplitview.cpp
+++ b/src/quicktemplates2/qquicksplitview.cpp
@@ -383,6 +383,20 @@ void QQuickSplitViewPrivate::layoutResizeSplitItems(qreal &usedWidth, qreal &use
                     attached = qobject_cast<QQuickSplitViewAttached*>(
                         qmlAttachedPropertiesObject<QQuickSplitView>(item, true));
                 }
+
+                /*
+                    Users could conceivably respond to size changes in items by setting attached
+                    SplitView properties:
+
+                        onWidthChanged: if (width < 10) secondItem.SplitView.preferredWidth = 100
+
+                    We handle this by doing another layout after the current layout if the
+                    attached/implicit size properties are set during this layout. However, we also
+                    need to set preferredWidth/Height here (for reasons mentioned in the comment above),
+                    but we don't want this to count as a request for a delayed layout, so we guard against it.
+                */
+                m_ignoreNextLayoutRequest = true;
+
                 if (horizontal)
                     attached->setPreferredWidth(newItemSize);
                 else
@@ -430,14 +444,17 @@ void QQuickSplitViewPrivate::layoutResizeSplitItems(qreal &usedWidth, qreal &use
                     attached = qobject_cast<QQuickSplitViewAttached*>(
                         qmlAttachedPropertiesObject<QQuickSplitView>(item, true));
                 }
+
+                m_ignoreNextLayoutRequest = true;
+
                 if (horizontal)
                     attached->setPreferredWidth(newItemSize);
                 else
                     attached->setPreferredHeight(newItemSize);
 
-                // We still need to use requestedWidth in the setWidth() call below,
+                // We still need to use requestedSize in the setWidth()/setHeight() call below,
                 // because sizeData has already been calculated and now contains an old
-                // effectivePreferredWidth value.
+                // effectivePreferredWidth/Height value.
                 requestedSize = newItemSize;
 
                 qCDebug(qlcQQuickSplitView).nospace() << "  - " << index << ": resized (dragged) " << item
@@ -1618,11 +1635,23 @@ void QQuickSplitViewAttached::setPreferredWidth(qreal width)
 {
     Q_D(QQuickSplitViewAttached);
     d->m_isPreferredWidthSet = true;
+    // Make sure that we clear this flag now, before we emit the change signals
+    // which could cause another setter to be called.
+    auto splitViewPrivate = QQuickSplitViewPrivate::get(d->m_splitView);
+    const bool ignoreNextLayoutRequest = splitViewPrivate->m_ignoreNextLayoutRequest;
+    splitViewPrivate->m_ignoreNextLayoutRequest = false;
+
     if (qFuzzyCompare(width, d->m_preferredWidth))
         return;
 
     d->m_preferredWidth = width;
-    d->requestLayoutView();
+
+    if (!ignoreNextLayoutRequest) {
+        // We are currently in the middle of performing a layout, and the user (not our internal code)
+        // changed the preferred width of one of the split items, so request another layout.
+        d->requestLayoutView();
+    }
+
     emit preferredWidthChanged();
 }
 
@@ -1674,11 +1703,23 @@ void QQuickSplitViewAttached::setPreferredHeight(qreal height)
 {
     Q_D(QQuickSplitViewAttached);
     d->m_isPreferredHeightSet = true;
+    // Make sure that we clear this flag now, before we emit the change signals
+    // which could cause another setter to be called.
+    auto splitViewPrivate = QQuickSplitViewPrivate::get(d->m_splitView);
+    const bool ignoreNextLayoutRequest = splitViewPrivate->m_ignoreNextLayoutRequest;
+    splitViewPrivate->m_ignoreNextLayoutRequest = false;
+
     if (qFuzzyCompare(height, d->m_preferredHeight))
         return;
 
     d->m_preferredHeight = height;
-    d->requestLayoutView();
+
+    if (!ignoreNextLayoutRequest) {
+        // We are currently in the middle of performing a layout, and the user (not our internal code)
+        // changed the preferred height of one of the split items, so request another layout.
+        d->requestLayoutView();
+    }
+
     emit preferredHeightChanged();
 }
 
diff --git a/src/quicktemplates2/qquicksplitview_p_p.h b/src/quicktemplates2/qquicksplitview_p_p.h
index 560276f0..5d71d461 100644
--- a/src/quicktemplates2/qquicksplitview_p_p.h
+++ b/src/quicktemplates2/qquicksplitview_p_p.h
@@ -106,7 +106,6 @@ public:
 
     static QQuickSplitViewPrivate *get(QQuickSplitView *splitView);
 
-private:
     Qt::Orientation m_orientation = Qt::Horizontal;
     QQmlComponent *m_handle = nullptr;
     QVector<QQuickItem*> m_handleItems;
@@ -119,6 +118,7 @@ private:
     qreal m_rightOrBottomItemSizeBeforePress = 0.0;
     int m_fillIndex = -1;
     bool m_layingOut = false;
+    bool m_ignoreNextLayoutRequest = false;
     bool m_resizing = false;
 };
 
diff --git a/tests/auto/controls/data/tst_splitview.qml b/tests/auto/controls/data/tst_splitview.qml
index 40e7dda0..28c00179 100644
--- a/tests/auto/controls/data/tst_splitview.qml
+++ b/tests/auto/controls/data/tst_splitview.qml
@@ -1847,4 +1847,77 @@ TestCase {
         control.restoreState(settings.value("splitView"))
         compare(lastItem.SplitView.preferredWidth, 123)
     }
+
+    function test_changePreferredSizeDuringLayout() {
+        var control = createTemporaryObject(threeSizedItemsComponent, testCase)
+        verify(control)
+
+        var firstItem = control.itemAt(0)
+        var secondItem = control.itemAt(1)
+        secondItem.widthChanged.connect(function() {
+            if (secondItem.width < 10)
+                firstItem.SplitView.preferredWidth = 50
+        })
+
+        // Change the size of the item so that a layout happens, but
+        // make the size small enough that the item's onWidthChanged handler gets triggered.
+        // The onWidthChanged handler will set the preferredWidth of another item during the
+        // layout, so we need to make sure the assignment isn't lost since we return early in that case.
+        secondItem.implicitWidth = 5
+        verify(isPolishScheduled(control))
+        verify(waitForItemPolished(control))
+        compare(secondItem.width, 5)
+        compare(firstItem.width, 50)
+
+        // Now do the same for height.
+        control.orientation = Qt.Vertical
+        secondItem.heightChanged.connect(function() {
+            if (secondItem.height < 10)
+                firstItem.SplitView.preferredHeight = 50
+        })
+        // Get the polishes for the orientation out of the way so that they
+        // don't intefere with our results.
+        verify(isPolishScheduled(control))
+        verify(waitForItemPolished(control))
+
+        secondItem.implicitHeight = 5
+        verify(isPolishScheduled(control))
+        verify(waitForItemPolished(control))
+        compare(secondItem.height, 5)
+        compare(firstItem.height, 50)
+    }
+
+    // When the user drags a handle, we internally set preferredWidth/Height
+    // to reflect the new value. However, we also have to make sure that when
+    // we do so, it doesn't trigger a delayed layout. This is why we have
+    // m_ignoreNextDelayedLayoutRequest. This test checks that
+    // m_ignoreNextDelayedLayoutRequest doesn't interfere with any action from
+    // the user that results in a delayed layout.
+    function test_changePreferredSizeDuringLayoutWhileDraggingHandle() {
+        var control = createTemporaryObject(threeSizedItemsComponent, testCase)
+        verify(control)
+
+        var firstItem = control.itemAt(0)
+        var secondItem = control.itemAt(1)
+        firstItem.widthChanged.connect(function() {
+            if (firstItem.width === 0)
+                secondItem.SplitView.preferredWidth = 50
+        })
+
+        // Start dragging the handle.
+        var handles = findHandles(control)
+        var targetHandle = handles[0]
+        mousePress(targetHandle)
+        verify(control.resizing)
+        var localPos = testCase.mapToItem(targetHandle, 15, testCase.height / 2)
+
+        // Move the handle to the very left, so that the item's width becomes zero.
+        mouseMove(targetHandle, -100, targetHandle.height / 2)
+        verify(control.resizing)
+        compare(firstItem.width, 0)
+        compare(secondItem.SplitView.preferredWidth, 50)
+        compare(secondItem.width, 50)
+        mouseRelease(targetHandle, -100, targetHandle.height / 2, Qt.LeftButton)
+        verify(!control.resizing)
+    }
 }