On Sun, 14 Jun 2026 16:27:33 GMT, Marius Hanl <[email protected]> wrote:
> Just to make it more clear what exactly I was worried about, this situation: > > ``` > treeTableRow.setDisclosureNode(node) > └── TreeTableRowSkin picks it up > └── stores in Map<Node, FadeTransition> > > treeTableRow.setSkin(new TreeTableRowSkin(treeTableRow)) > └── Old Skin disposed, new TreeTableRowSkin picks it up > └── stores in Map<Node, FadeTransition> as well > > Old skin has the reference and new one has. > ``` Okay, but unless I misunderstand something really fundamental here, there shouldn't be a problem if the old skin is not retained. It will simply be collected, along with the `Map` which contains the strong reference. There _would_ be a problem if the old skin were retained, but then again, retaining an object after disposal is not proper use. Disposal must always end the logical lifetime of the object (it's in the name of the operation), and after an object has reached the end of its logical lifetime, actively retaining it in memory is just a leak. I think you're arguing that it's safer (for some definition of safety) to prevent an accidentally retained object from keeping other objects alive. While I don't think that this is an entirely unreasonable argument, I think it encourages a coding style that borders on wishful thinking; often accompanied by saying "it can't hurt"... sure, but that shouldn't be the criterion on which we judge code. It's a bit like trying to "help" the GC, which almost always is not what we should be doing. > I did run some tests and could not find a problem. Still was a bit worried as > it was not always clear how to correctly write the skin so it follows the > best practices. I think in the past the people probably chose to rather > clear/null everything to be safe. Or, I saw a comment somewhere IRC that this > was done to 'help' the GC. So yeah, a bit out of place and I personally then > just followed the `null everything` pattern as I saw it quite often. > > Example in the super class of `TableRowSkinBase`: > > https://github.com/openjdk/jfx/blob/2abb9a3b0a7e1071bea0b8bf2ca3d83e6fdf25f8/modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java#L246-L252 > > > No, it's not allowed to reuse a skin > > Yes (although this can be achieved when extending from `Skin` and using the > `install` method correctly - too much work). Just brought it up so we think > about everything. But does not affect us in any way. I'm not sure whether we're on the same page here. There's no possibility to reuse a `SkinBase`-based skin other than to implement it incorrectly. The `SkinBase.dispose()` method nulls out the control reference, so reusing a disposed skin will _always_ fail with a NPE, regardless of how the `install()` method was implemented: var skin = button.getSkin(); button.setSkin(new ButtonSkin(button)); button.setSkin(skin); // <-- NPE: "Skin does not correspond to this Control" The only way to have code like this not instantly fail is by overriding `SkinBase.dispose()` and not calling the base implementation (which would not be a correct implementation of the disposal pattern). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2177#discussion_r3410148953
