RFR: 8294589: MenuBarSkin: memory leak when changing skin [v15]

[John Hendrikx]

On Wed, 30 Nov 2022 18:43:42 GMT, Andy Goryachev <angorya at 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