On Wed, 6 Jan 2021 12:22:17 GMT, Ambarish Rapte <[email protected]> wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java >> line 289: >> >>> 287: assertNull(tabPane.getSkin()); >>> 288: assertEquals(0, tabPane.getChildrenUnmodifiable().size()); >>> 289: } >> >> Same as above: what's the reason to actually show? >> >> And just curious: why is this passing already before the fix? > > Corrected to use installDefaultSkin(). > It works without this fix is because the children are > cleared([here](https://github.com/openjdk/jfx/blob/e74f679fda6f03ee8449836147815fdaafb5d626/modules/javafx.controls/src/main/java/javafx/scene/control/Control.java#L300)) > in Control class when the skin is set to null. thanks for the info :) >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java >> line 1577: >> >>> 1575: listener.dispose(); >>> 1576: inner.getChildren().clear(); >>> 1577: getChildren().clear(); >> >> I'm aware this is internal and old api but .. could we consider cleaning it >> up a bit? >> >> - its task is to cleanup itself, doing whatever it deems is needed (here: >> removing listeners and children), so removeListeners is a misnomer as it >> describes only part of it. A (well-established) name for the task would be >> dispose >> - passing in the tab is .. strange, as can be seen in its usage in >> TabHeaderArea.dispose: `header.removeListeners(header.getTab())` Actually, >> it would be an error to call the method with a `tab != header.getTab()` > > +1, passing `tab` is unnecessary here. I have removed the parameter and the > method is now renamed as `dispose()`. > Also it is not required to clear the children of `TabHeaderSkin`, so have > removed > `inner.getChildren().clear();` and `getChildren().clear();` calls from this > method. good catch :) ------------- PR: https://git.openjdk.java.net/jfx/pull/318
