On Fri, 28 Oct 2022 20:08:33 GMT, Andy Goryachev <[email protected]> wrote:
>> Fixes memory leaks as determined by SkinMemoryLeakTest (remove line 171) and >> a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.PAGINATION; >> >> Found another issue: Pagination class does not survive replacing its skin >> (all components disappear). >> >> caused by: >> - adding and not removing listeners >> - adding and not removing children Nodes >> - setting control's properties in the constructor >> - incorrectly setting a clip rectangle >> >> NOTE: the fix will requires both ListenerHelper >> [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) and >> Skin.install() [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) >> changes. > > 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. ------------- PR: https://git.openjdk.org/jfx/pull/925
