On Wed, 30 Nov 2022 18:43:42 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Fixed memory leak by using weak listeners and making sure to undo everything >> that has been done to MenuBar/Skin in dispose(). >> >> This PR depends on a new internal class ListenerHelper, a replacement for >> LambdaMultiplePropertyChangeListenerHandler. >> See https://github.com/openjdk/jfx/pull/908 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > 8294589: cleanup modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 286: > 284: ParentHelper.setTraversalEngine(getSkinnable(), engine); > 285: > 286: lh.addChangeListener(control.sceneProperty(), true, (scene) -> { minor: generally, the parenthesis are omitted for lambda's with one parameter (multiple occurences) modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 293: > 291: > 292: if (scene != null ) { > 293: sceneListenerHelper = new > ListenerHelper(MenuBarSkin.this); Why does this (still) need to rely on a weak reference? Skins have a well specified life cycle do they not? modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 360: > 358: break; > 359: default: > 360: break; minor: indent is incorrect (also in original) modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 374: > 372: > 373: // When the parent window looses focus - menu selection > should be cleared > 374: > sceneListenerHelper.addChangeListener(scene.windowProperty(), true, (sr, > oldw, w) -> { Suggestion: sceneListenerHelper.addChangeListener(scene.windowProperty(), true, w -> { modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 381: > 379: > 380: if (w != null) { > 381: windowFocusHelper = > sceneListenerHelper.addChangeListener(w.focusedProperty(), true, (s, p, > focused) -> { Suggestion: windowFocusHelper = sceneListenerHelper.addChangeListener(w.focusedProperty(), true, focused -> { modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 387: > 385: }); > 386: } > 387: }); The goal seems to be to subscribe to changes on scene->window->focused. This can be done with `flatMap` / `orElse`. Suggestion: // When the parent window looses focus - menu selection should be cleared sceneListenerHelper.addChangeListener( scene.windowProperty() .flatMap(Window::focusedProperty) .orElse(false), true, focused -> { if (!focused) { unSelectMenus(); } } ); You could even do this at the top level I think: lh.addChangeListener( control.sceneProperty() .flatMap(Scene::windowProperty) .flatMap(Window::focusedProperty) .orElse(false), true, focused -> { if (!focused) { unSelectMenus(); } } ); Also available is to make use of `Subscription`: Subscription sub = Subscription.subscribe(scene.windowProperty().flatMap(Window::focusedProperty).orElse(false), focused -> { if (!focused) { unSelectMenus(); } }); The subscription can then be integrated with `ListenerHelper` again (or even more directly if it accepted `Subscription`): lh.addDisconnectable(sub::unsubscribe); modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 436: > 434: if (weakSceneAltKeyEventHandler != null) { > 435: t.removeEventHandler(KeyEvent.ANY, > weakSceneAltKeyEventHandler); > 436: } So, am I correct that `MenuBarSkin` was badly broken before as it never re-registers these weak handlers when the scene changes? It does re-register the F10 accelerator, but that's all I can see. So a scenario where I have a MenuBar, and I move it to another Scene, it would basically no longer fully function? modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 1180: > 1178: > 1179: private static final CssMetaData<MenuBar,Pos> ALIGNMENT = > 1180: new CssMetaData<>("-fx-alignment", new > EnumConverter<Pos>(Pos.class), Pos.TOP_LEFT ) { minor: You can use the diamond operator here, probably came from the merge conflict Suggestion: new CssMetaData<>("-fx-alignment", new EnumConverter<>(Pos.class), Pos.TOP_LEFT) { modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java line 227: > 225: ComboBox.class, > 226: DatePicker.class, > 227: //MenuBar.class, minor: commented out code ------------- PR: https://git.openjdk.org/jfx/pull/906
