On Thu, 20 Oct 2022 20:48:07 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Fixes memory leaks as determined by SkinMemoryLeakTest (remove line 171) and > a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.PAGINATION; > > Found another issue: Pagination class does not survive replacing its skin > (all components disappear). > > caused by: > - adding and not removing listeners > - adding and not removing children Nodes > - setting control's properties in the constructor > - incorrectly setting a clip rectangle > > NOTE: the fix will requires both ListenerHelper > [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) and Skin.install() > [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) changes. I looked at this in connection with PR #908 . It generally looks good. I left a few questions. modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 201: > 199: getChildren().addAll(currentStackPane, nextStackPane, > navigation); > 200: > 201: > ListenerHelper.get(this).addInvalidationListener(getSkinnable().maxPageIndicatorCountProperty(), > (o) -> { Minor: In the constructor, I recommend using the `control` argument rather than calling `getSkinnable()`, although this is just a recommendation. Minor: since you make several calls to `ListenerHelper`, a temporary variable might make the code simpler; this is also just a recommendation. modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 209: > 207: }); > 208: > 209: > ListenerHelper.get(this).addChangeListener(getSkinnable().heightProperty(), > true, (ev) -> { Why does `fireImmediately` need to be true? modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 1361: > 1359: // FIX own listener helper? > 1360: > //getSkinnable().getStyleClass().removeListener(updateSkinIndicatorType); > 1361: > //tooltipVisibleProperty().removeListener(updateTooltipVisibility); I don't quite understand the comment. What follow-up is needed? modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java line 171: > 169: MenuBar.class, > 170: MenuButton.class, > 171: //Pagination.class, I would just remove this (which was done for other classes once the leaks were fixed). ------------- PR: https://git.openjdk.org/jfx/pull/925
