jung icon indicating copy to clipboard operation
jung copied to clipboard

TreeLayout neither implements reset(), nor calls it when setting a new graph

Open ddenton opened this issue 7 years ago • 1 comments

TreeLayout requires a non-null graph as an argument to its constructor. Thus the setGraph() method can only be intended for changing the graph post initialization. This clearly entails cleaning out the positions of the nodes from the old graph.

I propose adding the following implementation of reset(), and then calling reset() instead of buildTree() in the setGraph() method:

public void reset() {
    basePositions.clear();
    locations.invalidateAll();
    alreadyDone.clear();
    buildTree();
}

ddenton avatar Apr 04 '17 22:04 ddenton

Note: I think that we probably still have some issues with this in the post-visualization-system refactoring world (which, to be fair, is still in a transitional state): while changing the graph for a LayoutModel does cause the change support to fire off an event, I don't think that the locations are invalidated as a result. The way that LoadingCacheLayoutModel works, I believe that any nodes that might be in common between the two Graph objects will retain the same positions, which is not necessarily what the user would want.

Stepping back a moment, we should give some thought to whether we even want setGraph() at all.

jrtom avatar Jan 22 '19 04:01 jrtom