Widgets focus abstraction: Fix re-parenting

Focus abstraction in 58d5d4b7c2faaeaa2c2ccdabb3da6d37f9db880a was
supposed to be behavior-neutral. QWidgetPrivate::reparentFocusChildren
used QObject::findChildren() to find children inside and outside the
current focus chain. If the re-parented widget had only direct children
and the focus chain was equal to creation-order, the result was
identical to previous behavior.

When the re-parented widget had grandchildren, the behavior differred.
While not being detected by existing tests, it caused a regression.

Re-implement the previous logic: Iterate through the focus chain,
instead of relying on QObject::findChildren() returning a complete and
well-ordered list.

Modify tst_QWidget::focusChainOnReparent() in order to cover the
regression.

This amends 58d5d4b7c2faaeaa2c2ccdabb3da6d37f9db880a.

Fixes: QTBUG-125257
Change-Id: Iff4f1d0d9b6117c50c8980dfb6eebfc6f6d4a710
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

src/widgets/kernel/qwidget.cpp

@@ -7093,7 +7093,6 @@ void QWidgetPrivate::reparentFocusWidgets(QWidget * oldtlw)
     if (focus_child)
         focus_child->clearFocus();
 
-    insertIntoFocusChain(QWidgetPrivate::FocusDirection::Previous, q->window());
     reparentFocusChildren(QWidgetPrivate::FocusDirection::Next);
 }
 
@@ -13369,32 +13368,61 @@ void QWidgetPrivate::initFocusChain()
 void QWidgetPrivate::reparentFocusChildren(FocusDirection direction)
 {
     Q_Q(QWidget);
-    QWidgetList focusChildrenInsideChain;
-    QDuplicateTracker<QWidget *> seen;
-    QWidget *widget = q->nextInFocusChain();
-    while (q->isAncestorOf(widget)
-           && !seen.hasSeen(widget)
-           && widget != q->window()) {
-        if (widget->focusPolicy() != Qt::NoFocus)
-            focusChildrenInsideChain << widget;
 
-        widget = direction == FocusDirection::Next ? widget->nextInFocusChain()
+    // separate the focus chain into new (children of myself) and old (the rest)
-                                                   : widget->previousInFocusChain();
+    QWidget *firstOld = nullptr;
+    QWidget *lastOld = nullptr; // last in the old list
+    QWidget *lastNew = q; // last in the new list
+    bool prevWasNew = true;
+    QWidget *widget = nextPrevElementInFocusChain(direction);
+
+    // For efficiency, do not maintain the list invariant inside the loop.
+    // Append items to the relevant list, and we optimize by not changing pointers,
+    // when subsequent items are going into the same list.
+    while (widget != q) {
+        bool currentIsNew = q->isAncestorOf(widget);
+        if (currentIsNew) {
+            if (!prevWasNew) {
+                // previous was old => append to new list
+                FOCUS_NEXT(lastNew) = widget;
+                FOCUS_PREV(widget) = lastNew;
+            }
+            lastNew = widget;
+        } else {
+            if (prevWasNew) {
+                // prev was new => append to old list, if it exists
+                if (lastOld) {
+                    FOCUS_NEXT(lastOld) = widget;
+                    FOCUS_PREV(widget) = lastOld;
+                } else {
+                    // start the old list
+                    firstOld = widget;
+                }
+            }
+            lastOld = widget;
+        }
+        widget = widget->d_func()->nextPrevElementInFocusChain(direction);
+        prevWasNew = currentIsNew;
     }
 
-    const QWidgetList children = q->findChildren<QWidget *>(Qt::FindDirectChildrenOnly);
+    // repair old list:
-    QWidgetList focusChildrenOutsideChain;
+    if (firstOld) {
-    for (auto *child : children) {
+        FOCUS_NEXT(lastOld) = firstOld;
-        if (!focusChildrenInsideChain.contains(child))
+        FOCUS_PREV(firstOld) = lastOld;
-            focusChildrenOutsideChain << child;
     }
-    if (focusChildrenOutsideChain.isEmpty())
-        return;
 
-    QWidget *previous = q;
+    if (!q->isWindow()) {
-    for (auto *child : focusChildrenOutsideChain) {
+        QWidget *topLevel = q->window();
-        child->d_func()->insertIntoFocusChain(direction, previous);
+        // insert new chain into toplevel's chain
-        previous = child;
+        QWidget *prev = FOCUS_PREV(topLevel);
+        FOCUS_PREV(topLevel) = lastNew;
+        FOCUS_NEXT(prev) = q;
+        FOCUS_PREV(q) = prev;
+        FOCUS_NEXT(lastNew) = topLevel;
+    } else {
+        // repair new list
+        FOCUS_NEXT(lastNew) = q;
+        FOCUS_PREV(q) = lastNew;
     }
 }
 

tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp

@@ -1902,6 +1902,7 @@ void tst_QWidget::focusChainOnReparent()
     }
 
     QWidget window2;
+    child22->setParent(child21);
     child2->setParent(&window2);
 
     QWidget *expectedNewChain[5] = {&window2, child2,  child21, child22, &window2};