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

[Andy Goryachev]

On Wed, 30 Nov 2022 21:24:02 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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 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 -> {

my version does not create extra object(s).

> 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 -> {

my version does not create extra object(s).

> 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);

Good suggestions.  
It is basically the existing code - I'd prefer to keep it as is, since re-writing it might introduce regression.

-------------

PR: https://git.openjdk.org/jfx/pull/906