RFR: 8295754: PaginationSkin: memory leak when changing skin

[Andy Goryachev]

On Thu, 3 Nov 2022 18:46:36 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 917:
>> 
>>> 915:             });
>>> 916: 
>>> 917:             ListenerHelper.get(PaginationSkin.this).addChangeListener(getSkinnable().currentPageIndexProperty(), (src, old, cur) -> {
>> 
>> so ugly...
>> @kevinrushforth
>
> Yes, I realize that by using an accessor rather than public API, getting the listener handler goes from:
> 
> 
>    getListenerHelper();
> 
> 
> to:
> 
> 
>    ListenerHelper.get(this);
> 
> 
> or, if in a nested class, to:
> 
> 
>    ListenerHelper.get(PaginationSkin.this);
> 
> 
> Since you are going to use it in multiple places, it would be less "ugly" to cache it, either as an instance field like this:
> 
> 
>    ListenerHelper listenerHelper = ListenerHelper.get(this);
> 
> 
> Or else as a local variable in the methods where you use it:
> 
> 
>    var listenerHelper = ListenerHelper.get(PaginationSkin.this);
> 
> 
> Then in all places where you use it, you would just reference the `listenerHelper` variable.
> 
> I'd probably lean towards the instance field.

I would avoid an instance field because it's already an instance field (will waste 8 perfectly good bytes).

I suspect javac might compile these accesses away, but it needs to be verified.

Ideally, this should be a CSR and we should add the listenerHelper() method as it's probably the best way to offer its functionality to the developer (as opposed to having ten thousand register**() methods).

I can live with the accessor, it's ugly but bearable.

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

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