From 6001e655fdf90a5cabda20e3717c529fa5235585 Mon Sep 17 00:00:00 2001
From: Sergio Martins <sergio.martins@kdab.com>
Date: Tue, 19 May 2020 14:46:30 +0100
Subject: [PATCH] Fix crash when moving separator

We weren't recurring up the tree to find the next separator
---
 src/private/multisplitter/Item.cpp            | 26 +++++++++++++++--
 src/private/multisplitter/Item_p.h            |  1 +
 .../multisplitter/tests/tst_multisplitter.cpp | 28 +++++++++++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/src/private/multisplitter/Item.cpp b/src/private/multisplitter/Item.cpp
index d9bb7f9e..b13e76ee 100644
--- a/src/private/multisplitter/Item.cpp
+++ b/src/private/multisplitter/Item.cpp
@@ -2020,8 +2020,11 @@ void ItemContainer::requestSeparatorMove(Separator *separator, int delta)
     const int max = maxPosForSeparator_global(separator);
 
     if (pos + delta < min || pos + delta > max) {
+        root()->dumpLayout();
         qWarning() << "Separator would have gone out of bounds"
-                   << separator << min << pos << max << delta;
+                   << "; separators=" << separator
+                   << "; min=" << min << "; pos=" << pos
+                   << "; max=" << max << "; delta=" << delta;
         return;
     }
 
@@ -2073,7 +2076,13 @@ void ItemContainer::requestSeparatorMove(Separator *separator, int delta)
             qWarning() << Q_FUNC_INFO << "Not enough space to move separator"
                        << this;
         } else {
-            Separator *nextSeparator = parentContainer()->neighbourSeparator(this, moveDirection, m_orientation);
+            Separator *nextSeparator = parentContainer()->neighbourSeparator_recursive(this, moveDirection, m_orientation);
+            if (!nextSeparator) {
+                // Doesn't happen
+                qWarning() << Q_FUNC_INFO << "nextSeparator is null, report a bug";
+                return;
+            }
+
             // nextSeparator might not belong to parentContainer(), due to different orientation
             const int remainingDelta = moveDirection == Side1 ? -remainingToTake : remainingToTake;
             nextSeparator->parentContainer()->requestSeparatorMove(nextSeparator, remainingDelta);
@@ -2965,6 +2974,19 @@ Separator *ItemContainer::neighbourSeparator(const Item *item, Side side, Qt::Or
     return m_separators[separatorIndex];
 }
 
+Separator *ItemContainer::neighbourSeparator_recursive(const Item *item, Side side,
+                                                       Qt::Orientation orientation) const
+{
+    Separator *separator = neighbourSeparator(item, side, orientation);
+    if (separator)
+        return separator;
+
+    if (!parentContainer())
+        return nullptr;
+
+    return parentContainer()->neighbourSeparator_recursive(this, side, orientation);
+}
+
 void ItemContainer::updateWidgets_recursive()
 {
     for (Item *item : m_children) {
diff --git a/src/private/multisplitter/Item_p.h b/src/private/multisplitter/Item_p.h
index fe978402..ee7f093d 100644
--- a/src/private/multisplitter/Item_p.h
+++ b/src/private/multisplitter/Item_p.h
@@ -548,6 +548,7 @@ private:
     void resizeChildren(QSize oldSize, QSize newSize, SizingInfo::List &sizes, ChildrenResizeStrategy);
     void scheduleCheckSanity() const;
     Separator *neighbourSeparator(const Item *item, Side, Qt::Orientation) const;
+    Separator *neighbourSeparator_recursive(const Item *item, Side, Qt::Orientation) const;
     void updateWidgets_recursive();
     /// Returns the positions that each separator should have (x position if Qt::Horizontal, y otherwise)
     QVector<int> requiredSeparatorPositions() const;
diff --git a/src/private/multisplitter/tests/tst_multisplitter.cpp b/src/private/multisplitter/tests/tst_multisplitter.cpp
index 0607cd1e..aa051e60 100644
--- a/src/private/multisplitter/tests/tst_multisplitter.cpp
+++ b/src/private/multisplitter/tests/tst_multisplitter.cpp
@@ -167,6 +167,7 @@ private Q_SLOTS:
     void tst_mapToRoot();
     void tst_closeAndRestorePreservesPosition();
     void tst_minSizeChangedBeforeRestore();
+    void tst_separatorMoveCrash();
 };
 
 class MyHostWidget : public QWidget {
@@ -1482,6 +1483,33 @@ void TestMultiSplitter::tst_minSizeChangedBeforeRestore()
     item2->restore(guest2);
 }
 
+void TestMultiSplitter::tst_separatorMoveCrash()
+{
+    // Tests a crash I got when moving separator to the right
+
+    auto root = createRoot();
+    auto item1 = createItem();
+    auto item2 = createItem();
+    auto item3 = createItem();
+    auto item4 = createItem();
+    auto item5 = createItem();
+    auto item6 = createItem();
+    root->insertItem(item1, Item::Location_OnTop);
+    root->insertItem(item2, Item::Location_OnBottom);
+    item2->insertItem(item3, Item::Location_OnRight);
+    item3->insertItem(item4, Item::Location_OnBottom);
+    item4->insertItem(item5, Item::Location_OnRight);
+    root->insertItem(item6, Item::Location_OnRight);
+
+    ItemContainer *c = item5->parentContainer();
+    auto separator = c->separators()[0];
+
+    const int available5 = item5->availableLength(Qt::Horizontal);
+
+    // Separator squeezes item5 and starts squeezing item6 by 10px
+    c->requestSeparatorMove(separator, available5 + 10);
+}
+
 int main(int argc, char *argv[])
 {
     bool qpaPassed = false;