fix TreeLayoutAlgorithm to not try to use pointModel before visit call #159
Conversation
Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/TreeLayoutAlgorithm.java
| } | ||
|
|
||
| public void visit(LayoutModel<N, P> layoutModel) { | ||
| super.visit(layoutModel); | ||
| this.m_currentPoint = pointModel.newPoint(0, 0); |
There was a problem hiding this comment.
Unless I'm missing something, this assignment never has any effect, because it's immediately overwritten inside the buildTree() call, so we can just remove it.
Pushing this even farther: it's not at all clear to me that we even want a currentPoint instance field. Seems like it would be better to make the following changes:
(1) change the buildTree overload with three arguments so that the last argument is the current point (not just its X coordinate)
(2) add a P argument to setCurrentPositionFor() and pass the current point to it
That way we don't need this piece of class-level state at all, which seems like a better/more robust design.
There was a problem hiding this comment.
Ah yes, TreeLayout. It was one of our contributed Layouts and has always been a little odd. There's probably a ticket out there somewhere to completely redesign it.
Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/TreeLayoutAlgorithm.java
|
All uses of TreeLayoutAlgorithm (several demos) are broken until this is merged. There are no unit tests for TreeLayoutAlgorithm. If there were, they would be broken too. |
|
Yes, I'm aware that this was broken; I spent as much time as I could on JUNG stuff yesterday. Feel free to add a minimal test for TreeLayout that would have caught the original bug. |
Changes to be committed:
modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/TreeLayoutAlgorithm.java