On Tue, 5 Jan 2021 15:42:40 GMT, Jeanette Winzenburg <[email protected]> wrote:
>> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Review update > > 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. > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java > line 1735: > >> 1733: removeListeners(); >> 1734: } >> 1735: > > Wondering why the removal of children down the hierarchy? Shouldn't that be > unnecessary as long as the direct children of the skin/control are and all > listeners of the grandChildren to the skin are removed? Corrected, removal of children of `TabContentRegion` is not needed. (same change is also done for `TabHeaderSkin` as mentioned in previous comment) > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java > line 1846: > >> 1844: >> getSkinnable().sideProperty().removeListener(weakSidePropListener); >> 1845: >> getSkinnable().getTabs().removeListener(weakTabsListenerForPopup); >> 1846: } > > yet another candidate for rename to dispose (see above) Corrected. ------------- PR: https://git.openjdk.java.net/jfx/pull/318
