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

[John Hendrikx]

On Tue, 11 Oct 2022 18:50:54 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>>> You are right, @kleopatra - this executeOnceWhenPropertyIsNonNull() is installing a listener. perhaps we should add a similar functionality to LambdaMultiplePropertyChangeListenerHandler and actually install a WeakListener instead.
>> 
>> - yes, we need weakListeners (to wait for a not-null value to add the event handler) _and_ remove both the weakListener and the event handlers in dispose
>> - adding functionality to the LambdaHandler sounds like a good idea, but needs some thinking and should be done in a separate issue which also adds delegate methods to expose the new functionality in SkinBase for use in sub classes
>> 
>> For this issue, you might simply inline the utility method and register the listeners using skinbase api (didn't try, it's your issue :), see TableRowSkin for a similar pattern 
>> 
>> This conditional adding of handlers/listeners is needed if we have to support path properties (also f.i. when listening to selectedXX in any of the skins with selectionModels as properties). Which basically should be supported by base - but probably without any chance to ever get them ;)
>
> ListenerHelper - replacement for LambdaMultiplePropertyChangeListenerHandler .
> Please take a look at https://github.com/openjdk/jfx/pull/908

> Sure, but I think this should be done with a clear vision of where we're going, and not just as a collection of ad-hoc helper methods. There are even more problems that a new API should at least consider: Some skins change their behavior depending on the configuration of their associated control at the time when the skin is installed. For example, `MenuButtonSkinBase` adds a `MOUSE_PRESSED` handler only if the control didn't specify one for its onMousePressed property:
> 
> ```
> if (control.getOnMousePressed() == null) {
>     control.addEventHandler(MouseEvent.MOUSE_PRESSED, ...
> ```
> 
> But what happens if the `onMousePressed` property is set after the skin is installed? Rather than a one-time check, cases like these should probably be a dynamic expression:
> 
> ```
> when control.onMousePressedProperty().isNotNull() and skin.isInstalled()
>     control.addEventHandler
> otherwise
>     control.removeEventHandler
> ```

I think you meant `isNull()` instead of `isNotNull()`:

    when control.onMousePressedProperty().isNull() and skin.isInstalled()
        control.addEventHandler
    otherwise
        control.removeEventHandler

So... I was curious, and was checking to see if this can be done with fluent bindings these days, and I can get quite close. First assume we have a convenient property in all skins (or everything that is `SkinBase`) that reflects whether or not the skin is currently installed or active:

        BooleanProperty installed = new SimpleBooleanProperty();

You can then create a condition:

        ObservableValue<Boolean> noMouseAndInstalled = installed
            .flatMap(b -> b ? control.onMousePressedProperty() : installed)
            .map(x -> false)  // ---+ use case for isNotEmpty
            .orElse(true);    // --/

Or (if `isNotEmpty` already exists): 

        ObservableValue<Boolean> noMouseAndInstalled = installed
            .flatMap(b -> b ? control.onMousePressedProperty() : installed)
            .isNotEmpty();

It's then a simple matter of adding a listener to this result to either add or remove the event handler:

         installed
            .flatMap(b -> b ? control.onMousePressedProperty() : installed)
            .isNotEmpty();
            .addListener((obs, old, active) -> {
               if(active) control.addEventHandler(...);
               else control.removeEventHandler(...);
            });

One of the nice things here is that when `installed` is `false`, the listener on `control.onMousePressedProperty()` is automatically removed.  This is because the `flatMap` will switch to listening to `installed`.  This should mean that it will automatically be eligible for GC when `installed` becomes `false`.

The second nice thing is that you can do this in the constructor already, no need to do this in the `install` callback.  If you do it in the callback, then make sure to do it before `installed` is set to `true` as otherwise the change listener won't fire for the first change (another reason to have value listeners...)

If this becomes a common idiom, we could introduce a short circuiting `and` construct so the `flatMap` can be expressed easier (the short circuiting part will ensure the onMousePressedProperty won't be observed if installed is `false`):

      installed
          .and(control.onMousePressedProperty().isNotEmpty())
          .addListener( ... );

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

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