Don't delete items we didn't create

Up until this patch, we've always deleted "old" items when a new one is
assigned. For example, the style's implementation of contentItem will
be destroyed here as it is not accessible by the user and is no longer
used:

    Button {
        contentItem: Item { /* ... */ }
    }

This was especially important before the introduction of deferred
execution, as the "default" items would always be created, regardless
of whether the user had overridden it with one of their own items.
By deleting the old items, we free unused resources that would
otherwise persist until application shutdown (calling gc() does not
result in the items being garbage-collected, from my testing).

Although this has largely worked without issues, deleting objects
that weren't created by us in C++ is not supported. User-assigned items
can be created in QML (with JavaScriptOwnership) or C++ (with
CppOwnership), and it is up to the user and/or the QML engine to
manage the lifetime of these items.

After the introduction of deferred execution, it became possible to
skip creation of the default items altogether, meaning that there was
nothing to delete when assigning a new, user-specified item. This
requires that no ids are used in these items, as doing so prevents
deferred execution. Assuming that users avoid using ids in their items,
there should be no unused items that live unnecessarily until
application shutdown. The remaining cases where items do not get
destroyed when they should result from the following:

- Imperative assignments (e.g. assigning an item to a Button's
  contentItem in Component.onCompleted). We already encourage
  declarative bindings rather than imperative assignments.
- Using ids in items.

Given that these are use cases that we will advise against in the
documentation, it's an acceptable compromise.

[ChangeLog][Important Behavior Changes] Old delegate items (background,
contentItem, etc.) are no longer destroyed, as they are technically
owned by user code. Instead, they are hidden, unparented from the
control (QQuickItem parent, not QObject), and Accessible.ignored is
set to true. This prevents them from being unintentionally visible and
interfering with the accessibility tree when a new delegate item is
set.

Change-Id: I56c39a73dfee989dbe8f8b8bb33aaa187750fdb7
Task-number: QTBUG-72085
Fixes: QTBUG-70144
Fixes: QTBUG-75605
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

3 files changed, 50 insertions, 5 deletions

diff --git a/tests/auto/controls/data/tst_spinbox.qml b/tests/auto/controls/data/tst_spinbox.qml
index d3a0d8bb..22dbb352 100644
--- a/tests/auto/controls/data/tst_spinbox.qml
+++ b/tests/auto/controls/data/tst_spinbox.qml
@@ -658,4 +658,20 @@ TestCase {
             compare(control.displayText, data.displayTexts[i])
         }
     }
+
+    Component {
+        id: overriddenSpinBox
+        SpinBox {
+            value: 50
+            up.indicator: Rectangle {
+                property string s: "this is the one"
+            }
+        }
+    }
+
+    function test_indicatorOverridden() {
+        var control = createTemporaryObject(overriddenSpinBox, testCase)
+        verify(control)
+        compare(control.up.indicator.s, "this is the one");
+    }
 }
diff --git a/tests/auto/customization/data/styles/identified/AbstractButton.qml b/tests/auto/customization/data/styles/identified/AbstractButton.qml
index b9656f7a..cd2b5bdc 100644
--- a/tests/auto/customization/data/styles/identified/AbstractButton.qml
+++ b/tests/auto/customization/data/styles/identified/AbstractButton.qml
@@ -58,15 +58,18 @@ T.AbstractButton {
     indicator: Item {
         id: indicator
         objectName: "abstractbutton-indicator-identified"
+        Accessible.name: objectName
     }
 
     contentItem: Item {
         id: contentItem
         objectName: "abstractbutton-contentItem-identified"
+        Accessible.name: objectName
     }
 
     background: Item {
         id: background
         objectName: "abstractbutton-background-identified"
+        Accessible.name: objectName
     }
 }
diff --git a/tests/auto/customization/tst_customization.cpp b/tests/auto/customization/tst_customization.cpp
index 41efc2a6..cce74b41 100644
--- a/tests/auto/customization/tst_customization.cpp
+++ b/tests/auto/customization/tst_customization.cpp
@@ -42,6 +42,7 @@
 #include <QtQuick/qquickitem.h>
 #include <QtQuick/qquickwindow.h>
 #include <QtQuickControls2/qquickstyle.h>
+#include <QtQuickTemplates2/private/qquickcontrol_p_p.h>
 #include "../shared/visualtestutil.h"
 
 using namespace QQuickVisualTestUtil;
@@ -439,11 +440,36 @@ void tst_customization::override()
     QVERIFY2(qt_createdQObjects()->isEmpty(), qPrintable("unexpectedly created: " + qt_createdQObjects->join(", ")));
 
     if (!nonDeferred.isEmpty()) {
-        for (QString delegate : qAsConst(delegates)) {
-            if (!delegate.contains("-"))
-                delegate.append("-" + nonDeferred);
-            delegate.prepend(type.toLower() + "-");
-            QVERIFY2(qt_destroyedQObjects()->removeOne(delegate), qPrintable(delegate + " was not destroyed as expected"));
+        // There were items for which deferred execution was not possible.
+        for (QString delegateName : qAsConst(delegates)) {
+            if (!delegateName.contains("-"))
+                delegateName.append("-" + nonDeferred);
+            delegateName.prepend(type.toLower() + "-");
+
+            const int delegateIndex = qt_destroyedQObjects()->indexOf(delegateName);
+            QVERIFY2(delegateIndex == -1, qPrintable(delegateName + " was unexpectedly destroyed"));
+
+            const auto controlChildren = control->children();
+            const auto childIt = std::find_if(controlChildren.constBegin(), controlChildren.constEnd(), [delegateName](const QObject *child) {
+                return child->objectName() == delegateName;
+            });
+            // We test other delegates (like the background) here, so make sure we don't end up with XPASSes by using the wrong delegate.
+            if (delegateName.contains(QLatin1String("handle"))) {
+                QEXPECT_FAIL("identified:RangeSlider", "For some reason, items that are belong to grouped properties fail here", Abort);
+                QEXPECT_FAIL("overidentified:RangeSlider", "For some reason, items that are belong to grouped properties fail here", Abort);
+            }
+            if (delegateName.contains(QLatin1String("indicator"))) {
+                QEXPECT_FAIL("identified:SpinBox", "For some reason, items that are belong to grouped properties fail here", Abort);
+                QEXPECT_FAIL("overidentified:SpinBox", "For some reason, items that are belong to grouped properties fail here", Abort);
+            }
+            QVERIFY2(childIt != controlChildren.constEnd(), qPrintable(QString::fromLatin1(
+                "Expected delegate \"%1\" to still be a QObject child of \"%2\"").arg(delegateName).arg(controlName)));
+
+            const auto *delegate = qobject_cast<QQuickItem*>(*childIt);
+            // Ensure that the item is hidden, etc.
+            QVERIFY(delegate);
+            QCOMPARE(delegate->isVisible(), false);
+            QCOMPARE(delegate->parentItem(), nullptr);
         }
     }