On Fri, 12 May 2023 08:13:01 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> one more test case > > 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. Even though I don't particularly like register*Listener() because of its asymmetric nature (when it comes to removing listeners), here we do need to create weak listeners that get unregistered upon `dispose()`. We need weak listeners because TreeTableView does not explicitly "disconnect" unused cells (e.g. refresh()), and we need `dispose()` due to skin life cycle. So, in this particular case, I think this change should be ok. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1192542423
