QGraphicsObjects as QAbstractItemModel items: RuntimeError

MoritzWM

Hello,
I feel like I'm doing a simple mistake here, but I just can't figure it out.

I'm sublassing QAbstractItemModel (Navigator) to hold a tree of QGraphicsObjects (NavigatorItem). The QGraphicsObjects are automatically added and removed from the scene whenever they are added/removed from the model. They are not hierarchical in the scene so I can place them all in absolute coordinates, so they hold their parent item as self.parent_item and their children in a list self._children.

When I remove a top-level item (as in a child of the root item) which does not have children, I get the following exception:

RuntimeError: wrapped C/C++ object of type QWidget has been deleted

The exception does not occur for non-top-level items, or items with children. Also, it is somehow linked to the QGraphicsScene: if I comment out the code to remove the object from the scene, no exception is raised.
I've tried to build a minimal example. The error is raised in line 149 in the parent() function of Navigator.

from typing import TYPE_CHECKING, List, Optional, Iterator

from PyQt5 import QtCore, QtWidgets, QtGui
from PyQt5.QtCore import Qt

class NavigatorItem(QtWidgets.QGraphicsObject):
    def __init__(self,
                 name: str,
                 model: Optional['Navigator'] = None,
                 parent_item: Optional['NavigatorItem'] = None):
        super().__init__()
        self.name = name
        self.model = model
        self._children = list()
        self.parent_item = parent_item

    def recurse_children(self, parents_first=True) -> Iterator['NavigatorItem']:
        """Iterate over the children of *parent* recursively (does not yield the item itself)"""
        for child in self._children:
            if parents_first:
                yield child
            yield from child.recurse_children()
            if not parents_first:
                yield child

    def boundingRect(self) -> QtCore.QRectF:
        return self.childrenBoundingRect()

    def paint(self, painter: QtGui.QPainter, option: QtWidgets.QStyleOptionGraphicsItem, widget: QtWidgets.QWidget):
        pass

    def insert_child(self, index: int, item: 'NavigatorItem'):
        if item.parent_item is not None and item in item.parent_item._children:
            item.parent_item._children.remove(item)
        item.parent_item = self
        self._children.insert(index, item)

    def remove_child(self, child: 'NavigatorItem'):
        if child.parent_item is not self:
             raise ValueError('parent of item to remove is not self')
        if child not in self._children:
            raise ValueError('child to remove not in children')
        self._children.remove(child)
        child.parent_item = None
            
    def child_count(self) -> int:
        return len(self._children)

    def child(self, index: int) -> Optional['NavigatorItem']:
        try:
            return self._children[index]
        except IndexError:
            return None

    def row(self) -> int:
        if self.parent_item is None:
            return 0
        return self.parent_item._children.index(self)


class Navigator(QtCore.QAbstractItemModel):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.root_item = NavigatorItem(name='__root__', model=self)
        self._current_nav_item = self.root_item
        self.graphics_scene = QtWidgets.QGraphicsScene(self)

    def item_by_index(self, index=QtCore.QModelIndex()) -> NavigatorItem:
        if not index.isValid():
            return self.root_item
        item = index.internalPointer()
        if item is None:
            return self.root_item
        return item

    def index_by_item(self, item: Optional[NavigatorItem]) -> QtCore.QModelIndex:
        if item is None:
            return QtCore.QModelIndex()
        try:
            return self.createIndex(item.row(), 0, item)
        except ValueError:
            return QtCore.QModelIndex()

    def add_item(self, item: NavigatorItem, index: int = 0):
        item.moveToThread(self.thread())
        if item.parent_item is None:
            item.parent_item = self.root_item
        parent_item = item.parent_item
        parent_index = self.index_by_item(parent_item)
        self.beginInsertRows(parent_index, index, index)
        parent_item.insert_child(index, item)
        self.endInsertRows()
        item.model = self
        self.graphics_scene.addItem(item)
        self.layoutChanged.emit()
        return item

    def remove_item(self, item: NavigatorItem):
        if item is self.root_item:
            raise ValueError(f'Removing root item is not possible!')
        parent = item.parent_item
        assert parent is not None
        self.beginRemoveRows(self.index_by_item(parent), item.row(), item.row())
        for child in item.recurse_children():
            self.graphics_scene.removeItem(child)
        self.graphics_scene.removeItem(item) # commenting this out gets rid of the error
        parent.remove_child(item)
        self.endRemoveRows()
        self.layoutChanged.emit()

    def rowCount(self, parent=QtCore.QModelIndex()):
        if not parent.isValid():
            item = self.root_item
        else:
            item = parent.internalPointer()
        return item.child_count()

    def columnCount(self, parent=QtCore.QModelIndex()):
        return 1

    def data(self, index: QtCore.QModelIndex, role=Qt.ItemDataRole.DisplayRole):
        item = self.item_by_index(index)
        if role == Qt.ItemDataRole.DisplayRole:
            return item.name

    def setData(self, index: QtCore.QModelIndex, value, role: Qt.ItemDataRole = Qt.ItemDataRole.EditRole) -> bool:
        return False

    def index(self, row: int, column: int, parent: QtCore.QModelIndex) -> QtCore.QModelIndex:
        # Return the index for the given row and column
        if parent.isValid() and parent.column() != 0:
            return QtCore.QModelIndex()
        parent_item = self.item_by_index(parent)
        if parent_item is None:
            return QtCore.QModelIndex()
        child_item = parent_item.child(row)
        if child_item is not None:
            return self.createIndex(row, column, child_item)
        return QtCore.QModelIndex()

    def parent(self, index: QtCore.QModelIndex) -> QtCore.QModelIndex:
        if not index.isValid():
            return QtCore.QModelIndex()
        child_item = self.item_by_index(index)
        if child_item is None:
            parent_item = None
        else:
            parent_item = child_item.parent_item # Error occurs here
        if parent_item is None or parent_item is self.root_item:
            return QtCore.QModelIndex()
        return self.createIndex(parent_item.row(), 0, parent_item)

    def flags(self, index):
        return super().flags(index) | Qt.ItemFlag.ItemIsSelectable


class MainWindow(QtWidgets.QMainWindow):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.navigator = Navigator(self)

        # Central widget
        self.graphics_view: QtWidgets.QGraphicsView = QtWidgets.QGraphicsView(self)
        self.graphics_view.setScene(self.navigator.graphics_scene)
        # Navigator
        self.tv_navigator = QtWidgets.QTreeView(self)
        self.tv_navigator.setModel(self.navigator)
        # Buttons
        self.btn_rem = QtWidgets.QPushButton('Remove')
        self.btn_rem.clicked.connect(self.remove) # type: ignore

        # Put widgets together
        wdg = QtWidgets.QWidget()
        self.setCentralWidget(wdg)
        layout = QtWidgets.QGridLayout()
        wdg.setLayout(layout)
        layout.addWidget(self.tv_navigator, 0, 0, 3, 1)
        layout.addWidget(self.graphics_view, 0, 1, 1, 1)
        layout.addWidget(self.btn_rem, 2, 1, 1, 1)

        # Populate model a bit
        for i in range(2):
            item = self.navigator.add_item(NavigatorItem(f'Item {i}', self, None))
            for j in range(2):
                self.navigator.add_item(NavigatorItem(f'Item {i}.{j}', self, item))
        for i in range(2, 4):
            self.navigator.add_item(NavigatorItem(f'Item {i} iwillcrash', self, None))

    @QtCore.pyqtSlot(bool)
    def remove(self, _):
        indices = self.tv_navigator.selectedIndexes()
        for index in indices:
            item = self.navigator.item_by_index(index)
            self.navigator.remove_item(item)

class App(QtWidgets.QApplication):
    def __init__(self, args: List[str]):
        super().__init__([''])
        self.ui: MainWindow = MainWindow()
        self.ui.show()

if __name__ == '__main__':
    app = App([])
    app.exec()

I'm really lost here and would appreciate if someone could point me in the right direction.

JonB

@MoritzWM
Start with:

item.moveToThread(self.thread())

Remove this. If you have threads and you need this all bets are off, it may be a threading issue.

 def add_item(self, item: NavigatorItem, index: int = 0):
    ...

item = self.navigator.add_item(NavigatorItem(f'Item {i}', self, None))
self.navigator.add_item(NavigatorItem(f'Item {i}.{j}', self, item))
self.navigator.add_item(NavigatorItem(f'Item {i} iwillcrash', self, None))

Sort out your parameters to add_item(). How Python lets you get away with this I don't know.

        self._children.remove(child)
        child.parent_item = None

In view of the error message talking about QWidget has been deleted I would reverse the order of these two lines, just in case: finish changing an item (child) before you remove it. Though I'm not sure this is relevant.

Because your NavigatorItem is derived from QObject you can watch it being deleted via something like:

item.destroyed.connect(lambda obj: print(obj.objectName()))

Give your items an objectName() and then you can see if/when they get destroyed before you try to access them.

MoritzWM

Hi @JonB
Thanks for your help!

I forgot to remove the moveToThread, you're right that in the application there are multiple threads. In the "real" application, I'm making sure that add_item is only called from slots within Navigator. However, in this minimal example this shouldn't be an issue, or am I mistaken?

I followed your advice: NavigatorItem now stores its name in objectName and prints its name when it gets destroyed.
I reordered remove_child, it also now sets the model to None:

def remove_child(self, child: 'NavigatorItem'):
        ...
        child.parent_item = None
        child.model = None
        self._children.remove(child)

And add_item:

def add_item(self, name: str, parent_item: Optional[NavigatorItem] = None, index: int = 0):
        item = NavigatorItem(name=name)
        if parent_item is None:
            parent_item = self.root_item
        parent_index = self.index_by_item(parent_item)
        self.beginInsertRows(parent_index, index, index)
        item.parent_item = parent_item
        parent_item.insert_child(index, item)
        item.model = self
        self.endInsertRows()
        self.graphics_scene.addItem(item)
        self.layoutChanged.emit()
        item.destroyed.connect(lambda obj: print(f'Destroyed: {obj.objectName()}'))
        return item

And remove_item with a lot of debug messages:

def remove_item(self, item: NavigatorItem):
        print(f'Remove item: {item.objectName()}')
        if item is self.root_item:
            raise ValueError(f'Removing root item is not possible!')
        parent = item.parent_item
        assert parent is not None
        self.beginRemoveRows(self.index_by_item(parent), item.row(), item.row())
        print('Removing children from scene')
        for child in item.recurse_children():
            self.graphics_scene.removeItem(child)
        print('Removing item from scene')
        self.graphics_scene.removeItem(item)
        print('Removing child')
        parent.remove_child(item)
        print('End remove rows')
        self.endRemoveRows()
        print('Layout changed emit')
        self.layoutChanged.emit()
        print('End of remove_item')

And the calls to add_item accordingly.

Now when I remove a non-top-level item (Item 0.0):

Remove item: Item 0.0
Removing children from scene
Removing item from scene
Removing child
End remove rows
Layout changed emit
End of remove_item
Destroyed: Item 0.0

A top level item with children:

Remove item: Item 0
Removing children from scene
Removing item from scene
Removing child
End remove rows
Layout changed emit
End of remove_item

A top level item without children:

Remove item: Item 3 iwillcrash
Removing children from scene
Removing item from scene
Removing child
End remove rows
Layout changed emit
End of remove_item
Destroyed: Item 3 iwillcrash
Traceback (most recent call last):
  File "minexample.py", line 155, in parent
    parent_item = child_item.parent_item
RuntimeError: wrapped C/C++ object of type NavigatorItem has been deleted
Aborted (core dumped)

This explains why removing top-level item with children and items still in the GraphicsScene do not cause a crash: they never get destroyed. But once the item gets destroyed, I still get the error...

JonB

@MoritzWM
Sorry, I can't figure your code in my head. But if you mean that child_item is a NavigatorItem and you have already seen it destroyed then a line like parent_item = child_item.parent_item is going to dump on child_item.anything.

MoritzWM

@JonB
I'm wondering though what causes the call to model.parent(): the item is removed from the model and the scene, so it should be destroyed. However, something wants to know the parent of the destroyed object and I don't understand where this call is coming from.

MoritzWM

So after some trial and error, I found out: the GraphicsScene doesn't matter, I stripped the example down to basically the Editable Tree Model example in the Qt documentation.
I got it to work by reimplementing QAbstractItemModel's removeRow function, which takes the index of the parent of the item to be removed and a row. Already directly passing the parent item instead of its index causes a crash, no idea why.

jeremy_k

I've given up on custom models for PyQt/PySide use, instead embracing QStandardItemModel. Unless there is significant compute intensive translation required to maintain state with a non-QAbstractItemModel representation, python code is unlikely to be anywhere near as performant. QStandardItemModel has presumably had any major item model defects worked out long ago (I didn't check the bug tracker...)

I don't use the QStandardItem subclassing pattern demonstrated in some of the documentation. That reintroduces the potential for item model interface mistakes, and degraded performance from the python interpreter. Custom roles have been sufficient for any non-standard data that I've wanted to store.