Tumbler: fix regression with currentIndex and currentItem

2c4b2d48 made Tumbler's wrap property follow its count by default,
but did so using updatePolish() to account for the use case where a
items are appended to the model in a for loop, as is done in
TumblerDatePicker.qml in Tumbler's auto tests. This (appending items
one at a time in a for loop) is not a good idea, but it should work.
The problem with the solution is that the delay means that the use
cases mentioned in the referenced bug report were broken.

This patch removes the delay. The recursion guards are necessary due to
the complex nature of TumblerView (and its non-standard use of the Qt
Quick views), as they prevent a memory leak in
QQuickListView::createHighlight() from being introuduced.

We now call deleteLater() to ensure we do not interfere with the views'
internal operations, and hence we also perform a few extra operations
at the same time as insurance (although it appears that simply
unparenting the internal view from QQuickTumblerView is enough to get
the tests to pass).

Task-number: QTBUG-61374
Change-Id: Ifef9e99522ea183b282ac862f346beaed12d0c09
Reviewed-by: J-P Nurmi <jpnurmi@qt.io>

2 files changed, 71 insertions, 44 deletions

diff --git a/src/quickcontrols2/qquicktumblerview.cpp b/src/quickcontrols2/qquicktumblerview.cpp
index 817ec370..5e6d9f5a 100644
--- a/src/quickcontrols2/qquicktumblerview.cpp
+++ b/src/quickcontrols2/qquicktumblerview.cpp
@@ -36,7 +36,6 @@
 
 #include "qquicktumblerview_p.h"
 
-#include <QtQml/private/qqmldelegatemodel_p.h>
 #include <QtQuick/private/qquickitem_p.h>
 #include <QtQuick/private/qquicklistview_p.h>
 #include <QtQuick/private/qquickpathview_p.h>
@@ -126,7 +125,17 @@ void QQuickTumblerView::createView()
     // the count yet, because we rely on the view to tell us the count.
     if (m_tumbler->wrap()) {
         if (m_listView) {
-            delete m_listView;
+            // It's necessary to call deleteLater() rather than delete,
+            // as this code is most likely being run in rensponse to a signal
+            // emission somewhere in the list view's internals, so we need to
+            // wait until that has finished.
+            m_listView->deleteLater();
+            QQml_setParent_noEvent(m_listView, nullptr);
+            // The auto tests pass with unparenting the list view alone, but
+            // just to be sure, we unset some other things as well.
+            m_listView->setParentItem(nullptr);
+            m_listView->setVisible(false);
+            m_listView->setModel(QVariant());
             m_listView = nullptr;
         }
 
@@ -143,12 +152,16 @@ void QQuickTumblerView::createView()
 
             // Give the view a size.
             updateView();
-            // Ensure that the model is set eventually.
-            polish();
+            // Set the model.
+            updateModel();
         }
     } else {
         if (m_pathView) {
-            delete m_pathView;
+            m_pathView->deleteLater();
+            QQml_setParent_noEvent(m_pathView, nullptr);
+            m_pathView->setParentItem(nullptr);
+            m_pathView->setVisible(false);
+            m_pathView->setModel(QVariant());
             m_pathView = nullptr;
         }
 
@@ -164,8 +177,8 @@ void QQuickTumblerView::createView()
 
             // Give the view a size.
             updateView();
-            // Ensure that the model is set eventually.
-            polish();
+            // Set the model.
+            updateModel();
         }
     }
 }
@@ -193,6 +206,56 @@ void QQuickTumblerView::updateView()
     }
 }
 
+void QQuickTumblerView::updateModel()
+{
+    if (m_pathView && !m_pathView->model().isValid() && m_model.isValid()) {
+        // QQuickPathView::setPathItemCount() resets the offset animation,
+        // so we just skip the animation while constructing the view.
+        const int oldHighlightMoveDuration = m_pathView->highlightMoveDuration();
+        m_pathView->setHighlightMoveDuration(0);
+
+        // Setting model can change the count, which can affect the wrap, which can cause
+        // the current view to be deleted before setModel() is finished, which causes a crash.
+        // Since QQuickTumbler can't know about QQuickTumblerView, we use its private API to
+        // inform it that it should delay setting wrap.
+        QQuickTumblerPrivate *tumblerPrivate = QQuickTumblerPrivate::get(m_tumbler);
+        tumblerPrivate->lockWrap();
+        m_pathView->setModel(m_model);
+        tumblerPrivate->unlockWrap();
+
+        // The count-depends-on-wrap behavior could cause wrap to change after
+        // the call above, so we must check that we're still using a PathView.
+        if (m_pathView)
+            m_pathView->setHighlightMoveDuration(oldHighlightMoveDuration);
+    } else if (m_listView && !m_listView->model().isValid() && m_model.isValid()) {
+        const int currentIndex = m_tumbler->currentIndex();
+        QQuickTumblerPrivate *tumblerPrivate = QQuickTumblerPrivate::get(m_tumbler);
+
+        // setModel() causes QQuickTumblerPrivate::_q_onViewCountChanged() to
+        // be called called, which calls QQuickTumbler::setCurrentIndex(),
+        // which results in QQuickItemViewPrivate::createHighlightItem() being
+        // called. When the highlight item is created,
+        // QQuickTumblerPrivate::itemChildAdded() is notified and
+        // QQuickTumblerPrivate::_q_updateItemHeights() is called, which causes
+        // a geometry change in the item and createHighlight() is called again.
+        // However, since the highlight item hadn't been assigned yet in the
+        // previous call frame, the "if (highlight) { delete highlight; }"
+        // check doesn't succeed, so the item is never deleted.
+        //
+        // To avoid this, we tell QQuickTumblerPrivate to ignore signals while
+        // setting the model, and manually call _q_onViewCountChanged() to
+        // ensure the correct sequence of calls happens (_q_onViewCountChanged()
+        // has to be within the ignoreSignals scope, because it also generates
+        // recursion otherwise).
+        tumblerPrivate->ignoreSignals = true;
+        m_listView->setModel(m_model);
+        m_listView->setCurrentIndex(currentIndex);
+
+        tumblerPrivate->_q_onViewCountChanged();
+        tumblerPrivate->ignoreSignals = false;
+    }
+}
+
 void QQuickTumblerView::geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry)
 {
     QQuickItem::geometryChanged(newGeometry, oldGeometry);
@@ -223,42 +286,6 @@ void QQuickTumblerView::itemChange(QQuickItem::ItemChange change, const QQuickIt
     }
 }
 
-void QQuickTumblerView::updatePolish()
-{
-    // There are certain cases where model count changes can potentially cause problems.
-    // An example of this is a ListModel that appends items in a for loop in Component.onCompleted.
-    // If we didn't delay assignment of the model, the PathView/ListView would be deleted in
-    // response to it emitting countChanged(), causing a crash. To avoid this issue,
-    // and to avoid the overhead of count affecting the wrap property, which in turn may
-    // unnecessarily create delegates that are never seen, we delay setting the model. This ensures that
-    // Component.onCompleted would have been finished, for example.
-    if (m_pathView && !m_pathView->model().isValid() && m_model.isValid()) {
-        // QQuickPathView::setPathItemCount() resets the offset animation,
-        // so we just skip the animation while constructing the view.
-        const int oldHighlightMoveDuration = m_pathView->highlightMoveDuration();
-        m_pathView->setHighlightMoveDuration(0);
-
-        // Setting model can change the count, which can affect the wrap, which can cause
-        // the current view to be deleted before setModel() is finished, which causes a crash.
-        // Since QQuickTumbler can't know about QQuickTumblerView, we use its private API to
-        // inform it that it should delay setting wrap.
-        QQuickTumblerPrivate *tumblerPrivate = QQuickTumblerPrivate::get(m_tumbler);
-        tumblerPrivate->lockWrap();
-        m_pathView->setModel(m_model);
-        tumblerPrivate->unlockWrap();
-
-        // The count-depends-on-wrap behavior could cause wrap to change after
-        // the call above, so we must check that we're still using a PathView.
-        if (m_pathView)
-            m_pathView->setHighlightMoveDuration(oldHighlightMoveDuration);
-    } else if (m_listView && !m_listView->model().isValid() && m_model.isValid()) {
-        // Usually we'd do this in QQuickTumbler::setWrap(), but that will be too early for polishes.
-        const int currentIndex = m_tumbler->currentIndex();
-        m_listView->setModel(m_model);
-        m_listView->setCurrentIndex(currentIndex);
-    }
-}
-
 QQuickItem *QQuickTumblerView::view()
 {
     if (!m_tumbler)
diff --git a/src/quickcontrols2/qquicktumblerview_p.h b/src/quickcontrols2/qquicktumblerview_p.h
index e83a8bd2..0ab0c3a9 100644
--- a/src/quickcontrols2/qquicktumblerview_p.h
+++ b/src/quickcontrols2/qquicktumblerview_p.h
@@ -87,12 +87,12 @@ protected:
     void geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry) override;
     void componentComplete() override;
     void itemChange(ItemChange change, const ItemChangeData &data) override;
-    void updatePolish() override;
 
 private:
     QQuickItem *view();
     void createView();
     void updateView();
+    void updateModel();
 
     void wrapChange();