Make Tumbler compatible with deferred execution

Now that the contentItem is created at a different stage, we
have to shuffle some code around and add some null checks.

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

8 files changed, 198 insertions, 76 deletions

diff --git a/src/quickcontrols2/qquicktumblerview.cpp b/src/quickcontrols2/qquicktumblerview.cpp
index e8f0c364..4469810c 100644
--- a/src/quickcontrols2/qquicktumblerview.cpp
+++ b/src/quickcontrols2/qquicktumblerview.cpp
@@ -173,14 +173,19 @@ void QQuickTumblerView::createView()
             m_listView->setParentItem(this);
             m_listView->setSnapMode(QQuickListView::SnapToItem);
             m_listView->setHighlightRangeMode(QQuickListView::StrictlyEnforceRange);
-            m_listView->setHighlightMoveDuration(1000);
             m_listView->setClip(true);
-            m_listView->setDelegate(m_delegate);
 
             // Give the view a size.
             updateView();
             // Set the model.
             updateModel();
+
+            // Set these after the model is set so that the currentItem animation
+            // happens instantly on startup/after switching models. If we set them too early,
+            // the view animates any potential currentIndex change over one second,
+            // which we don't want when the contentItem has just been created.
+            m_listView->setDelegate(m_delegate);
+            m_listView->setHighlightMoveDuration(1000);
         }
     }
 }
diff --git a/src/quicktemplates2/qquicktumbler.cpp b/src/quicktemplates2/qquicktumbler.cpp
index 3429f1f4..241a5fc3 100644
--- a/src/quicktemplates2/qquicktumbler.cpp
+++ b/src/quicktemplates2/qquicktumbler.cpp
@@ -119,6 +119,11 @@ namespace {
 */
 QQuickItem *QQuickTumblerPrivate::determineViewType(QQuickItem *contentItem)
 {
+    if (!contentItem) {
+        resetViewData();
+        return nullptr;
+    }
+
     if (contentItem->inherits("QQuickPathView")) {
         view = contentItem;
         viewContentItem = contentItem;
@@ -139,6 +144,7 @@ QQuickItem *QQuickTumblerPrivate::determineViewType(QQuickItem *contentItem)
     }
 
     resetViewData();
+    viewContentItemType = UnsupportedContentItemType;
     return nullptr;
 }
 
@@ -146,7 +152,7 @@ void QQuickTumblerPrivate::resetViewData()
 {
     view = nullptr;
     viewContentItem = nullptr;
-    viewContentItemType = UnsupportedContentItemType;
+    viewContentItemType = NoContentItem;
 }
 
 QList<QQuickItem *> QQuickTumblerPrivate::viewContentItemChildItems() const
@@ -484,8 +490,6 @@ void QQuickTumbler::componentComplete()
 {
     Q_D(QQuickTumbler);
     QQuickControl::componentComplete();
-    d->_q_updateItemHeights();
-    d->_q_updateItemWidths();
 
     if (!d->view) {
         // Force the view to be created.
@@ -493,6 +497,17 @@ void QQuickTumbler::componentComplete()
         // Determine the type of view for attached properties, etc.
         d->setupViewData(d->contentItem);
     }
+
+    // If there was no contentItem or it was of an unsupported type,
+    // we don't have anything else to do.
+    if (!d->view)
+        return;
+
+    // Update item heights after we've populated the model,
+    // otherwise ignoreSignals will cause these functions to return early.
+    d->_q_updateItemHeights();
+    d->_q_updateItemWidths();
+    d->_q_onViewCountChanged();
 }
 
 void QQuickTumbler::contentItemChange(QQuickItem *newItem, QQuickItem *oldItem)
@@ -511,6 +526,9 @@ void QQuickTumbler::contentItemChange(QQuickItem *newItem, QQuickItem *oldItem)
             // Make sure we use the new content item and not the current one, as that won't
             // be changed until after contentItemChange() has finished.
             d->setupViewData(newItem);
+
+            d->_q_updateItemHeights();
+            d->_q_updateItemWidths();
         }
     }
 }
@@ -545,6 +563,9 @@ void QQuickTumblerPrivate::setupViewData(QQuickItem *newControlContentItem)
 
     determineViewType(newControlContentItem);
 
+    if (viewContentItemType == QQuickTumblerPrivate::NoContentItem)
+        return;
+
     if (viewContentItemType == QQuickTumblerPrivate::UnsupportedContentItemType) {
         qWarning() << "Tumbler: contentItem must contain either a PathView or a ListView";
         return;
@@ -561,6 +582,16 @@ void QQuickTumblerPrivate::setupViewData(QQuickItem *newControlContentItem)
 
     // Sync the view's currentIndex with ours.
     syncCurrentIndex();
+
+    const auto items = viewContentItemChildItems();
+    for (QQuickItem *childItem : items) {
+        QQuickTumblerAttached *attached = qobject_cast<QQuickTumblerAttached *>(
+            qmlAttachedPropertiesObject<QQuickTumbler>(childItem, false));
+        if (attached) {
+            QQuickTumblerAttachedPrivate *attachedPrivate = QQuickTumblerAttachedPrivate::get(attached);
+            attachedPrivate->viewDataSetUp();
+        }
+    }
 }
 
 void QQuickTumblerPrivate::syncCurrentIndex()
@@ -568,17 +599,27 @@ void QQuickTumblerPrivate::syncCurrentIndex()
     const int actualViewIndex = view->property("currentIndex").toInt();
     Q_Q(QQuickTumbler);
 
+    const bool isPendingCurrentIndex = pendingCurrentIndex != -1;
+    const int indexToSet = isPendingCurrentIndex ? pendingCurrentIndex : currentIndex;
+
     // Nothing to do.
-    if (actualViewIndex == currentIndex)
+    if (actualViewIndex == indexToSet) {
+        pendingCurrentIndex = -1;
         return;
+    }
 
     // PathView likes to use 0 as currentIndex for empty models, but we use -1 for that.
     if (q->count() == 0 && actualViewIndex == 0)
         return;
 
     ignoreCurrentIndexChanges = true;
-    view->setProperty("currentIndex", currentIndex);
+    view->setProperty("currentIndex", QVariant(indexToSet));
     ignoreCurrentIndexChanges = false;
+
+    if (view->property("currentIndex").toInt() == indexToSet)
+        pendingCurrentIndex = -1;
+    else if (isPendingCurrentIndex)
+        q->polish();
 }
 
 void QQuickTumblerPrivate::setCount(int newCount)
@@ -629,11 +670,17 @@ void QQuickTumblerPrivate::setWrap(bool shouldWrap, bool isExplicit)
 
     ignoreCurrentIndexChanges = false;
 
-    // The view should have been created now, so we can start determining its type, etc.
-    // If the delegates use attached properties, this will have already been called,
-    // in which case it will return early. If the delegate doesn't use attached properties,
-    // we need to call it here.
-    setupViewData(contentItem);
+    // If isComponentComplete() is true, we require a contentItem. If it's not
+    // true, it might not have been created yet, so we wait until
+    // componentComplete() is called.
+    //
+    // When the contentItem (usually QQuickTumblerView) has been created, we
+    // can start determining its type, etc. If the delegates use attached
+    // properties, this will have already been called, in which case it will
+    // return early. If the delegate doesn't use attached properties, we need
+    // to call it here.
+    if (q->isComponentComplete() || contentItem)
+        setupViewData(contentItem);
 
     q->setCurrentIndex(oldCurrentIndex);
 }
@@ -668,6 +715,10 @@ void QQuickTumbler::updatePolish()
 {
     Q_D(QQuickTumbler);
     if (d->pendingCurrentIndex != -1) {
+        // Update our count, as ignoreSignals might have been true
+        // when _q_onViewCountChanged() was last called.
+        d->setCount(d->view->property("count").toInt());
+
         // If the count is still 0, it's not going to happen.
         if (d->count == 0) {
             d->pendingCurrentIndex = -1;
@@ -689,53 +740,34 @@ void QQuickTumbler::updatePolish()
     }
 }
 
-class QQuickTumblerAttachedPrivate : public QObjectPrivate, public QQuickItemChangeListener
+QQuickTumblerAttachedPrivate::QQuickTumblerAttachedPrivate()
+    : tumbler(nullptr),
+      index(-1),
+      displacement(0)
 {
-    Q_DECLARE_PUBLIC(QQuickTumblerAttached)
-public:
-    QQuickTumblerAttachedPrivate()
-        : tumbler(nullptr),
-          index(-1),
-          displacement(0)
-    {
-    }
-
-    void init(QQuickItem *delegateItem)
-    {
-        if (!delegateItem->parentItem()) {
-            qWarning() << "Tumbler: attached properties must be accessed through a delegate item that has a parent";
-            return;
-        }
-
-        QVariant indexContextProperty = qmlContext(delegateItem)->contextProperty(QStringLiteral("index"));
-        if (!indexContextProperty.isValid()) {
-            qWarning() << "Tumbler: attempting to access attached property on item without an \"index\" property";
-            return;
-        }
-
-        index = indexContextProperty.toInt();
+}
 
-        QQuickItem *parentItem = delegateItem;
-        while ((parentItem = parentItem->parentItem())) {
-            if ((tumbler = qobject_cast<QQuickTumbler*>(parentItem)))
-                break;
-        }
+void QQuickTumblerAttachedPrivate::init(QQuickItem *delegateItem)
+{
+    if (!delegateItem->parentItem()) {
+        qWarning() << "Tumbler: attached properties must be accessed through a delegate item that has a parent";
+        return;
     }
 
-    void itemGeometryChanged(QQuickItem *item, QQuickGeometryChange change, const QRectF &diff) override;
-    void itemChildAdded(QQuickItem *, QQuickItem *) override;
-    void itemChildRemoved(QQuickItem *, QQuickItem *) override;
+    QVariant indexContextProperty = qmlContext(delegateItem)->contextProperty(QStringLiteral("index"));
+    if (!indexContextProperty.isValid()) {
+        qWarning() << "Tumbler: attempting to access attached property on item without an \"index\" property";
+        return;
+    }
 
-    void _q_calculateDisplacement();
-    void emitIfDisplacementChanged(qreal oldDisplacement, qreal newDisplacement);
+    index = indexContextProperty.toInt();
 
-    // The Tumbler that contains the delegate. Required to calculated the displacement.
-    QPointer<QQuickTumbler> tumbler;
-    // The index of the delegate. Used to calculate the displacement.
-    int index;
-    // The displacement for our delegate.
-    qreal displacement;
-};
+    QQuickItem *parentItem = delegateItem;
+    while ((parentItem = parentItem->parentItem())) {
+        if ((tumbler = qobject_cast<QQuickTumbler*>(parentItem)))
+            break;
+    }
+}
 
 void QQuickTumblerAttachedPrivate::itemGeometryChanged(QQuickItem *, QQuickGeometryChange, const QRectF &)
 {
@@ -768,9 +800,9 @@ void QQuickTumblerAttachedPrivate::_q_calculateDisplacement()
     const int previousDisplacement = displacement;
     displacement = 0;
 
-    // Can happen if the attached properties are accessed on the wrong type of item or the tumbler was destroyed.
     if (!tumbler) {
-        emitIfDisplacementChanged(previousDisplacement, displacement);
+        // Can happen if the attached properties are accessed on the wrong type of item or the tumbler was destroyed.
+        // We don't want to emit the change signal though, as this could cause warnings about Tumbler.tumbler being null.
         return;
     }
 
@@ -820,6 +852,28 @@ void QQuickTumblerAttachedPrivate::emitIfDisplacementChanged(qreal oldDisplaceme
         emit q->displacementChanged();
 }
 
+void QQuickTumblerAttachedPrivate::viewDataSetUp()
+{
+    Q_Q(QQuickTumblerAttached);
+    QQuickTumblerPrivate *tumblerPrivate = QQuickTumblerPrivate::get(tumbler);
+    if (!tumblerPrivate->viewContentItem)
+        return;
+
+    QQuickItemPrivate *viewContentItemPrivate = QQuickItemPrivate::get(tumblerPrivate->viewContentItem);
+    viewContentItemPrivate->addItemChangeListener(this, QQuickItemPrivate::Geometry | QQuickItemPrivate::Children);
+
+    const char *contentItemSignal = tumblerPrivate->viewContentItemType == QQuickTumblerPrivate::PathViewContentItem
+        ? SIGNAL(offsetChanged()) : SIGNAL(contentYChanged());
+    QObject::connect(tumblerPrivate->view, contentItemSignal, q, SLOT(_q_calculateDisplacement()));
+
+    _q_calculateDisplacement();
+}
+
+QQuickTumblerAttachedPrivate *QQuickTumblerAttachedPrivate::get(QQuickTumblerAttached *attached)
+{
+    return attached->d_func();
+}
+
 QQuickTumblerAttached::QQuickTumblerAttached(QObject *parent)
     : QObject(*(new QQuickTumblerAttachedPrivate), parent)
 {
@@ -839,17 +893,24 @@ QQuickTumblerAttached::QQuickTumblerAttached(QObject *parent)
         QQuickTumblerPrivate *tumblerPrivate = QQuickTumblerPrivate::get(d->tumbler);
         tumblerPrivate->setupViewData(tumblerPrivate->contentItem);
 
-        if (!tumblerPrivate->viewContentItem)
-            return;
-
-        QQuickItemPrivate *p = QQuickItemPrivate::get(tumblerPrivate->viewContentItem);
-        p->addItemChangeListener(d, QQuickItemPrivate::Geometry | QQuickItemPrivate::Children);
-
-        const char *contentItemSignal = tumblerPrivate->viewContentItemType == QQuickTumblerPrivate::PathViewContentItem
-            ? SIGNAL(offsetChanged()) : SIGNAL(contentYChanged());
-        connect(tumblerPrivate->view, contentItemSignal, this, SLOT(_q_calculateDisplacement()));
-
-        d->_q_calculateDisplacement();
+        // When creating a custom contentItem with e.g. Component's createObject() function,
+        // the item is created and *then* assigned to Tumbler's contentItem property,
+        // meaning that, at this point, the tumbler's contentItem is still its old one,
+        // as the delegates and their attached objects are created before the contentItem is assigned.
+        // That's why setupViewData() calls viewDataSetUp() on each attached object:
+        // we have to be notified whenever the contentItem changes, as otherwise
+        // we may never end up using the correct view data, and end up calculating incorrect displacements.
+
+        if (delegateItem->parentItem() == tumblerPrivate->viewContentItem) {
+            // This item belongs to the "new" view, meaning that the tumbler's contentItem
+            // was probably assigned declaratively. If the contentItem was instead created
+            // dynamically and then assigned (as mentioned above), these two won't
+            // be equal and we'll have to wait until viewDataSetUp() is called before
+            // connecting signals to our slots. Since viewDataSetUp() is only called once
+            // for each contentItem, and every attached object after the first one that is
+            // created would miss the chance for it to be called, we call the slot manually here.
+            d->viewDataSetUp();
+        }
     }
 }
 
diff --git a/src/quicktemplates2/qquicktumbler_p.h b/src/quicktemplates2/qquicktumbler_p.h
index 480ebb23..de7de595 100644
--- a/src/quicktemplates2/qquicktumbler_p.h
+++ b/src/quicktemplates2/qquicktumbler_p.h
@@ -70,7 +70,6 @@ class Q_QUICKTEMPLATES2_PRIVATE_EXPORT QQuickTumbler : public QQuickControl
     Q_PROPERTY(bool wrap READ wrap WRITE setWrap RESET resetWrap NOTIFY wrapChanged FINAL REVISION 1)
     // 2.2 (Qt 5.9)
     Q_PROPERTY(bool moving READ isMoving NOTIFY movingChanged FINAL REVISION 2)
-    Q_CLASSINFO("DeferredPropertyNames", "background")
 
 public:
     explicit QQuickTumbler(QQuickItem *parent = nullptr);
diff --git a/src/quicktemplates2/qquicktumbler_p_p.h b/src/quicktemplates2/qquicktumbler_p_p.h
index 0dcae762..38b0f60d 100644
--- a/src/quicktemplates2/qquicktumbler_p_p.h
+++ b/src/quicktemplates2/qquicktumbler_p_p.h
@@ -64,6 +64,7 @@ public:
     ~QQuickTumblerPrivate();
 
     enum ContentItemType {
+        NoContentItem,
         UnsupportedContentItemType,
         PathViewContentItem,
         ListViewContentItem
@@ -109,6 +110,34 @@ public:
     void itemChildRemoved(QQuickItem *, QQuickItem *) override;
 };
 
+class Q_QUICKTEMPLATES2_PRIVATE_EXPORT QQuickTumblerAttachedPrivate : public QObjectPrivate, public QQuickItemChangeListener
+{
+    Q_DECLARE_PUBLIC(QQuickTumblerAttached)
+
+public:
+    QQuickTumblerAttachedPrivate();
+
+    void init(QQuickItem *delegateItem);
+
+    void itemGeometryChanged(QQuickItem *item, QQuickGeometryChange change, const QRectF &diff) override;
+    void itemChildAdded(QQuickItem *, QQuickItem *) override;
+    void itemChildRemoved(QQuickItem *, QQuickItem *) override;
+
+    void _q_calculateDisplacement();
+    void emitIfDisplacementChanged(qreal oldDisplacement, qreal newDisplacement);
+
+    void viewDataSetUp();
+
+    static QQuickTumblerAttachedPrivate *get(QQuickTumblerAttached *attached);
+
+    // The Tumbler that contains the delegate. Required to calculated the displacement.
+    QPointer<QQuickTumbler> tumbler;
+    // The index of the delegate. Used to calculate the displacement.
+    int index;
+    // The displacement for our delegate.
+    qreal displacement;
+};
+
 QT_END_NAMESPACE
 
 #endif // QQUICKTUMBLER_P_P_H
diff --git a/tests/auto/controls/data/TumblerListView.qml b/tests/auto/controls/data/TumblerListView.qml
index 4e71f471..1248bec0 100644
--- a/tests/auto/controls/data/TumblerListView.qml
+++ b/tests/auto/controls/data/TumblerListView.qml
@@ -49,15 +49,21 @@
 ****************************************************************************/
 
 import QtQuick 2.6
+import QtQuick.Controls 2.2
 
 ListView {
     anchors.fill: parent
-    model: parent.model
-    delegate: parent.delegate
-
     snapMode: ListView.SnapToItem
     highlightRangeMode: ListView.StrictlyEnforceRange
     preferredHighlightBegin: height / 2 - (height / parent.visibleItemCount / 2)
     preferredHighlightEnd: height / 2 + (height / parent.visibleItemCount / 2)
     clip: true
+    model: parent.model
+    delegate: Text {
+        objectName: text
+        text: "Custom" + modelData
+        opacity: 1.0 - Math.abs(Tumbler.displacement) / (Tumbler.tumbler.visibleItemCount / 2)
+        horizontalAlignment: Text.AlignHCenter
+        verticalAlignment: Text.AlignVCenter
+    }
 }
diff --git a/tests/auto/controls/data/TumblerPathView.qml b/tests/auto/controls/data/TumblerPathView.qml
index 4f6f653f..7b7cd5f4 100644
--- a/tests/auto/controls/data/TumblerPathView.qml
+++ b/tests/auto/controls/data/TumblerPathView.qml
@@ -49,16 +49,23 @@
 ****************************************************************************/
 
 import QtQuick 2.6
+import QtQuick.Controls 2.2
 
 PathView {
     id: pathView
-    model: parent.model
-    delegate: parent.delegate
     clip: true
     pathItemCount: parent.visibleItemCount + 1
     preferredHighlightBegin: 0.5
     preferredHighlightEnd: 0.5
     dragMargin: width / 2
+    model: parent.model
+    delegate: Text {
+        objectName: text
+        text: "Custom" + modelData
+        opacity: 1.0 - Math.abs(Tumbler.displacement) / (Tumbler.tumbler.visibleItemCount / 2)
+        horizontalAlignment: Text.AlignHCenter
+        verticalAlignment: Text.AlignVCenter
+    }
 
     path: Path {
         startX: pathView.width / 2
diff --git a/tests/auto/controls/data/tst_tumbler.qml b/tests/auto/controls/data/tst_tumbler.qml
index 825c2489..36808b8f 100644
--- a/tests/auto/controls/data/tst_tumbler.qml
+++ b/tests/auto/controls/data/tst_tumbler.qml
@@ -287,11 +287,10 @@ TestCase {
         compare(tumblerView.currentIndex, data.currentIndex);
         compare(tumblerView.currentItem.text, data.currentIndex.toString());
 
-        var fuzz = 1;
         if (data.wrap) {
-            fuzzyCompare(tumblerView.offset, data.currentIndex > 0 ? tumblerView.count - data.currentIndex : 0, fuzz);
+            tryCompare(tumblerView, "offset", data.currentIndex > 0 ? tumblerView.count - data.currentIndex : 0);
         } else {
-            fuzzyCompare(tumblerView.contentY, tumblerDelegateHeight * data.currentIndex - tumblerView.preferredHighlightBegin, fuzz);
+            tryCompare(tumblerView, "contentY", tumblerDelegateHeight * data.currentIndex - tumblerView.preferredHighlightBegin);
         }
     }
 
@@ -680,6 +679,17 @@ TestCase {
         compare(tumbler.currentIndex, 3);
     }
 
+    function findFirstDelegateWithText(view, text) {
+        var delegate = null;
+        var contentItem = view.hasOwnProperty("contentItem") ? view.contentItem : view;
+        for (var i = 0; i < contentItem.children.length && !delegate; ++i) {
+            var child = contentItem.children[i];
+            if (child.hasOwnProperty("text") && child.text === text)
+                delegate = child;
+        }
+        return delegate;
+    }
+
     function test_customContentItemAfterConstruction_data() {
         return [
             { tag: "ListView", componentPath: "TumblerListView.qml" },
@@ -705,6 +715,11 @@ TestCase {
         tumblerView = findView(tumbler);
         compare(tumblerView.currentIndex, 2);
 
+        var delegate = findFirstDelegateWithText(tumblerView, "Custom2");
+        verify(delegate);
+        compare(delegate.height, defaultImplicitDelegateHeight);
+        tryCompare(delegate.Tumbler, "displacement", 0);
+
         tumblerView.incrementCurrentIndex();
         compare(tumblerView.currentIndex, 3);
         compare(tumbler.currentIndex, 3);
diff --git a/tests/auto/customization/tst_customization.cpp b/tests/auto/customization/tst_customization.cpp
index 05d95ec8..3010565c 100644
--- a/tests/auto/customization/tst_customization.cpp
+++ b/tests/auto/customization/tst_customization.cpp
@@ -101,7 +101,7 @@ static const ControlInfo ControlInfos[] = {
     { "ToolButton", QStringList() << "background" << "contentItem" },
     { "ToolSeparator", QStringList() << "background" << "contentItem" },
     { "ToolTip", QStringList() << "background" << "contentItem" },
-    // { "Tumbler", QStringList() << "background" << "contentItem" } ### TODO: fix and enable deferred execution
+    { "Tumbler", QStringList() << "background" << "contentItem" }
 };
 
 class tst_customization : public QQmlDataTest