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>

17 files changed, 63 insertions, 21 deletions

diff --git a/src/quicktemplates2/qquickabstractbutton.cpp b/src/quicktemplates2/qquickabstractbutton.cpp
index bb07d13e..eb592032 100644
--- a/src/quicktemplates2/qquickabstractbutton.cpp
+++ b/src/quicktemplates2/qquickabstractbutton.cpp
@@ -712,7 +712,7 @@ void QQuickAbstractButton::setIndicator(QQuickItem *indicator)
     const qreal oldImplicitIndicatorHeight = implicitIndicatorHeight();
 
     d->removeImplicitSizeListener(d->indicator);
-    delete d->indicator;
+    QQuickControlPrivate::hideOldItem(d->indicator);
     d->indicator = indicator;
 
     if (indicator) {
diff --git a/src/quicktemplates2/qquickapplicationwindow.cpp b/src/quicktemplates2/qquickapplicationwindow.cpp
index 19f6f82b..9739650e 100644
--- a/src/quicktemplates2/qquickapplicationwindow.cpp
+++ b/src/quicktemplates2/qquickapplicationwindow.cpp
@@ -407,7 +407,7 @@ void QQuickApplicationWindow::setBackground(QQuickItem *background)
     if (!d->background.isExecuting())
         d->cancelBackground();
 
-    delete d->background;
+    QQuickControlPrivate::hideOldItem(d->background);
     d->background = background;
     if (background) {
         background->setParentItem(QQuickWindow::contentItem());
diff --git a/src/quicktemplates2/qquickcombobox.cpp b/src/quicktemplates2/qquickcombobox.cpp
index 6b03bbf2..dcd427f3 100644
--- a/src/quicktemplates2/qquickcombobox.cpp
+++ b/src/quicktemplates2/qquickcombobox.cpp
@@ -51,6 +51,7 @@
 #include <QtQml/qqmlcontext.h>
 #include <QtQml/private/qlazilyallocated_p.h>
 #include <private/qqmldelegatemodel_p.h>
+#include <QtQuick/private/qquickaccessibleattached_p.h>
 #include <QtQuick/private/qquickevents_p_p.h>
 #include <QtQuick/private/qquicktextinput_p.h>
 #include <QtQuick/private/qquickitemview_p.h>
@@ -264,6 +265,8 @@ public:
     void itemImplicitWidthChanged(QQuickItem *item) override;
     void itemImplicitHeightChanged(QQuickItem *item) override;
 
+    static void hideOldPopup(QQuickPopup *popup);
+
     bool flat = false;
     bool down = false;
     bool hasDown = false;
@@ -774,6 +777,21 @@ void QQuickComboBoxPrivate::itemImplicitHeightChanged(QQuickItem *item)
         emit q->implicitIndicatorHeightChanged();
 }
 
+void QQuickComboBoxPrivate::hideOldPopup(QQuickPopup *popup)
+{
+    if (!popup)
+        return;
+
+    qCDebug(lcItemManagement) << "hiding old popup" << popup;
+
+    popup->setVisible(false);
+    popup->setParentItem(nullptr);
+    // Remove the item from the accessibility tree.
+    QQuickAccessibleAttached *accessible = accessibleAttached(popup);
+    if (accessible)
+        accessible->setIgnored(true);
+}
+
 QQuickComboBox::QQuickComboBox(QQuickItem *parent)
     : QQuickControl(*(new QQuickComboBoxPrivate), parent)
 {
@@ -794,7 +812,7 @@ QQuickComboBox::~QQuickComboBox()
         // Disconnect visibleChanged() to avoid a spurious highlightedIndexChanged() signal
         // emission during the destruction of the (visible) popup. (QTBUG-57650)
         QObjectPrivate::disconnect(d->popup.data(), &QQuickPopup::visibleChanged, d, &QQuickComboBoxPrivate::popupVisibleChanged);
-        delete d->popup;
+        QQuickComboBoxPrivate::hideOldPopup(d->popup);
         d->popup = nullptr;
     }
 }
@@ -1136,7 +1154,7 @@ void QQuickComboBox::setIndicator(QQuickItem *indicator)
     const qreal oldImplicitIndicatorHeight = implicitIndicatorHeight();
 
     d->removeImplicitSizeListener(d->indicator);
-    delete d->indicator;
+    QQuickControlPrivate::hideOldItem(d->indicator);
     d->indicator = indicator;
     if (indicator) {
         if (!indicator->parentItem())
@@ -1184,7 +1202,7 @@ void QQuickComboBox::setPopup(QQuickPopup *popup)
 
     if (d->popup) {
         QObjectPrivate::disconnect(d->popup.data(), &QQuickPopup::visibleChanged, d, &QQuickComboBoxPrivate::popupVisibleChanged);
-        delete d->popup;
+        QQuickComboBoxPrivate::hideOldPopup(d->popup);
     }
     if (popup) {
         QQuickPopupPrivate::get(popup)->allowVerticalFlip = true;
diff --git a/src/quicktemplates2/qquickcombobox_p.h b/src/quicktemplates2/qquickcombobox_p.h
index c9063d6a..00bc1242 100644
--- a/src/quicktemplates2/qquickcombobox_p.h
+++ b/src/quicktemplates2/qquickcombobox_p.h
@@ -48,10 +48,13 @@
 // We mean it.
 //
 
+#include <QtCore/qloggingcategory.h>
 #include <QtQuickTemplates2/private/qquickcontrol_p.h>
 
 QT_BEGIN_NAMESPACE
 
+Q_DECLARE_LOGGING_CATEGORY(lcItemManagement)
+
 class QValidator;
 class QQuickPopup;
 class QQmlInstanceModel;
diff --git a/src/quicktemplates2/qquickcontainer.cpp b/src/quicktemplates2/qquickcontainer.cpp
index 5f38c5b9..08129866 100644
--- a/src/quicktemplates2/qquickcontainer.cpp
+++ b/src/quicktemplates2/qquickcontainer.cpp
@@ -219,7 +219,7 @@ void QQuickContainerPrivate::cleanup()
             QQuickWindowPrivate::get(window)->clearFocusInScope(contentItem, focusItem, Qt::OtherFocusReason);
 
         q->contentItemChange(nullptr, contentItem);
-        delete contentItem;
+        QQuickControlPrivate::hideOldItem(contentItem);
     }
 
     QObject::disconnect(contentModel, &QQmlObjectModel::countChanged, q, &QQuickContainer::countChanged);
diff --git a/src/quicktemplates2/qquickcontrol.cpp b/src/quicktemplates2/qquickcontrol.cpp
index 0ebaf9a3..3306ff09 100644
--- a/src/quicktemplates2/qquickcontrol.cpp
+++ b/src/quicktemplates2/qquickcontrol.cpp
@@ -56,6 +56,8 @@
 
 QT_BEGIN_NAMESPACE
 
+Q_LOGGING_CATEGORY(lcItemManagement, "qt.quick.controls.control.itemmanagement")
+
 /*!
     \qmltype Control
     \inherits Item
@@ -420,7 +422,7 @@ void QQuickControlPrivate::setContentItem_helper(QQuickItem *item, bool notify)
 
     contentItem = item;
     q->contentItemChange(item, oldContentItem);
-    delete oldContentItem;
+    QQuickControlPrivate::hideOldItem(oldContentItem);
 
     if (item) {
         connect(contentItem.data(), &QQuickItem::baselineOffsetChanged, this, &QQuickControlPrivate::updateBaselineOffset);
@@ -838,6 +840,22 @@ void QQuickControlPrivate::executeBackground(bool complete)
         quickCompleteDeferred(q, backgroundName(), background);
 }
 
+void QQuickControlPrivate::hideOldItem(QQuickItem *item)
+{
+    if (!item)
+        return;
+
+    qCDebug(lcItemManagement) << "hiding old item" << item;
+
+    item->setVisible(false);
+    item->setParentItem(nullptr);
+
+    // Remove the item from the accessibility tree.
+    QQuickAccessibleAttached *accessible = accessibleAttached(item);
+    if (accessible)
+        accessible->setIgnored(true);
+}
+
 void QQuickControlPrivate::updateBaselineOffset()
 {
     Q_Q(QQuickControl);
@@ -1590,7 +1608,7 @@ void QQuickControl::setBackground(QQuickItem *background)
     }
 
     d->removeImplicitSizeListener(d->background, QQuickControlPrivate::ImplicitSizeChanges | QQuickItemPrivate::Geometry);
-    delete d->background;
+    QQuickControlPrivate::hideOldItem(d->background);
     d->background = background;
 
     if (background) {
diff --git a/src/quicktemplates2/qquickcontrol_p_p.h b/src/quicktemplates2/qquickcontrol_p_p.h
index a657307b..b649db0a 100644
--- a/src/quicktemplates2/qquickcontrol_p_p.h
+++ b/src/quicktemplates2/qquickcontrol_p_p.h
@@ -60,8 +60,12 @@
 #include <QtGui/qaccessible.h>
 #endif
 
+#include <QtCore/qloggingcategory.h>
+
 QT_BEGIN_NAMESPACE
 
+Q_DECLARE_LOGGING_CATEGORY(lcItemManagement)
+
 class QQuickAccessibleAttached;
 
 class Q_QUICKTEMPLATES2_PRIVATE_EXPORT QQuickControlPrivate : public QQuickItemPrivate, public QQuickItemChangeListener
@@ -168,6 +172,8 @@ public:
     virtual void cancelBackground();
     virtual void executeBackground(bool complete = false);
 
+    static void hideOldItem(QQuickItem *item);
+
     void updateBaselineOffset();
 
     static const ChangeTypes ImplicitSizeChanges;
diff --git a/src/quicktemplates2/qquickdial.cpp b/src/quicktemplates2/qquickdial.cpp
index 431b25a8..65fed91c 100644
--- a/src/quicktemplates2/qquickdial.cpp
+++ b/src/quicktemplates2/qquickdial.cpp
@@ -620,7 +620,7 @@ void QQuickDial::setHandle(QQuickItem *handle)
     if (!d->handle.isExecuting())
         d->cancelHandle();
 
-    delete d->handle;
+    QQuickControlPrivate::hideOldItem(d->handle);
     d->handle = handle;
     if (d->handle && !d->handle->parentItem())
         d->handle->setParentItem(this);
diff --git a/src/quicktemplates2/qquickgroupbox.cpp b/src/quicktemplates2/qquickgroupbox.cpp
index 7f3c7a2d..8f6f8bc4 100644
--- a/src/quicktemplates2/qquickgroupbox.cpp
+++ b/src/quicktemplates2/qquickgroupbox.cpp
@@ -199,7 +199,7 @@ void QQuickGroupBox::setLabel(QQuickItem *label)
     const qreal oldImplicitLabelHeight = implicitLabelHeight();
 
     d->removeImplicitSizeListener(d->label);
-    delete d->label;
+    QQuickControlPrivate::hideOldItem(d->label);
     d->label = label;
 
     if (label) {
diff --git a/src/quicktemplates2/qquicklabel.cpp b/src/quicktemplates2/qquicklabel.cpp
index f3e0d512..da9b6985 100644
--- a/src/quicktemplates2/qquicklabel.cpp
+++ b/src/quicktemplates2/qquicklabel.cpp
@@ -396,7 +396,7 @@ void QQuickLabel::setBackground(QQuickItem *background)
     }
 
     QQuickControlPrivate::removeImplicitSizeListener(d->background, d, QQuickControlPrivate::ImplicitSizeChanges | QQuickItemPrivate::Geometry);
-    delete d->background;
+    QQuickControlPrivate::hideOldItem(d->background);
     d->background = background;
 
     if (background) {
diff --git a/src/quicktemplates2/qquickmenuitem.cpp b/src/quicktemplates2/qquickmenuitem.cpp
index a7fc63e8..118db822 100644
--- a/src/quicktemplates2/qquickmenuitem.cpp
+++ b/src/quicktemplates2/qquickmenuitem.cpp
@@ -216,7 +216,7 @@ void QQuickMenuItem::setArrow(QQuickItem *arrow)
     if (!d->arrow.isExecuting())
         d->cancelArrow();
 
-    delete d->arrow;
+    QQuickControlPrivate::hideOldItem(d->arrow);
     d->arrow = arrow;
     if (arrow && !arrow->parentItem())
         arrow->setParentItem(this);
diff --git a/src/quicktemplates2/qquickoverlay.cpp b/src/quicktemplates2/qquickoverlay.cpp
index 73a31c84..bf2fc192 100644
--- a/src/quicktemplates2/qquickoverlay.cpp
+++ b/src/quicktemplates2/qquickoverlay.cpp
@@ -34,6 +34,7 @@
 **
 ****************************************************************************/
 
+#include "qquickcontrol_p_p.h"
 #include "qquickoverlay_p.h"
 #include "qquickoverlay_p_p.h"
 #include "qquickpopupitem_p_p.h"
@@ -342,7 +343,6 @@ void QQuickOverlay::setModal(QQmlComponent *modal)
     if (d->modal == modal)
         return;
 
-    delete d->modal;
     d->modal = modal;
     emit modalChanged();
 }
@@ -359,7 +359,6 @@ void QQuickOverlay::setModeless(QQmlComponent *modeless)
     if (d->modeless == modeless)
         return;
 
-    delete d->modeless;
     d->modeless = modeless;
     emit modelessChanged();
 }
@@ -671,7 +670,6 @@ void QQuickOverlayAttached::setModal(QQmlComponent *modal)
     if (d->modal == modal)
         return;
 
-    delete d->modal;
     d->modal = modal;
     emit modalChanged();
 }
@@ -704,7 +702,6 @@ void QQuickOverlayAttached::setModeless(QQmlComponent *modeless)
     if (d->modeless == modeless)
         return;
 
-    delete d->modeless;
     d->modeless = modeless;
     emit modelessChanged();
 }
diff --git a/src/quicktemplates2/qquickrangeslider.cpp b/src/quicktemplates2/qquickrangeslider.cpp
index 378ece50..17948987 100644
--- a/src/quicktemplates2/qquickrangeslider.cpp
+++ b/src/quicktemplates2/qquickrangeslider.cpp
@@ -267,7 +267,7 @@ void QQuickRangeSliderNode::setHandle(QQuickItem *handle)
     const qreal oldImplicitHandleHeight = implicitHandleHeight();
 
     QQuickControlPrivate::get(d->slider)->removeImplicitSizeListener(d->handle);
-    delete d->handle;
+    QQuickControlPrivate::hideOldItem(d->handle);
     d->handle = handle;
 
     if (handle) {
diff --git a/src/quicktemplates2/qquickslider.cpp b/src/quicktemplates2/qquickslider.cpp
index 054ea502..c7f8a025 100644
--- a/src/quicktemplates2/qquickslider.cpp
+++ b/src/quicktemplates2/qquickslider.cpp
@@ -579,7 +579,7 @@ void QQuickSlider::setHandle(QQuickItem *handle)
     const qreal oldImplicitHandleHeight = implicitHandleHeight();
 
     d->removeImplicitSizeListener(d->handle);
-    delete d->handle;
+    QQuickControlPrivate::hideOldItem(d->handle);
     d->handle = handle;
 
     if (handle) {
diff --git a/src/quicktemplates2/qquickspinbox.cpp b/src/quicktemplates2/qquickspinbox.cpp
index 389e5c54..bc8ec809 100644
--- a/src/quicktemplates2/qquickspinbox.cpp
+++ b/src/quicktemplates2/qquickspinbox.cpp
@@ -1126,7 +1126,7 @@ void QQuickSpinButton::setIndicator(QQuickItem *indicator)
 
     QQuickSpinBox *spinBox = static_cast<QQuickSpinBox *>(parent());
     QQuickSpinBoxPrivate::get(spinBox)->removeImplicitSizeListener(d->indicator);
-    delete d->indicator;
+    QQuickControlPrivate::hideOldItem(d->indicator);
     d->indicator = indicator;
 
     if (indicator) {
diff --git a/src/quicktemplates2/qquicktextarea.cpp b/src/quicktemplates2/qquicktextarea.cpp
index ef59bd93..ef452fd0 100644
--- a/src/quicktemplates2/qquicktextarea.cpp
+++ b/src/quicktemplates2/qquicktextarea.cpp
@@ -633,7 +633,7 @@ void QQuickTextArea::setBackground(QQuickItem *background)
     }
 
     QQuickControlPrivate::removeImplicitSizeListener(d->background, d, QQuickControlPrivate::ImplicitSizeChanges | QQuickItemPrivate::Geometry);
-    delete d->background;
+    QQuickControlPrivate::hideOldItem(d->background);
     d->background = background;
 
     if (background) {
diff --git a/src/quicktemplates2/qquicktextfield.cpp b/src/quicktemplates2/qquicktextfield.cpp
index 740edff0..c03b05da 100644
--- a/src/quicktemplates2/qquicktextfield.cpp
+++ b/src/quicktemplates2/qquicktextfield.cpp
@@ -501,7 +501,7 @@ void QQuickTextField::setBackground(QQuickItem *background)
     }
 
     QQuickControlPrivate::removeImplicitSizeListener(d->background, d, QQuickControlPrivate::ImplicitSizeChanges | QQuickItemPrivate::Geometry);
-    delete d->background;
+    QQuickControlPrivate::hideOldItem(d->background);
     d->background = background;
 
     if (background) {