Deleting multiple nodes and connections crashes

[spiriapbergeron]

FlowView::deleteSelectedNodes() crashes when multiple nodes and multiple connections are currently selected.

eg: In the calculator example, do a big graph with many adds, then try to delete the middle portion of the graph. boom.

The issue is that "_scene->selectedItems()" will return multiple elements, among them nodes and connections... If we first delete the nodes (eg: _scene->removeNode()) then the call to removeNode will also remove attached connections.

Then when we do a next iteration, the call to qgraphiucsitem_cast<> will crash because the item (a connection) will have already been deleted.

The fix is simple: delete all connections first, then delete the nodes (eg: change the order of the loops).

The correct implementation should be this:

void FlowView:: deleteSelectedNodes() { // Delete connections first because _scene->removeNode() will delete connections that are in the selectedItems() list for (QGraphicsItem * item : _scene->selectedItems()) { if (auto c = qgraphicsitem_cast<ConnectionGraphicsObject*>(item)) { _scene->deleteConnection(c->connection()); } }

// Now delete the disconnected nodes and clean up any remaining connections attached to those nodes.
for (QGraphicsItem * item : _scene->selectedItems())
{
    if (auto n = qgraphicsitem_cast<NodeGraphicsObject*>(item))
    {
        _scene->removeNode(n->node());
    }
}

}

[Daguerreo]

@spiriapbergeron I cannot replicate the issue, could you add a scene like you described?

[spiriapbergeron]

Ugh. It's been quite a while. I have a heavily modified version here locally, so it's not easy to "send a scene" since I have also changed the persistence scheme on my end.

But, let me try to be clearer in my explanation:

• Using the calculator example, create a "big graph", where you add together a bunch of nodes like this,

Create Number nodes A,B,C,D,E,F.

Create 2 add nodes:

#1 (A+B) #2 (C+D)

Create 2 more add nodes

#3 (A+B) + E #4 (C+D) + F

create 1 more add node

#5 ((A+B)+E) + ((C+D)+F)

Output node #5 into Result node.

Now do rectangle select of everything, including connections, EXCEPT for the number nodes ABCD, and Result node.

Hit delete

You should crash

[Daguerreo]

I just tried and it works properly again

[spiriapbergeron]

You mean you can reproduce and you fixed it, or you can't reproduce?

I might try to do video recording of crash and the problem in a bit if I have time. Where would I upload the video?

[Daguerreo]

Sorry, I mean I can't reproduce it. Are you sure it not depends from your locally modified version?