deleteLater vs removeItem on a QGraphicsObject

Question

Although I'm using Qt from Python via PyQt, this question is equally applicable to pure Qt, just the syntax is a bit different, the issue is the same:

When we want to dispose of a QGraphicsItem object in our scene, we call scene.removeItem(item). When we want to dispose of a QGraphicsObject object in our scene, we call scene.removeItem(item) because it derives from QGraphicsItem, but we ALSO call item.deleteLater() because it derives from QObject and that is the recommended way of disposing of QObjects (so that pending signals to and from the item are properly handled).

PROBLEM is that slots in the object item may can get called AFTER the item has been removed from the scene, due to how deleteLater() functions. This requires that we test for self.scene() being None in slots. But this is error prone as it is easy to forget to do that, and forgetting this leads to exception if slot is called.

Another approach is to not call deleteLater() before removing the item from the scene, but this requires manually disconnecting the item from other objects. This has similar disadvantage to testing for self.scene() being None in slots, and its easy to forgot to disconnect a slot.

A better way of mitigating this source of error (if there are no hidden gotchas) would be to NOT call scene.removeItem(item) when item is a QGraphicsObject, and JUST call its deleteLater(): it seems, based on some simple tests, that the scene automatically removes item from its list when it eventually gets destroyed. HOWEVER, I can't find any Qt documentation that states this, and I might have just been lucky; perhaps in a more realistic scenario I would get a memory leak or a crash.

So I'm leaning towards calling deleteLater() without calling removeItem() when item is a QGraphicsObject, do you think this is safe?

Answer 1

Below is the source code for the QGraphicsItem destructor (taken from qt-5.7/qtbase/src/widgets/graphicsview/qgraphicsitem.cpp). As you can see, it does a whole load of cleanup, as well as calling the scene's internal removeItemHelper function (which is also called by removeItem). Thus, it seems well designed to handle removal via deletion.

QGraphicsItem::~QGraphicsItem()
{
    if (d_ptr->isObject) {
        QGraphicsObject *o = static_cast<QGraphicsObject *>(this);
        QObjectPrivate *p = QObjectPrivate::get(o);
        p->wasDeleted = true;
        if (p->declarativeData) {
            if (static_cast<QAbstractDeclarativeDataImpl*>(p->declarativeData)->ownedByQml1) {
                if (QAbstractDeclarativeData::destroyed_qml1)
                    QAbstractDeclarativeData::destroyed_qml1(p->declarativeData, o);
            } else {
                if (QAbstractDeclarativeData::destroyed)
                    QAbstractDeclarativeData::destroyed(p->declarativeData, o);
            }
            p->declarativeData = 0;
        }
    }

    d_ptr->inDestructor = 1;
    d_ptr->removeExtraItemCache();

#ifndef QT_NO_GESTURES
    if (d_ptr->isObject && !d_ptr->gestureContext.isEmpty()) {
        QGraphicsObject *o = static_cast<QGraphicsObject *>(this);
        if (QGestureManager *manager = QGestureManager::instance()) {
            const auto types  = d_ptr->gestureContext.keys(); // FIXME: iterate over the map directly?
            for (Qt::GestureType type : types)
                manager->cleanupCachedGestures(o, type);
        }
    }
#endif

    clearFocus();
    setFocusProxy(0);

    // Update focus scope item ptr.
    QGraphicsItem *p = d_ptr->parent;
    while (p) {
        if (p->flags() & ItemIsFocusScope) {
            if (p->d_ptr->focusScopeItem == this)
                p->d_ptr->focusScopeItem = 0;
            break;
        }
        p = p->d_ptr->parent;
    }

    if (!d_ptr->children.isEmpty()) {
        while (!d_ptr->children.isEmpty())
            delete d_ptr->children.first();
        Q_ASSERT(d_ptr->children.isEmpty());
    }

    if (d_ptr->scene) {
        d_ptr->scene->d_func()->removeItemHelper(this);
    } else {
        d_ptr->resetFocusProxy();
        setParentItem(0);
    }

#ifndef QT_NO_GRAPHICSEFFECT
    delete d_ptr->graphicsEffect;
#endif //QT_NO_GRAPHICSEFFECT
    if (d_ptr->transformData) {
        for(int i = 0; i < d_ptr->transformData->graphicsTransforms.size(); ++i) {
            QGraphicsTransform *t = d_ptr->transformData->graphicsTransforms.at(i);
            static_cast<QGraphicsTransformPrivate *>(t->d_ptr.data())->item = 0;
            delete t;
        }
    }
    delete d_ptr->transformData;

    if (QGraphicsItemCustomDataStore *dataStore = qt_dataStore())
        dataStore->data.remove(this);
}

Answer 2

Another approach is to not call deleteLater() before removing the item from the scene, but this requires manually disconnecting the item from other objects. This has similar disadvantage to testing for self.scene() being None in slots, and its easy to forgot to disconnect a slot.

First of all, there's no reason to manually remove an item from the scene if your goal is to destroy the item. The scene tracks the item lifetime. So all you need to do is to destroy the item by appropriate means.

If none of the item's methods are on the call stack, simply delete item.

If the item's methods may be on the call stack, use the QObject::deleteLater method.

Qt's classes are mostly well designed and thus follow the Liskov Substitution Principle. The QGraphicsObject is-substitutable-for-a QObject and you can treat it as if it was, indeed, a QObject, without worrying that it happens to be something a QGraphicsItem too.

That's all there's to it. It will solve all your problems in one go.

You almost never have to call scene.removeItem directly: manage the lifetime of the items, and the scene will follow it for you. It's just like the interaction between QWidget and QLayout: widgets that are managed by layouts are still destructible and the layout will forget about the widget when the widget gets destroyed.