On Thu, 11 May 2023 22:38:58 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Fixed a memory leak in TreeTableView by reverting to register**Listener >> (which is ok in this particular situation) - the leak is specific to >> TreeTableRowSkin. >> >> Added a unit test. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > one more test case I confirmed the tests fail before the changes in `setupTreeTableViewListeners` and pass with those changes. Removing the constructor and dispose changes did not fail any tests. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 103: > 101: updateTreeItem(); > 102: // There used to be an isDirty = true statement here, but > this was > 103: // determined to be unnecessary and led to performance > issues such as These changes don't fail the test when I undo them. As said before, they're unnecessary. Reasoning: they register on `TreeTableRow`, which is associated with the skin directly. If their lifecycles don't match, then `dispose` will take care of unregistering. If their lifecycles do match, then they go out of scope at the same time. Unless you prefer using the `register` functions, I think this change should be undone. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 161: > 159: fixedCellSize = p.get(); > 160: fixedCellSizeEnabled = fixedCellSize > 0; > 161: } The `null` check on the property is not needed, properties never return `null`. I would suggest just calling the getter: `t.getFixedCellSize()`. ------------- Changes requested by jhendrikx (Committer). PR Review: https://git.openjdk.org/jfx/pull/1129#pullrequestreview-1423999964 PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1192054924 PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1192057917
