From 0af21b1f95180949f75ee2933d4147f610a29b6b Mon Sep 17 00:00:00 2001 From: Paolo Angelelli Date: Tue, 12 Mar 2019 20:35:26 +0100 Subject: Fix Map destructor incorrectly deleting MapItemViews Currently the views are explicitly destroyed, leading to issues, in particular for views declared inside the Map. Change it to simply remove these views from the map. Child views will be destroyed in ~QObject. [ChangeLog] Fixed crash when destroying Maps containing MapItemViews. Change-Id: Iff9b1afd6b17b55671b1f999b1bf69f172a05483 Fixes: QTBUG-69195 Fixes: QTBUG-74337 Reviewed-by: Eskil Abrahamsen Blomfeldt --- .../declarativemaps/qdeclarativegeomap.cpp | 58 ++++++++++++---------- .../declarativemaps/qdeclarativegeomapitemview.cpp | 55 ++++++++++++++------ .../declarativemaps/qdeclarativegeomapitemview_p.h | 2 + 3 files changed, 75 insertions(+), 40 deletions(-) diff --git a/src/location/declarativemaps/qdeclarativegeomap.cpp b/src/location/declarativemaps/qdeclarativegeomap.cpp index 63587efe..106c4dae 100644 --- a/src/location/declarativemaps/qdeclarativegeomap.cpp +++ b/src/location/declarativemaps/qdeclarativegeomap.cpp @@ -234,33 +234,45 @@ QDeclarativeGeoMap::~QDeclarativeGeoMap() m_map->clearMapItems(); } - // This forces the destruction of the associated items now, not when QObject destructor is called, at which point - // QDeclarativeGeoMap is long gone + // Remove the items from the map, making them deletable. + // Go in the same order as in removeMapChild: views, groups, then items if (!m_mapViews.isEmpty()) { - for (QDeclarativeGeoMapItemView *v : qAsConst(m_mapViews)) { + const auto mapViews = m_mapViews; + for (QDeclarativeGeoMapItemView *v : mapViews) { // so that removeMapItemView_real can safely modify m_mapViews; if (!v) continue; - if (v->parent() == this) { - delete v; - } else { - // FIXME: removeInstantiatedItems should abort, as well as exit transitions terminated - v->removeInstantiatedItems(false); - v->m_map = nullptr; - } + + QQuickItem *parent = v->parentItem(); + QDeclarativeGeoMapItemGroup *group = qobject_cast(parent); + if (group) + continue; // Ignore non-top-level MIVs. They will be recursively processed. + // Identify them as being parented by a MapItemGroup. + + removeMapItemView_real(v); } } - // remove any map items associations - for (int i = 0; i < m_mapItems.count(); ++i) { - if (m_mapItems.at(i)) - m_mapItems.at(i).data()->setMap(0,0); - } - // remove any map item groups associations - for (int i = 0; i < m_mapItemGroups.count(); ++i) { - if (m_mapItemGroups.at(i)) - m_mapItemGroups.at(i).data()->setQuickMap(nullptr); + if (!m_mapItemGroups.isEmpty()) { + const auto mapGroups = m_mapItemGroups; + for (QDeclarativeGeoMapItemGroup *g : mapGroups) { + if (!g) + continue; + + QQuickItem *parent =g->parentItem(); + QDeclarativeGeoMapItemGroup *group = qobject_cast(parent); + if (group) + continue; // Ignore non-top-level Groups. They will be recursively processed. + // Identify them as being parented by a MapItemGroup. + + removeMapItemGroup_real(g); + } } + // remove any remaining map items associations + const auto mapItems = m_mapItems; + for (auto mi: mapItems) + removeMapItem_real(mi.data()); + if (m_copyrights.data()) delete m_copyrights.data(); m_copyrights.clear(); @@ -2094,14 +2106,10 @@ bool QDeclarativeGeoMap::removeMapItemView_real(QDeclarativeGeoMapItemView *item if (!itemView || itemView->m_map != this) // can't remove a view that is already added to another map return false; - // Leaving this as void since the removal is async (potentially transitioned) - // && the delegates *could* be empty mapItemGroups. - itemView->removeInstantiatedItems(); + itemView->removeInstantiatedItems(false); // remove the items without using transitions AND abort ongoing ones itemView->m_map = 0; - // it can be removed from the list at this point, since no operations that require a Map have to be done - // anymore on destruction. m_mapViews.removeOne(itemView); - return removeMapItemGroup_real(itemView); // at this point, delegate instances have been removed. + return removeMapItemGroup_real(itemView); // at this point, all delegate instances have been removed. } /*! diff --git a/src/location/declarativemaps/qdeclarativegeomapitemview.cpp b/src/location/declarativemaps/qdeclarativegeomapitemview.cpp index d404fd47..90736d23 100644 --- a/src/location/declarativemaps/qdeclarativegeomapitemview.cpp +++ b/src/location/declarativemaps/qdeclarativegeomapitemview.cpp @@ -107,7 +107,8 @@ QDeclarativeGeoMapItemView::QDeclarativeGeoMapItemView(QQuickItem *parent) QDeclarativeGeoMapItemView::~QDeclarativeGeoMapItemView() { - removeInstantiatedItems(); + // No need to remove instantiated items: if the MIV has instantiated items because it has been added + // to a Map (or is child of a Map), the Map destructor takes care of removing it and the instantiated items. } /*! @@ -309,7 +310,7 @@ void QDeclarativeGeoMapItemView::removeInstantiatedItems(bool transition) if (!m_map) return; - // FIXME: removeInstantiatedItems should abort, as well as exit transitions terminated QTBUG-69195 + // with transition = false removeInstantiatedItems aborts ongoing exit transitions //QTBUG-69195 // Backward as removeItemFromMap modifies m_instantiatedItems for (int i = m_instantiatedItems.size() -1; i >= 0 ; i--) removeDelegateFromMap(i, transition); @@ -356,24 +357,38 @@ QList QDeclarativeGeoMapItemView::mapItems() return m_instantiatedItems; } +QQmlInstanceModel::ReleaseFlags QDeclarativeGeoMapItemView::disposeDelegate(QQuickItem *item) +{ + disconnect(item, 0, this, 0); + removeDelegateFromMap(item); + item->setParentItem(nullptr); // Needed because + item->setParent(nullptr); // m_delegateModel->release(item) does not destroy the item most of the times!! + QQmlInstanceModel::ReleaseFlags releaseStatus = m_delegateModel->release(item); + return releaseStatus; +} + void QDeclarativeGeoMapItemView::removeDelegateFromMap(int index, bool transition) { if (index >= 0 && index < m_instantiatedItems.size()) { QQuickItem *item = m_instantiatedItems.takeAt(index); if (!item) { // not yet incubated - // Don't cancel incubation explicitly, as DelegateModel apparently takes care of incubating elements when the model - // remove those indices. + // Don't cancel incubation explicitly when model rows are removed, as DelegateModel + // apparently takes care of incubating elements when the model remove those indices. + // Cancel them explicitly only when a MIV is removed from a map. + if (!transition) + m_delegateModel->cancel(index); return; } // item can be either a QDeclarativeGeoMapItemBase or a QDeclarativeGeoMapItemGroup (subclass) - if (m_exit && m_map && transition) { + if (m_exit && m_map && transition) { transitionItemOut(item); } else { - disconnect(item, 0, this, 0); - removeDelegateFromMap(item); - item->setParentItem(nullptr); // Needed because - item->setParent(nullptr); // m_delegateModel->release(item) does not destroy the item most of the times!! - QQmlInstanceModel::ReleaseFlags releaseStatus = m_delegateModel->release(item); + if (m_exit && m_map && !transition) { + // check if the exit transition is still running, if so stop it. + // This can happen when explicitly calling Map.removeMapItemView, soon after adding it. + terminateExitTransition(item); + } + QQmlInstanceModel::ReleaseFlags releaseStatus = disposeDelegate(item); #ifdef QT_DEBUG if (releaseStatus == QQmlInstanceModel::Referenced) qWarning() << "item "<< index << "(" << item << ") still referenced"; @@ -436,16 +451,26 @@ void QDeclarativeGeoMapItemView::transitionItemOut(QQuickItem *o) } } +void QDeclarativeGeoMapItemView::terminateExitTransition(QQuickItem *o) +{ + QDeclarativeGeoMapItemGroup *group = qobject_cast(o); + if (group && group->m_transitionManager) { + group->m_transitionManager->cancel(); + return; + } + QDeclarativeGeoMapItemBase *item = qobject_cast(o); + if (item && item->m_transitionManager) { + item->m_transitionManager->cancel(); + return; + } +} + void QDeclarativeGeoMapItemView::exitTransitionFinished() { QQuickItem *item = qobject_cast(sender()); if (!item) return; - disconnect(item, 0, this, 0); - removeDelegateFromMap(item); - item->setParentItem(nullptr); - item->setParent(nullptr); - QQmlInstanceModel::ReleaseFlags releaseStatus = m_delegateModel->release(item); + QQmlInstanceModel::ReleaseFlags releaseStatus = disposeDelegate(item); #ifdef QT_DEBUG if (releaseStatus == QQmlInstanceModel::Referenced) qWarning() << "item "<