On Fri, 12 Jun 2026 21:17:15 GMT, Michael Strauß <[email protected]> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
>> line 147:
>>
>>> 145: public void dispose() {
>>> 146: if (currentTransitions != null) {
>>> 147: currentTransitions.forEach((_, value) -> value.stop());
>>
>> We should `clear()` the `currentTransitions` here
>
> This shouldn't be necessary, since the object is being disposed here. After
> that, it is no longer usable and will be eligible for GC. Nulling out fields
> really only makes sense when the object is _not_ disposed, when only the
> interior object should be GC'd, but not the enclosing object.
I agree, but my thought behind was not the GC, but rather this:
We can change just the skin at runtime.
In this case we would still have stale references to the `disclosureNode` here
in the `Map`.
And the `disclosureNode` can still exist after we changed the skin, as the
`TableRow` is unchanged (and the `disclosureNode` comes from it). Then this
skin might not be GC'ed at all.
Also it is allowed to re-use a skin. So e.g. we could replace the skin of
`TableRow` with another one, and then back to the previous one.
I don't think this will work right now because everything is done in the
constructor (and not in `install()`), but that might be possible in the future.
I agree both are rare situation and something you most likely don't do.
But since it does not hurt to clear/null the fields here, and we are improving
the skin in case it is not possible to dispose (GC) it immediately, I think we
should rather do it and are safe for whatever a developer might do.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2177#discussion_r3408170678
