Delay removal/readd of map items until incubation completion on reset

When the model is reset, QDeclarativeGeoMapItemView currently removes
all map items and then begins recreating new delegates for model items.

This causes flickering since the mapitems are removed at once, but
the repopulation is asynchronous and allows the scene graph to repaint
in the meanwhile.

This patch delays the removal of current map items until all the new
items have been all incubated, so that removal and readd happen at
the same point.

The patch also adds additional checks to methods like
itemModelRowsRemoved, to verify if an item is still incubating.

Task-number: QTBUG-52301
Task-number: QTBUG-54188
Change-Id: I2f27114c3898bd61b1282b1f27b5f062bed1fe35
Reviewed-by: Alex Blasche <alexander.blasche@theqtcompany.com>

7 files changed, 259 insertions, 81 deletions

diff --git a/src/imports/location/location.pro b/src/imports/location/location.pro
index 7441fab3..f55a88a7 100644
--- a/src/imports/location/location.pro
+++ b/src/imports/location/location.pro
@@ -29,7 +29,8 @@ HEADERS += \
            locationvaluetypehelper_p.h\
            qquickgeomapgesturearea_p.h\
            ../positioning/qquickgeocoordinateanimation_p.h \
-           mapitemviewdelegateincubator.h
+           mapitemviewdelegateincubator.h \
+           qdeclarativegeomapitemview_p_p.h
 
 SOURCES += \
            location.cpp \
diff --git a/src/imports/location/mapitemviewdelegateincubator.cpp b/src/imports/location/mapitemviewdelegateincubator.cpp
index 98f0d460..06dee7ba 100644
--- a/src/imports/location/mapitemviewdelegateincubator.cpp
+++ b/src/imports/location/mapitemviewdelegateincubator.cpp
@@ -37,17 +37,18 @@
 
 #include "mapitemviewdelegateincubator.h"
 #include "qdeclarativegeomapitemview_p.h"
+#include "qdeclarativegeomapitemview_p_p.h"
 
 QT_BEGIN_NAMESPACE
 
-MapItemViewDelegateIncubator::MapItemViewDelegateIncubator(QDeclarativeGeoMapItemView *view)
-:   m_view(view)
+MapItemViewDelegateIncubator::MapItemViewDelegateIncubator(QDeclarativeGeoMapItemView *view, QDeclarativeGeoMapItemViewItemData *itemData, bool batched)
+:   m_view(view), m_itemData(itemData), m_batched(batched)
 {
 }
 
 void MapItemViewDelegateIncubator::statusChanged(QQmlIncubator::Status status)
 {
-    m_view->incubatorStatusChanged(this, status);
+    m_view->incubatorStatusChanged(this, status, m_batched);
 }
 
 QT_END_NAMESPACE
diff --git a/src/imports/location/mapitemviewdelegateincubator.h b/src/imports/location/mapitemviewdelegateincubator.h
index aa82a6bf..94c73252 100644
--- a/src/imports/location/mapitemviewdelegateincubator.h
+++ b/src/imports/location/mapitemviewdelegateincubator.h
@@ -38,6 +38,7 @@
 #define MAPITEMVIEWDELEGATEINCUBATOR_H
 
 #include <QtQml/QQmlIncubator>
+#include "qdeclarativegeomapitemview_p_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -46,13 +47,17 @@ class QDeclarativeGeoMapItemView;
 class MapItemViewDelegateIncubator : public QQmlIncubator
 {
 public:
-    MapItemViewDelegateIncubator(QDeclarativeGeoMapItemView *view);
+    MapItemViewDelegateIncubator(QDeclarativeGeoMapItemView *view, QDeclarativeGeoMapItemViewItemData *itemData, bool batched = true);
 
 protected:
     void statusChanged(Status status) Q_DECL_OVERRIDE;
 
 private:
     QDeclarativeGeoMapItemView *m_view;
+    QDeclarativeGeoMapItemViewItemData *m_itemData;
+    bool m_batched;
+
+    friend class QDeclarativeGeoMapItemView;
 };
 
 QT_END_NAMESPACE
diff --git a/src/imports/location/qdeclarativegeomapitemview.cpp b/src/imports/location/qdeclarativegeomapitemview.cpp
index 3e17e13a..0a9128f3 100644
--- a/src/imports/location/qdeclarativegeomapitemview.cpp
+++ b/src/imports/location/qdeclarativegeomapitemview.cpp
@@ -75,7 +75,8 @@ QT_BEGIN_NAMESPACE
 
 QDeclarativeGeoMapItemView::QDeclarativeGeoMapItemView(QQuickItem *parent)
     : QObject(parent), componentCompleted_(false), delegate_(0),
-      itemModel_(0), map_(0), fitViewport_(false), m_metaObjectType(0)
+      itemModel_(0), map_(0), fitViewport_(false), m_metaObjectType(0),
+      m_readyIncubators(0), m_repopulating(false)
 {
 }
 
@@ -95,45 +96,73 @@ void QDeclarativeGeoMapItemView::componentComplete()
 }
 
 void QDeclarativeGeoMapItemView::incubatorStatusChanged(MapItemViewDelegateIncubator *incubator,
-                                                        QQmlIncubator::Status status)
+                                                        QQmlIncubator::Status status,
+                                                        bool batched)
 {
     if (status == QQmlIncubator::Loading)
         return;
 
-    for (int i = 0; i < m_itemData.length(); ++i) {
-        ItemData *itemData = m_itemData.at(i);
-        if (itemData->incubator != incubator)
-            continue;
+    QDeclarativeGeoMapItemViewItemData *itemData = incubator->m_itemData;
+    if (!itemData) {
+        // Should never get here
+        qWarning() << "MapItemViewDelegateIncubator incubating invalid itemData";
+        return;
+    }
 
-        switch (status) {
-        case QQmlIncubator::Ready:
-            itemData->item = qobject_cast<QDeclarativeGeoMapItemBase *>(incubator->object());
+    switch (status) {
+    case QQmlIncubator::Ready:
+        {
+            QDeclarativeGeoMapItemBase *item = qobject_cast<QDeclarativeGeoMapItemBase *>(incubator->object());
+            if (!item)
+                break;
+            itemData->item = item;
             if (!itemData->item) {
                 qWarning() << "QDeclarativeGeoMapItemView map item delegate is of unsupported type.";
                 delete incubator->object();
             } else {
-                map_->addMapItem(itemData->item);
-                if (fitViewport_)
+                if (!batched) {
+                    map_->addMapItem(itemData->item);
                     fitViewport();
+                } else {
+                    ++m_readyIncubators; // QSemaphore not needed as multiple threads not involved
+
+                    if (m_readyIncubators == m_itemDataBatched.size()) {
+
+                        // Clearing stuff older than the reset
+                        foreach (QDeclarativeGeoMapItemViewItemData *i, m_itemData)
+                            removeItemData(i);
+                        m_itemData.clear();
+
+                        // Adding everthing created after reset was issued
+                        foreach (QDeclarativeGeoMapItemViewItemData *i, m_itemDataBatched) {
+                            map_->addMapItem(i->item);
+                        }
+                        m_itemData = m_itemDataBatched;
+                        m_itemDataBatched.clear();
+
+                        m_readyIncubators = 0;
+                        m_repopulating = false;
+
+                        fitViewport();
+                    }
+                }
             }
             delete itemData->incubator;
             itemData->incubator = 0;
             break;
-        case QQmlIncubator::Null:
-            // Should never get here
-            delete itemData->incubator;
-            itemData->incubator = 0;
-            break;
-        case QQmlIncubator::Error:
-            qWarning() << "QDeclarativeGeoMapItemView map item creation failed.";
-            delete itemData->incubator;
-            itemData->incubator = 0;
-            break;
-        default:
-            ;
         }
-
+    case QQmlIncubator::Null:
+        // Should never get here
+        delete itemData->incubator;
+        itemData->incubator = 0;
+        break;
+    case QQmlIncubator::Error:
+        qWarning() << "QDeclarativeGeoMapItemView map item creation failed.";
+        delete itemData->incubator;
+        itemData->incubator = 0;
         break;
+    default:
+        ;
     }
 }
 
@@ -165,7 +194,7 @@ void QDeclarativeGeoMapItemView::setModel(const QVariant &model)
         disconnect(itemModel_, SIGNAL(dataChanged(QModelIndex,QModelIndex,QVector<int>)),
                    this, SLOT(itemModelDataChanged(QModelIndex,QModelIndex,QVector<int>)));
 
-        removeInstantiatedItems();
+        removeInstantiatedItems(); // this also terminates ongong repopulations.
         m_metaObjectType->release();
         m_metaObjectType = 0;
 
@@ -214,11 +243,13 @@ void QDeclarativeGeoMapItemView::itemModelRowsInserted(const QModelIndex &index,
 
     for (int i = start; i <= end; ++i) {
         const QModelIndex insertedIndex = itemModel_->index(i, 0, index);
-        createItemForIndex(insertedIndex);
+        // If ran inside a qquickwidget which forces incubators to be synchronous, this call won't happen
+        // with m_repopulating == true while incubators from a model reset are still incubating.
+        // Note that having the model in a different thread is not supported in general.
+        createItemForIndex(insertedIndex, m_repopulating);
     }
 
-    if (fitViewport_)
-        fitViewport();
+    fitViewport();
 }
 
 /*!
@@ -232,16 +263,25 @@ void QDeclarativeGeoMapItemView::itemModelRowsRemoved(const QModelIndex &index,
         return;
 
     for (int i = end; i >= start; --i) {
-        ItemData *itemData = m_itemData.takeAt(i);
-        if (!itemData)
-            break;
-
-        map_->removeMapItem(itemData->item);
-        delete itemData;
+        if (m_repopulating) {
+            QDeclarativeGeoMapItemViewItemData *itemData = m_itemDataBatched.takeAt(i);
+            if (!itemData)
+                continue;
+            if (itemData->incubator) {
+                if (itemData->incubator->isReady()) {
+                    --m_readyIncubators;
+                    delete itemData->incubator->object();
+                }
+                itemData->incubator->clear();
+            }
+            delete itemData;
+        } else {
+            QDeclarativeGeoMapItemViewItemData *itemData = m_itemData.takeAt(i);
+            removeItemData(itemData);
+        }
     }
 
-    if (fitViewport_)
-        fitViewport();
+    fitViewport();
 }
 
 void QDeclarativeGeoMapItemView::itemModelRowsMoved(const QModelIndex &parent, int start, int end,
@@ -262,12 +302,16 @@ void QDeclarativeGeoMapItemView::itemModelDataChanged(const QModelIndex &topLeft
 {
     Q_UNUSED(roles)
 
-    if (!m_itemData.count())
+    if (!m_itemData.count() || (m_repopulating && !m_itemDataBatched.count()) )
         return;
 
     for (int i = topLeft.row(); i <= bottomRight.row(); ++i) {
         const QModelIndex index = itemModel_->index(i, 0);
-        ItemData *itemData = m_itemData.at(i);
+        QDeclarativeGeoMapItemViewItemData *itemData;
+        if (m_repopulating)
+            itemData= m_itemDataBatched.at(i);
+        else
+            itemData= m_itemData.at(i);
 
         QHashIterator<int, QByteArray> iterator(itemModel_->roleNames());
         while (iterator.hasNext()) {
@@ -279,7 +323,6 @@ void QDeclarativeGeoMapItemView::itemModelDataChanged(const QModelIndex &topLeft
 
             itemData->context->setContextProperty(QString::fromLatin1(iterator.value().constData()),
                                                   modelData);
-
             itemData->modelDataMeta->setValue(iterator.value(), modelData);
         }
     }
@@ -334,7 +377,7 @@ void QDeclarativeGeoMapItemView::setAutoFitViewport(const bool &fitViewport)
 */
 void QDeclarativeGeoMapItemView::fitViewport()
 {
-    if (!map_ || !fitViewport_)
+    if (!map_ || !fitViewport_ || m_repopulating)
         return;
 
     if (map_->mapItems().size() > 0)
@@ -359,10 +402,9 @@ void QDeclarativeGeoMapItemView::removeInstantiatedItems()
     if (!map_)
         return;
 
-    foreach (ItemData *itemData, m_itemData) {
-        map_->removeMapItem(itemData->item);
-        delete itemData;
-    }
+    terminateOngoingRepopulation();
+    foreach (QDeclarativeGeoMapItemViewItemData *itemData, m_itemData)
+        removeItemData(itemData);
     m_itemData.clear();
 }
 
@@ -375,14 +417,53 @@ void QDeclarativeGeoMapItemView::instantiateAllItems()
 {
     if (!componentCompleted_ || !map_ || !delegate_ || !itemModel_)
         return;
+    Q_ASSERT(!m_itemDataBatched.size());
+    m_repopulating = true;
 
+    // QQuickWidget forces incubators to synchronous mode. Thus itemDataChanged gets called during the for loop below.
+    m_itemDataBatched.resize(itemModel_->rowCount());
     for (int i = 0; i < itemModel_->rowCount(); ++i) {
         const QModelIndex index = itemModel_->index(i, 0);
-        createItemForIndex(index);
+        createItemForIndex(index, true);
+    }
+
+    fitViewport();
+}
+
+void QDeclarativeGeoMapItemView::removeItemData(QDeclarativeGeoMapItemViewItemData *itemData)
+{
+    if (!itemData)
+        return;
+    if (itemData->incubator) {
+        if (itemData->incubator->isReady()) {
+            if (itemData->incubator->object() == itemData->item) {
+                map_->removeMapItem(itemData->item); // removeMapItem checks whether the item is in the map, so it's safe to call.
+                itemData->item = 0;
+            }
+            delete itemData->incubator->object();
+        }
+        itemData->incubator->clear(); // stops ongoing incubation
     }
+    if (itemData->item)
+        map_->removeMapItem(itemData->item);
+    delete itemData; // destroys the ->item too.
+}
 
-    if (fitViewport_)
-        fitViewport();
+void QDeclarativeGeoMapItemView::terminateOngoingRepopulation()
+{
+    if (m_repopulating) {
+        // Terminate the previous resetting task. Not all incubators finished, but
+        // QQmlIncubatorController operates in the same thread, so it is safe
+        // to check, here, whether incubators are ready or not, without having
+        // to race with them.
+
+        foreach (QDeclarativeGeoMapItemViewItemData *itemData, m_itemDataBatched)
+            removeItemData(itemData);
+
+        m_itemDataBatched.clear();
+        m_readyIncubators = 0;
+        m_repopulating = false;
+    }
 }
 
 /*!
@@ -391,20 +472,27 @@ void QDeclarativeGeoMapItemView::instantiateAllItems()
 */
 void QDeclarativeGeoMapItemView::repopulate()
 {
-    removeInstantiatedItems();
-    instantiateAllItems();
+    if (!itemModel_ || !itemModel_->rowCount()) {
+        removeInstantiatedItems();
+    } else {
+        terminateOngoingRepopulation();
+        instantiateAllItems(); // removal of instantiated item done at incubation completion
+    }
 }
 
 /*!
     \internal
+
+    Note: this call is async. that is returns to the event loop before returning to the caller.
+    May also trigger incubatorStatusChanged() before returning to the caller if the incubator is fast enough.
 */
-void QDeclarativeGeoMapItemView::createItemForIndex(const QModelIndex &index)
+void QDeclarativeGeoMapItemView::createItemForIndex(const QModelIndex &index, bool batched)
 {
     // Expected to be already tested by caller.
     Q_ASSERT(delegate_);
     Q_ASSERT(itemModel_);
 
-    ItemData *itemData = new ItemData;
+    QDeclarativeGeoMapItemViewItemData *itemData = new QDeclarativeGeoMapItemViewItemData;
 
     itemData->modelData = new QObject;
     itemData->modelDataMeta = new QQmlOpenMetaObject(itemData->modelData, m_metaObjectType, false);
@@ -427,13 +515,19 @@ void QDeclarativeGeoMapItemView::createItemForIndex(const QModelIndex &index)
     itemData->context->setContextProperty(QLatin1String("model"), itemData->modelData);
     itemData->context->setContextProperty(QLatin1String("index"), index.row());
 
-    itemData->incubator = new MapItemViewDelegateIncubator(this);
-    delegate_->create(*itemData->incubator, itemData->context);
+    if (batched || m_repopulating) {
+        if (index.row() < m_itemDataBatched.size())
+            m_itemDataBatched.replace(index.row(), itemData);
+        else
+            m_itemDataBatched.insert(index.row(), itemData);
+    } else
+        m_itemData.insert(index.row(), itemData);
+    itemData->incubator = new MapItemViewDelegateIncubator(this, itemData, batched || m_repopulating);
 
-    m_itemData.insert(index.row(), itemData);
+    delegate_->create(*itemData->incubator, itemData->context);
 }
 
-QDeclarativeGeoMapItemView::ItemData::~ItemData()
+QDeclarativeGeoMapItemViewItemData::~QDeclarativeGeoMapItemViewItemData()
 {
     delete incubator;
     delete item;
diff --git a/src/imports/location/qdeclarativegeomapitemview_p.h b/src/imports/location/qdeclarativegeomapitemview_p.h
index 004e3fb9..6a4a2921 100644
--- a/src/imports/location/qdeclarativegeomapitemview_p.h
+++ b/src/imports/location/qdeclarativegeomapitemview_p.h
@@ -54,6 +54,7 @@
 #include <QtQml/QQmlParserStatus>
 #include <QtQml/QQmlIncubator>
 #include <QtQml/qqml.h>
+#include "qdeclarativegeomapitemview_p_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -108,7 +109,8 @@ Q_SIGNALS:
 
 protected:
     void incubatorStatusChanged(MapItemViewDelegateIncubator *incubator,
-                                QQmlIncubator::Status status);
+                                QQmlIncubator::Status status,
+                                bool batched);
 
 private Q_SLOTS:
     void itemModelReset();
@@ -120,39 +122,27 @@ private Q_SLOTS:
                               const QVector<int> &roles);
 
 private:
-    struct ItemData {
-        ItemData()
-        :   incubator(0), item(0), context(0), modelData(0), modelDataMeta(0)
-        {
-        }
-
-        ~ItemData();
-
-        MapItemViewDelegateIncubator *incubator;
-        QDeclarativeGeoMapItemBase *item;
-        QQmlContext *context;
-        QObject *modelData;
-        QQmlOpenMetaObject *modelDataMeta;
-    };
-
-    void createItemForIndex(const QModelIndex &index);
+    void createItemForIndex(const QModelIndex &index, bool batched = false);
     void fitViewport();
+    void terminateOngoingRepopulation();
+    void removeItemData(QDeclarativeGeoMapItemViewItemData *itemData);
 
     bool componentCompleted_;
     QQmlComponent *delegate_;
     QAbstractItemModel *itemModel_;
     QDeclarativeGeoMap *map_;
-    QVector<ItemData *> m_itemData;
+    QVector<QDeclarativeGeoMapItemViewItemData *> m_itemData;
+    QVector<QDeclarativeGeoMapItemViewItemData *> m_itemDataBatched;
     bool fitViewport_;
 
     QQmlOpenMetaObjectType *m_metaObjectType;
+    int m_readyIncubators;
+    bool m_repopulating;
 
-    friend class QTypeInfo<ItemData>;
+    friend class QDeclarativeGeoMapItemViewItemData;
     friend class MapItemViewDelegateIncubator;
 };
 
-Q_DECLARE_TYPEINFO(QDeclarativeGeoMapItemView::ItemData, Q_MOVABLE_TYPE);
-
 QT_END_NAMESPACE
 
 QML_DECLARE_TYPE(QDeclarativeGeoMapItemView)
diff --git a/src/imports/location/qdeclarativegeomapitemview_p_p.h b/src/imports/location/qdeclarativegeomapitemview_p_p.h
new file mode 100644
index 00000000..5a4e3b25
--- /dev/null
+++ b/src/imports/location/qdeclarativegeomapitemview_p_p.h
@@ -0,0 +1,85 @@
+/****************************************************************************
+**
+** Copyright (C) 2016 The Qt Company Ltd.
+** Contact: http://www.qt.io/licensing/
+**
+** This file is part of the QtLocation module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL3$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see http://www.qt.io/terms-conditions. For further
+** information use the contact form at http://www.qt.io/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPLv3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or later as published by the Free
+** Software Foundation and appearing in the file LICENSE.GPL included in
+** the packaging of this file. Please review the following information to
+** ensure the GNU General Public License version 2.0 requirements will be
+** met: http://www.gnu.org/licenses/gpl-2.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#ifndef QDECLARATIVEGEOMAPITEMVIEW_P_P_H
+#define QDECLARATIVEGEOMAPITEMVIEW_P_P_H
+
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists purely as an
+// implementation detail.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <QtCore/QModelIndex>
+#include <QtQml/QQmlParserStatus>
+#include <QtQml/QQmlIncubator>
+#include <QtQml/qqml.h>
+#include <QtQml/private/qqmlopenmetaobject_p.h>
+
+QT_BEGIN_NAMESPACE
+
+class MapItemViewDelegateIncubator;
+class QDeclarativeGeoMapItemView;
+class QDeclarativeGeoMapItemBase;
+
+struct QDeclarativeGeoMapItemViewItemData {
+    QDeclarativeGeoMapItemViewItemData()
+    :   incubator(0), item(0), context(0), modelData(0), modelDataMeta(0)
+    {
+    }
+
+    ~QDeclarativeGeoMapItemViewItemData();
+
+    MapItemViewDelegateIncubator *incubator;
+    QDeclarativeGeoMapItemBase *item;
+    QQmlContext *context;
+    QObject *modelData;
+    QQmlOpenMetaObject *modelDataMeta;
+
+    friend class MapItemViewDelegateIncubator;
+    friend class QDeclarativeGeoMapItemView;
+};
+
+Q_DECLARE_TYPEINFO(QDeclarativeGeoMapItemViewItemData, Q_MOVABLE_TYPE);
+
+QT_END_NAMESPACE
+
+#endif // QDECLARATIVEGEOMAPITEMVIEW_P_P_H
diff --git a/tests/auto/declarative_ui/tst_map_itemview.qml b/tests/auto/declarative_ui/tst_map_itemview.qml
index 5fe00d97..25853835 100644
--- a/tests/auto/declarative_ui/tst_map_itemview.qml
+++ b/tests/auto/declarative_ui/tst_map_itemview.qml
@@ -398,6 +398,8 @@ Item {
             tryCompare(mapForView, "mapItemsLength", 7)
             testModel.datacount += 2
             testModel2.datacount += 1
+            // delegate spawning is async. wait a bit.
+            wait(1)
             tryCompare(mapForView, "mapItemsLength", 9)
 
             theItemView.model = testModel