Remove duplicated code for removing an item from the scene.

Before we had almost two identical functions for removing an item from
the scene. There was only minor differences depending on whether the
item was removed from QGraphicsScene::removeItem or from the item's
destructor. Now we have one function that handles both cases just fine.

Reviewed-by: Andreas

5 files changed, 101 insertions, 140 deletions

diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp
index 5ba87ee..db16213 100644
--- a/src/gui/graphicsview/qgraphicsitem.cpp
+++ b/src/gui/graphicsview/qgraphicsitem.cpp
@@ -840,11 +840,11 @@ QVariant QGraphicsItemPrivate::inputMethodQueryHelper(Qt::InputMethodQuery query
 /*!
     \internal
 
-    If \a deleting is true, then this item is being deleted, and \a parent is
-    null. Make sure not to trigger any pure virtual function calls (e.g.,
-    prepareGeometryChange).
+    Make sure not to trigger any pure virtual function calls (e.g.,
+    prepareGeometryChange) if the item is in its destructor, i.e.
+    inDestructor is 1.
 */
-void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, bool deleting)
+void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent)
 {
     Q_Q(QGraphicsItem);
     if (newParent == q) {
@@ -871,7 +871,7 @@ void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, bool de
     // We anticipate geometry changes. If the item is deleted, it will be
     // removed from the index at a later stage, and the whole scene will be
     // updated.
-    if (!deleting)
+    if (!inDestructor)
         q_ptr->prepareGeometryChange();
 
     const QVariant thisPointerVariant(qVariantFromValue<QGraphicsItem *>(q));
@@ -883,7 +883,7 @@ void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, bool de
 
     // Update toplevelitem list. If this item is being deleted, its parent
     // will be 0 but we don't want to register/unregister it in the TLI list.
-    if (scene && !deleting) {
+    if (scene && !inDestructor) {
         if (parent && !newParent) {
             scene->d_func()->registerTopLevelItem(q);
         } else if (!parent && newParent) {
@@ -931,7 +931,7 @@ void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, bool de
         updateAncestorFlag(QGraphicsItem::ItemClipsChildrenToShape);
         updateAncestorFlag(QGraphicsItem::ItemIgnoresTransformations);
 
-        if (!deleting) {
+        if (!inDestructor) {
             // Update item visible / enabled.
             if (!visible && !explicitlyHidden)
                 setVisibleHelper(true, /* explicit = */ false);
@@ -1115,22 +1115,22 @@ QGraphicsItem::QGraphicsItem(QGraphicsItemPrivate &dd, QGraphicsItem *parent,
 */
 QGraphicsItem::~QGraphicsItem()
 {
-    if (d_ptr->scene && !d_ptr->parent)
-        d_ptr->scene->d_func()->unregisterTopLevelItem(this);
+    d_ptr->inDestructor = 1;
+    d_ptr->removeExtraItemCache();
 
     clearFocus();
+    if (!d_ptr->children.isEmpty()) {
+        QList<QGraphicsItem *> oldChildren = d_ptr->children;
+        qDeleteAll(oldChildren);
+        Q_ASSERT(d_ptr->children.isEmpty());
+    }
 
-    d_ptr->removeExtraItemCache();
-    QList<QGraphicsItem *> oldChildren = d_ptr->children;
-    qDeleteAll(oldChildren);
-    Q_ASSERT(d_ptr->children.isEmpty());
-
-    d_ptr->setParentItemHelper(0, /* deleting = */ true);
     if (d_ptr->scene)
-        d_ptr->scene->d_func()->_q_removeItemLater(this);
+        d_ptr->scene->d_func()->removeItemHelper(this);
+    else
+        d_ptr->setParentItemHelper(0);
 
     delete d_ptr->transformData;
-
     delete d_ptr;
 
     qt_dataStore()->data.remove(this);
@@ -1272,7 +1272,7 @@ QGraphicsWidget *QGraphicsItem::window() const
 */
 void QGraphicsItem::setParentItem(QGraphicsItem *parent)
 {
-    d_ptr->setParentItemHelper(parent, /* deleting = */ false);
+    d_ptr->setParentItemHelper(parent);
 }
 
 /*!
diff --git a/src/gui/graphicsview/qgraphicsitem_p.h b/src/gui/graphicsview/qgraphicsitem_p.h
index e2a37a1..c502655 100644
--- a/src/gui/graphicsview/qgraphicsitem_p.h
+++ b/src/gui/graphicsview/qgraphicsitem_p.h
@@ -148,6 +148,7 @@ public:
         paintedViewBoundingRectsNeedRepaint(0),
         dirtySceneTransform(1),
         geometryChanged(0),
+        inDestructor(0),
         globalStackingOrder(-1),
         q_ptr(0)
     {
@@ -183,7 +184,7 @@ public:
     void resolveDepth(int parentDepth);
     void addChild(QGraphicsItem *child);
     void removeChild(QGraphicsItem *child);
-    void setParentItemHelper(QGraphicsItem *parent, bool deleting);
+    void setParentItemHelper(QGraphicsItem *parent);
     void childrenBoundingRectHelper(QTransform *x, QRectF *rect);
     void initStyleOption(QStyleOptionGraphicsItem *option, const QTransform &worldTransform,
                          const QRegion &exposedRegion, bool allItems = false) const;
@@ -403,7 +404,8 @@ public:
     quint32 paintedViewBoundingRectsNeedRepaint : 1;
     quint32 dirtySceneTransform  : 1;
     quint32 geometryChanged : 1;
-    quint32 unused : 16; // feel free to use
+    quint32 inDestructor : 1;
+    quint32 unused : 15; // feel free to use
 
     // Optional stacking order
     int globalStackingOrder;
diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp
index e43c9cd..b63efd6 100644
--- a/src/gui/graphicsview/qgraphicsscene.cpp
+++ b/src/gui/graphicsview/qgraphicsscene.cpp
@@ -710,39 +710,65 @@ void QGraphicsScenePrivate::_q_processDirtyItems()
     \internal
 
     Schedules an item for removal. This function leaves some stale indexes
-    around in the BSP tree; these will be cleaned up the next time someone
-    triggers purgeRemovedItems().
+    around in the BSP tree if called from the item's destructor; these will
+    be cleaned up the next time someone triggers purgeRemovedItems().
 
-    Note: This function is called from QGraphicsItem's destructor. \a item is
+    Note: This function might get called from QGraphicsItem's destructor. \a item is
     being destroyed, so we cannot call any pure virtual functions on it (such
     as boundingRect()). Also, it is unnecessary to update the item's own state
     in any way.
-
-    ### Refactoring: This function shares much functionality with removeItem()
 */
-void QGraphicsScenePrivate::_q_removeItemLater(QGraphicsItem *item)
+void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item)
 {
     Q_Q(QGraphicsScene);
 
-    // Clear focus on the item to remove any reference in the focusWidget
-    // chain.
+    // Clear focus on the item to remove any reference in the focusWidget chain.
     item->clearFocus();
-
-    int index = item->d_func()->index;
-    if (index != -1) {
-        // Important: The index is useless until purgeRemovedItems() is
-        // called.
-        indexedItems[index] = (QGraphicsItem *)0;
-        if (!purgePending) {
+    markDirty(item, QRectF(), false, false, false, false, /*removingItemFromScene=*/true);
+
+    if (!item->d_ptr->inDestructor) {
+        // Can potentially call item->boundingRect() (virtual function), that's why
+        // we only can call this function if the item is not in its destructor.
+        removeFromIndex(item);
+    } else if (item->d_ptr->index != -1) {
+        // Important: The index is useless until purgeRemovedItems() is called.
+        indexedItems[item->d_ptr->index] = (QGraphicsItem *)0;
+        if (!purgePending)
             purgePending = true;
-            q->update();
-        }
         removedItems << item;
     } else {
         // Recently added items are purged immediately. unindexedItems() never
         // contains stale items.
         unindexedItems.removeAll(item);
-        q->update();
+    }
+
+    if (!item->d_ptr->inDestructor && item == tabFocusFirst) {
+        QGraphicsWidget *widget = static_cast<QGraphicsWidget *>(item);
+        widget->d_func()->fixFocusChainBeforeReparenting(0, 0);
+    }
+
+    item->d_func()->scene = 0;
+
+    // Remove from parent, or unregister from toplevels.
+    if (QGraphicsItem *parentItem = item->parentItem()) {
+        if (parentItem->scene()) {
+            Q_ASSERT_X(parentItem->scene() == q, "QGraphicsScene::removeItem",
+                       "Parent item's scene is different from this item's scene");
+            item->d_ptr->setParentItemHelper(0);
+        }
+    } else {
+        unregisterTopLevelItem(item);
+    }
+
+    if (!item->d_ptr->inDestructor) {
+        // Remove from our item lists.
+        int index = item->d_func()->index;
+        if (index != -1) {
+            freeItemIndexes << index;
+            indexedItems[index] = 0;
+        } else {
+            unindexedItems.removeAll(item);
+        }
     }
 
     // Reset the mouse grabber and focus item data.
@@ -776,13 +802,19 @@ void QGraphicsScenePrivate::_q_removeItemLater(QGraphicsItem *item)
             ++iterator;
     }
 
-    // Reset the mouse grabber
+    if (!item->d_ptr->inDestructor) {
+        // Remove all children recursively
+        for (int i = 0; i < item->d_ptr->children.size(); ++i)
+            q->removeItem(item->d_ptr->children.at(i));
+    }
+
+    // Reset the mouse grabber and focus item data.
     if (mouseGrabberItems.contains(item))
-        ungrabMouse(item, /* item is dying */ true);
+        ungrabMouse(item, /* item is dying */ item->d_ptr->inDestructor);
 
     // Reset the keyboard grabber
     if (keyboardGrabberItems.contains(item))
-        ungrabKeyboard(item, /* item is dying */ true);
+        ungrabKeyboard(item, /* item is dying */ item->d_ptr->inDestructor);
 
     // Reset the last mouse grabber item
     if (item == lastMouseGrabberItem)
@@ -801,8 +833,6 @@ void QGraphicsScenePrivate::_q_removeItemLater(QGraphicsItem *item)
 */
 void QGraphicsScenePrivate::purgeRemovedItems()
 {
-    Q_Q(QGraphicsScene);
-
     if (!purgePending && removedItems.isEmpty())
         return;
 
@@ -818,9 +848,6 @@ void QGraphicsScenePrivate::purgeRemovedItems()
             freeItemIndexes << i;
     }
     purgePending = false;
-
-    // No locality info for the items; update the whole scene.
-    q->update();
 }
 
 /*!
@@ -3352,95 +3379,7 @@ void QGraphicsScene::removeItem(QGraphicsItem *item)
         return;
     }
 
-    // If the item has focus, remove it (and any focusWidget reference).
-    item->clearFocus();
-
-    // Clear its background
-    item->update();
-
-    // Note: This will access item's sceneBoundingRect(), which (as this is
-    // C++) is why we cannot call removeItem() from QGraphicsItem's
-    // destructor.
-    d->removeFromIndex(item);
-
-    if (item == d->tabFocusFirst) {
-        QGraphicsWidget *widget = static_cast<QGraphicsWidget *>(item);
-        widget->d_func()->fixFocusChainBeforeReparenting(0, 0);
-    }
-    // Set the item's scene ptr to 0.
-    item->d_func()->scene = 0;
-
-    // Remove from parent, or unregister from toplevels.
-    if (QGraphicsItem *parentItem = item->parentItem()) {
-        if (parentItem->scene()) {
-            Q_ASSERT_X(parentItem->scene() == this, "QGraphicsScene::removeItem",
-                       "Parent item's scene is different from this item's scene");
-            item->setParentItem(0);
-        }
-    } else {
-        d->unregisterTopLevelItem(item);
-    }
-
-    // Remove from our item lists.
-    int index = item->d_func()->index;
-    if (index != -1) {
-        d->freeItemIndexes << index;
-        d->indexedItems[index] = 0;
-    } else {
-        d->unindexedItems.removeAll(item);
-    }
-
-    if (item == d->focusItem)
-        d->focusItem = 0;
-    if (item == d->lastFocusItem)
-        d->lastFocusItem = 0;
-    if (item == d->activeWindow) {
-        // ### deactivate...
-        d->activeWindow = 0;
-    }
-
-    // Disable selectionChanged() for individual items
-    ++d->selectionChanging;
-    int oldSelectedItemsSize = d->selectedItems.size();
-
-    // Update selected & hovered item bookkeeping
-    d->selectedItems.remove(item);
-    d->hoverItems.removeAll(item);
-    d->pendingUpdateItems.removeAll(item);
-    d->cachedItemsUnderMouse.removeAll(item);
-    d->unpolishedItems.removeAll(item);
-    d->resetDirtyItem(item);
-
-    //We remove all references of item from the sceneEventFilter arrays
-    QMultiMap<QGraphicsItem*, QGraphicsItem*>::iterator iterator = d->sceneEventFilters.begin();
-    while (iterator != d->sceneEventFilters.end()) {
-        if (iterator.value() == item || iterator.key() == item)
-            iterator = d->sceneEventFilters.erase(iterator);
-        else
-            ++iterator;
-    }
-
-    // Remove all children recursively
-    foreach (QGraphicsItem *child, item->children())
-        removeItem(child);
-
-    // Reset the mouse grabber and focus item data.
-    if (d->mouseGrabberItems.contains(item))
-        d->ungrabMouse(item);
-
-    // Reset the keyboard grabber
-    if (d->keyboardGrabberItems.contains(item))
-        item->ungrabKeyboard();
-
-    // Reset the last mouse grabber item
-    if (item == d->lastMouseGrabberItem)
-        d->lastMouseGrabberItem = 0;
-
-    // Reenable selectionChanged() for individual items
-    --d->selectionChanging;
-
-    if (!d->selectionChanging && d->selectedItems.size() != oldSelectedItemsSize)
-        emit selectionChanged();
+    d->removeItemHelper(item);
 
     // Deliver post-change notification
     item->itemChange(QGraphicsItem::ItemSceneHasChanged, newSceneVariant);
@@ -5272,7 +5211,8 @@ void QGraphicsScenePrivate::drawSubtreeRecursive(QGraphicsItem *item, QPainter *
 }
 
 void QGraphicsScenePrivate::markDirty(QGraphicsItem *item, const QRectF &rect, bool invalidateChildren,
-                                      bool maybeDirtyClipPath, bool force, bool ignoreOpacity)
+                                      bool maybeDirtyClipPath, bool force, bool ignoreOpacity,
+                                      bool removingItemFromScene)
 {
     Q_ASSERT(item);
     if (updateAll)
@@ -5280,7 +5220,8 @@ void QGraphicsScenePrivate::markDirty(QGraphicsItem *item, const QRectF &rect, b
 
     if (item->d_ptr->discardUpdateRequest(/*ignoreClipping=*/maybeDirtyClipPath,
                                           /*ignoreVisibleBit=*/force,
-                                          /*ignoreDirtyBit=*/false, ignoreOpacity)) {
+                                          /*ignoreDirtyBit=*/removingItemFromScene,
+                                          /*ignoreOpacity=*/ignoreOpacity)) {
         return;
     }
 
@@ -5293,6 +5234,24 @@ void QGraphicsScenePrivate::markDirty(QGraphicsItem *item, const QRectF &rect, b
         processDirtyItemsEmitted = true;
     }
 
+    if (removingItemFromScene) {
+        // Note that this function can be called from the item's destructor, so
+        // do NOT call any virtual functions on it within this block.
+        if ((connectedSignals & changedSignalMask) || views.isEmpty()) {
+            // This block of code is kept for compatibility. Since 4.5, by default
+            // QGraphicsView does not connect the signal and we use the below
+            // method of delivering updates.
+            q_func()->update();
+            return;
+        }
+
+        for (int i = 0; i < views.size(); ++i) {
+            QGraphicsViewPrivate *viewPrivate = views.at(i)->d_func();
+            viewPrivate->updateRect(item->d_ptr->paintedViewBoundingRects.value(viewPrivate->viewport));
+        }
+        return;
+    }
+
     item->d_ptr->dirty = 1;
     if (fullItemUpdate)
         item->d_ptr->fullUpdatePending = 1;
diff --git a/src/gui/graphicsview/qgraphicsscene.h b/src/gui/graphicsview/qgraphicsscene.h
index 4c0f2ec..c4c9f9c 100644
--- a/src/gui/graphicsview/qgraphicsscene.h
+++ b/src/gui/graphicsview/qgraphicsscene.h
@@ -275,7 +275,6 @@ private:
     Q_DISABLE_COPY(QGraphicsScene)
     Q_PRIVATE_SLOT(d_func(), void _q_updateIndex())
     Q_PRIVATE_SLOT(d_func(), void _q_emitUpdated())
-    Q_PRIVATE_SLOT(d_func(), void _q_removeItemLater(QGraphicsItem *item))
     Q_PRIVATE_SLOT(d_func(), void _q_updateLater())
     Q_PRIVATE_SLOT(d_func(), void _q_polishItems())
     Q_PRIVATE_SLOT(d_func(), void _q_updateSortCache())
diff --git a/src/gui/graphicsview/qgraphicsscene_p.h b/src/gui/graphicsview/qgraphicsscene_p.h
index 42c4d8c..11e9b64 100644
--- a/src/gui/graphicsview/qgraphicsscene_p.h
+++ b/src/gui/graphicsview/qgraphicsscene_p.h
@@ -131,7 +131,7 @@ public:
     bool regenerateIndex;
 
     bool purgePending;
-    void _q_removeItemLater(QGraphicsItem *item);
+    void removeItemHelper(QGraphicsItem *item);
     QSet<QGraphicsItem *> removedItems;
     void purgeRemovedItems();
 
@@ -264,7 +264,8 @@ public:
                               QRegion *exposedRegion, QWidget *widget,
                               QList<QGraphicsItem *> *topLevelItems = 0, qreal parentOpacity = qreal(1.0));
     void markDirty(QGraphicsItem *item, const QRectF &rect = QRectF(), bool invalidateChildren = false,
-                   bool maybeDirtyClipPath = false, bool force = false, bool ignoreOpacity = false);
+                   bool maybeDirtyClipPath = false, bool force = false, bool ignoreOpacity = false,
+                   bool removingItemFromScene = false);
     void processDirtyItemsRecursive(QGraphicsItem *item, bool dirtyAncestorContainsChildren = false);
 
     inline void resetDirtyItem(QGraphicsItem *item)