Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v2]
On Thu, 1 Dec 2022 11:11:31 GMT, Ajit Ghaisas wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 23 commits: >> >> - Merge remote-tracking branch 'origin/master' into >>8295506.button.bar.skin >> - Merge remote-tracking branch 'origin/master' into >>8295506.button.bar.skin >> - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin >> - 8294809: generics >> - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin >> - 8294809: is alive >> - Revert "8294809: removed weak listeners support" >> >>This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. >> - 8295506: button bar skin >> - 8294809: removed weak listeners support >> - 8294809: use weak reference correctly this time >> - ... and 13 more: https://git.openjdk.org/jfx/compare/0a785ae0...759ecaf4 > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonBarSkin.java > line 139: > >> 137: } >> 138: >> 139: updateButtonListeners(getSkinnable().getButtons(), false); > > Can we use `ListenerHelper` in `updateButtonListeners` method? > We may need to use `ListernerHelper` of `ButtonSkin`. good question. I did not want to make drastic structural changes for risk of introducing regression. Right now, this skin uses the same pattern when dealing with multiple child items - it adds/removes listeners in the constructor, in the dispose() method, and upon any changes to the observable list. Perhaps not the most optimal solution, but it works and causes no trouble. I'd rather keep it as is. - PR: https://git.openjdk.org/jfx/pull/921
Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 17:10:50 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 165) 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.BUTTON_BAR; >> >> >> caused by: >> - adding and not removing listeners >> - adding and not removing children Nodes > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 23 commits: > > - Merge remote-tracking branch 'origin/master' into >8295506.button.bar.skin > - Merge remote-tracking branch 'origin/master' into >8295506.button.bar.skin > - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin > - 8294809: generics > - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin > - 8294809: is alive > - Revert "8294809: removed weak listeners support" > >This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. > - 8295506: button bar skin > - 8294809: removed weak listeners support > - 8294809: use weak reference correctly this time > - ... and 13 more: https://git.openjdk.org/jfx/compare/0a785ae0...759ecaf4 modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonBarSkin.java line 139: > 137: } > 138: > 139: updateButtonListeners(getSkinnable().getButtons(), false); Can we use `ListenerHelper` in `updateButtonListeners` method? We may need to use `ListernerHelper` of `ButtonSkin`. - PR: https://git.openjdk.org/jfx/pull/921
Re: RFR: 8295506: ButtonBarSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 165) 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.BUTTON_BAR; > > > caused by: > - adding and not removing listeners > - adding and not removing children Nodes Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge remote-tracking branch 'origin/master' into 8295506.button.bar.skin - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin - 8294809: generics - Merge branch '8294809.listener.helper' into 8295506.button.bar.skin - 8294809: is alive - Revert "8294809: removed weak listeners support" This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. - 8295506: button bar skin - 8294809: removed weak listeners support - 8294809: use weak reference correctly this time - ... and 13 more: https://git.openjdk.org/jfx/compare/0a785ae0...759ecaf4 - Changes: https://git.openjdk.org/jfx/pull/921/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=921&range=01 Stats: 24 lines in 2 files changed: 17 ins; 2 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/921.diff Fetch: git fetch https://git.openjdk.org/jfx pull/921/head:pull/921 PR: https://git.openjdk.org/jfx/pull/921
