On Fri, 27 Mar 2020 11:46:30 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> Several controls with selection/focusModels leave memory leaks on replacing >> the model. >> >> Added tests for all such, those for the misbehaving models fail before and >> pass after the fix. >> >> for convenience, the bug reference >> https://bugs.openjdk.java.net/browse/JDK-8241455 > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > widened issue/fix/test for all controls with selection/focus models Change looks good to me. Similar to Skin classes the listeners here are also not explicitly removed. But I don't see any problematic scenario even if the listeners of an old Model are executed before it gets GCed. Suggested minor changes in test. modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 203: > 202: ObservableList<String> data = > FXCollections.observableArrayList("Apple", "Orange", "Banana"); > 203: data.forEach(text -> control.getTabs().add(new Tab("text"))); > 204: WeakReference<SelectionModel<?>> weakRef = new > WeakReference<>(control.getSelectionModel()); `new Tab("text")` -> `new Tab(text)` modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 200: > 199: // has its own issue > https://bugs.openjdk.java.net/browse/JDK-8241737 > 200: if (showBeforeReplaceSM) return; > 201: TabPane control = new TabPane(); @Ignore is used to ignore the test that are known to fail, It also becomes easy to track the ignored test with the @Ignore tag. I think it will be good to tag this scenario with a commented @Ignore inside the if statement `//@Ignore("JDK-8241737")` modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 228: > 227: // will be fixed as side-effect of skin cleanup in > https://bugs.openjdk.java.net/browse/JDK-8087555 > 228: if (showBeforeReplaceSM) return; > 229: ChoiceBox<String> control = new > ChoiceBox<>(FXCollections.observableArrayList("Apple", "Orange", > "Banana")); @Ignore inside the if statement, `//@Ignore("JDK-8087555")` modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 285: > 284: } > 285: root.getChildren().add(node); > 286: if (!stage.isShowing()) { Will it be good to add a call to `root.getChildren().removeAll()` before adding the `node` to `root` ? ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/148
