SplitView: fix hidden items causing visible items to not be resizable

When a handle is dragged, the items on either side of it are resized.
Until this patch, we were assuming that the item after the
one at the handle index was visible, which was wrong. Now we iterate
through each item after the one at the pressed index until we find
one that's visible. Since we need this in a few other places during
a handle drag, we cache it as a member variable.

This patch also fixes an issue where the visibility of handles were
not updated after setting a new handle delegate.

Change-Id: Icd246abae2ed4dc6c3b81217b9a241b7e4debf7d
Fixes: QTBUG-81867
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

3 files changed, 108 insertions, 15 deletions

diff --git a/src/quicktemplates2/qquicksplitview.cpp b/src/quicktemplates2/qquicksplitview.cpp
index 219a6b08..a3566c56 100644
--- a/src/quicktemplates2/qquicksplitview.cpp
+++ b/src/quicktemplates2/qquicksplitview.cpp
@@ -327,7 +327,7 @@ void QQuickSplitViewPrivate::layoutResizeSplitItems(qreal &usedWidth, qreal &use
         const bool isAHandlePressed = m_pressedHandleIndex != -1;
         // True if this particular item is being resized as a result of a handle being dragged.
         const bool isBeingResized = isAHandlePressed && ((resizeLeftItem && index == m_pressedHandleIndex)
-            || (!resizeLeftItem && index == m_pressedHandleIndex + 1));
+            || (!resizeLeftItem && index == m_nextVisibleIndexAfterPressedHandle));
         if (isBeingResized) {
             indexBeingResizedDueToDrag = index;
             qCDebug(qlcQQuickSplitView).nospace() << "  - " << index << ": dragging handle for item";
@@ -343,7 +343,7 @@ void QQuickSplitViewPrivate::layoutResizeSplitItems(qreal &usedWidth, qreal &use
 
             // We also need to ensure that the item's edge doesn't go too far
             // out and hence give the item more space than is available.
-            const int firstIndex = resizeLeftItem ? m_pressedHandleIndex + 1 : 0;
+            const int firstIndex = resizeLeftItem ? m_nextVisibleIndexAfterPressedHandle : 0;
             const int lastIndex = resizeLeftItem ? contentModel->count() - 1 : m_pressedHandleIndex;
             const qreal accumulated = accumulatedSize(firstIndex, lastIndex);
 
@@ -421,8 +421,8 @@ void QQuickSplitViewPrivate::layoutResizeSplitItems(qreal &usedWidth, qreal &use
                 // The handle shouldn't cross other handles, so use the left edge of
                 // the first handle to the right as the right edge.
                 qreal rightEdge = size;
-                if (m_pressedHandleIndex + 1 < m_handleItems.size()) {
-                    const QQuickItem *rightHandle = m_handleItems.at(m_pressedHandleIndex + 1);
+                if (m_nextVisibleIndexAfterPressedHandle < m_handleItems.size()) {
+                    const QQuickItem *rightHandle = m_handleItems.at(m_nextVisibleIndexAfterPressedHandle);
                     rightEdge = horizontal ? rightHandle->x() : rightHandle->y();
                 }
 
@@ -740,23 +740,26 @@ void QQuickSplitViewPrivate::createHandleItem(int index)
         creationContext = qmlContext(q);
     QQmlContext *context = new QQmlContext(creationContext, q);
     context->setContextObject(q);
-    QQuickItem *item = qobject_cast<QQuickItem*>(m_handle->beginCreate(context));
-    if (item) {
+    QQuickItem *handleItem = qobject_cast<QQuickItem*>(m_handle->beginCreate(context));
+    if (handleItem) {
+        qCDebug(qlcQQuickSplitView) << "- successfully created handle item" << handleItem << "for split item at index" << index;
+
         // Insert the item to our list of items *before* its parent is set to us,
         // so that we can avoid it being added as a content item by checking
         // if it is in the list in isContent().
-        m_handleItems.insert(index, item);
+        m_handleItems.insert(index, handleItem);
 
-        item->setParentItem(q);
+        handleItem->setParentItem(q);
 
         m_handle->completeCreate();
-        resizeHandle(item);
+        resizeHandle(handleItem);
     }
 }
 
 void QQuickSplitViewPrivate::removeExcessHandles()
 {
     int excess = m_handleItems.size() - qMax(0, contentModel->count() - 1);
+    qCDebug(qlcQQuickSplitView) << "removing" << excess << "excess handles from the end of our list";
     for (; excess > 0; --excess) {
         QQuickItem *handleItem = m_handleItems.takeLast();
         delete handleItem;
@@ -861,6 +864,7 @@ int QQuickSplitViewPrivate::handleIndexForSplitIndex(int splitIndex) const
 
 void QQuickSplitViewPrivate::destroyHandles()
 {
+    qCDebug(qlcQQuickSplitView) << "destroying" << m_handleItems.size() << "handles";
     qDeleteAll(m_handleItems);
     m_handleItems.clear();
 }
@@ -907,7 +911,7 @@ void QQuickSplitViewPrivate::updateHandleVisibilities()
             handleItem->setVisible(item->isVisible());
         else
             handleItem->setVisible(false);
-        qCDebug(qlcQQuickSplitView) << "set visible property of handle at index"
+        qCDebug(qlcQQuickSplitView) << "set visible property of handle" << handleItem << "at index"
             << i << "to" << handleItem->isVisible();
     }
 }
@@ -915,6 +919,8 @@ void QQuickSplitViewPrivate::updateHandleVisibilities()
 void QQuickSplitViewPrivate::updateHoveredHandle(QQuickItem *hoveredItem)
 {
     Q_Q(QQuickSplitView);
+    qCDebug(qlcQQuickSplitViewMouse) << "updating hovered handle after" << hoveredItem << "was hovered";
+
     const int oldHoveredHandleIndex = m_hoveredHandleIndex;
     m_hoveredHandleIndex = m_handleItems.indexOf(hoveredItem);
     if (m_hoveredHandleIndex == oldHoveredHandleIndex)
@@ -983,7 +989,21 @@ void QQuickSplitViewPrivate::handlePress(const QPointF &point)
         m_mousePos = point;
 
         const QQuickItem *leftOrTopItem = qobject_cast<QQuickItem*>(contentModel->object(m_pressedHandleIndex));
-        const QQuickItem *rightOrBottomItem = qobject_cast<QQuickItem*>(contentModel->object(m_pressedHandleIndex + 1));
+        // Find the first item to the right/bottom of this one that is visible.
+        QQuickItem *rightOrBottomItem = nullptr;
+        m_nextVisibleIndexAfterPressedHandle = -1;
+        for (int i = m_pressedHandleIndex + 1; i < contentModel->count(); ++i) {
+            auto nextItem = qobject_cast<QQuickItem*>(contentModel->object(i));
+            if (nextItem->isVisible()) {
+                rightOrBottomItem = nextItem;
+                m_nextVisibleIndexAfterPressedHandle = i;
+                break;
+            }
+        }
+        Q_ASSERT_X(rightOrBottomItem, Q_FUNC_INFO, qPrintable(QString::fromLatin1(
+            "Failed to find a visible item to the right/bottom of the one that was pressed at index %1; this shouldn't happen")
+                .arg(m_pressedHandleIndex)));
+
         const bool isHorizontal = m_orientation == Qt::Horizontal;
         m_leftOrTopItemSizeBeforePress = isHorizontal ? leftOrTopItem->width() : leftOrTopItem->height();
         m_rightOrBottomItemSizeBeforePress = isHorizontal ? rightOrBottomItem->width() : rightOrBottomItem->height();
@@ -1002,8 +1022,10 @@ void QQuickSplitViewPrivate::handlePress(const QPointF &point)
         qCDebug(qlcQQuickSplitViewMouse).nospace() << "handled press -"
             << " left/top index=" << m_pressedHandleIndex << ","
             << " size before press=" << m_leftOrTopItemSizeBeforePress << ","
-            << " right/bottom index=" << m_pressedHandleIndex + 1 << ","
-            << " size before press=" << m_rightOrBottomItemSizeBeforePress;
+            << " item=" << leftOrTopItem
+            << " right/bottom index=" << m_nextVisibleIndexAfterPressedHandle << ","
+            << " size before press=" << m_rightOrBottomItemSizeBeforePress
+            << " item=" << rightOrBottomItem;
     }
 }
 
@@ -1205,8 +1227,10 @@ void QQuickSplitView::setHandle(QQmlComponent *handle)
 
     d->m_handle = handle;
 
-    if (d->m_handle)
+    if (d->m_handle) {
         d->createHandles();
+        d->updateHandleVisibilities();
+    }
 
     d->requestLayout();
 
diff --git a/src/quicktemplates2/qquicksplitview_p_p.h b/src/quicktemplates2/qquicksplitview_p_p.h
index ccefe5ec..2430eac1 100644
--- a/src/quicktemplates2/qquicksplitview_p_p.h
+++ b/src/quicktemplates2/qquicksplitview_p_p.h
@@ -112,6 +112,7 @@ public:
     QVector<QQuickItem*> m_handleItems;
     int m_hoveredHandleIndex = -1;
     int m_pressedHandleIndex = -1;
+    int m_nextVisibleIndexAfterPressedHandle = -1;
     QPointF m_pressPos;
     QPointF m_mousePos;
     QPointF m_handlePosBeforePress;
diff --git a/tests/auto/controls/data/tst_splitview.qml b/tests/auto/controls/data/tst_splitview.qml
index 74e4c68e..ae8179b2 100644
--- a/tests/auto/controls/data/tst_splitview.qml
+++ b/tests/auto/controls/data/tst_splitview.qml
@@ -148,6 +148,7 @@ TestCase {
             color: "#444"
 
             Text {
+                objectName: "handleText_" + text
                 text: parent.x + "," + parent.y + " " + parent.width + "x" + parent.height
                 color: "white"
                 anchors.centerIn: parent
@@ -871,6 +872,42 @@ TestCase {
         }
     }
 
+    Component {
+        id: hiddenItemSplitViewComponent
+
+        SplitView {
+            anchors.fill: parent
+            handle: handleComponent
+
+            Rectangle {
+                objectName: "steelblue"
+                color: objectName
+
+                SplitView.minimumWidth: 50
+            }
+            Rectangle {
+                objectName: "tomato"
+                color: objectName
+
+                SplitView.fillWidth: true
+                SplitView.preferredWidth: 200
+            }
+            Rectangle {
+                objectName: "navajowhite"
+                color: objectName
+                visible: false
+
+                SplitView.minimumWidth: visible ? 100 : 0
+            }
+            Rectangle {
+                objectName: "mediumseagreen"
+                color: objectName
+
+                SplitView.minimumWidth: 50
+            }
+        }
+    }
+
     function test_dragHandle_data() {
         var splitViewWidth = testCase.width - splitViewMargins * 2
         var splitViewHeight = testCase.height - splitViewMargins * 2
@@ -1151,6 +1188,36 @@ TestCase {
                     { x: 140, y: 0, width: defaultHorizontalHandleWidth, height: splitViewHeight },
                     { x: 150, y: 0, width: 150, height: splitViewHeight }
                 ]
+            },
+            {
+                tag: "hiddenItemSplitViewComponent",
+                // [50] | [200 (fill)] | [hidden] | [50]
+                component: hiddenItemSplitViewComponent,
+                orientation: Qt.Horizontal,
+                fillIndex: 1,
+                handleIndex: 1,
+                // Drag to the horizontal centre of the SplitView.
+                newHandlePos: Qt.point(splitViewMargins + 150, testCase.height / 2),
+                expectedGeometriesBeforeDrag: [
+                    { x: 0, y: 0, width: 50, height: splitViewHeight },
+                    { x: 50, y: 0, width: defaultHorizontalHandleWidth, height: splitViewHeight },
+                    { x: 50 + defaultHorizontalHandleWidth, y: 0, width: 200 - defaultHorizontalHandleWidth * 2, height: splitViewHeight },
+                    { x: 250 - (defaultHorizontalHandleWidth * 2) + defaultHorizontalHandleWidth, y: 0, width: defaultHorizontalHandleWidth, height: splitViewHeight },
+                    { hidden: true }, // Third item should be hidden.
+                    { hidden: true }, // Handle for third item should be hidden.
+                    { x: 250 - (defaultHorizontalHandleWidth * 2) + defaultHorizontalHandleWidth * 2, y: 0, width: 50, height: splitViewHeight }
+                ],
+                expectedGeometriesAfterDrag: [
+                    { x: 0, y: 0, width: 50, height: splitViewHeight },
+                    { x: 50, y: 0, width: defaultHorizontalHandleWidth, height: splitViewHeight },
+                    // Width of the fill item should end up smaller.
+                    { x: 50 + defaultHorizontalHandleWidth, y: 0, width: 100 - defaultHorizontalHandleWidth * 2, height: splitViewHeight },
+                    { x: 150 - (defaultHorizontalHandleWidth * 2) + defaultHorizontalHandleWidth, y: 0, width: defaultHorizontalHandleWidth, height: splitViewHeight },
+                    { hidden: true }, // Third item should be hidden.
+                    { hidden: true }, // Handle for third item should be hidden.
+                    // Width of the last item should grow.
+                    { x: 150 - (defaultHorizontalHandleWidth * 2) + defaultHorizontalHandleWidth * 2, y: 0, width: 150, height: splitViewHeight }
+                ]
             }
         ]
         return data
@@ -1171,7 +1238,7 @@ TestCase {
         else
             fillItem.SplitView.fillHeight = true
 
-        // Check the sizes of the items before the drag.
+        // Check the sizes (and visibility) of the items before the drag.
         verify(isPolishScheduled(control))
         verify(waitForItemPolished(control))
         compareSizes(control, data.expectedGeometriesBeforeDrag, "before drag")
@@ -1179,6 +1246,7 @@ TestCase {
         // Drag the handle.
         var handles = findHandles(control)
         var targetHandle = handles[data.handleIndex]
+        verify(targetHandle.visible)
         mousePress(targetHandle)
         verify(control.resizing)
         // newHandlePos is in scene coordinates, so map it to coordinates local to the handle.