Re: RFR: 8295426: MenuButtonSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 17:00:55 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 170) and a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Also applies to SplitMenuButton, since they share the same base class >> MenuButtonSkinBase. >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or >> SPLIT_MENU_BUTTON >> >> In addition, there seems to be another failure scenario when simply >> replacing the skin - no menu is shown upon a click. To reproduce, launch >> LeakTest and click once on the [Replace Skin] button. Second click restores >> the function. >> >> caused by: >> - adding and not removing EventHandlers >> - setting and not clearing onAction handlers >> - incorrect logic in setting mousePressed/mouse/Released handlers > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 37 commits: > > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - 8295426: listener helper update > - Merge remote-tracking branch 'origin/8294809.listener.helper' into > 8295426.menu.button.skin > - 8294809: review comments > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - 8294809: whitespace > - 8294809: no public api > - 8294809: map change listener > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - ... and 27 more: https://git.openjdk.org/jfx/compare/0a785ae0...800d3f1e Fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/919
Re: RFR: 8295426: MenuButtonSkin: memory leak when changing skin [v2]
On Wed, 30 Nov 2022 17:00:55 GMT, Andy Goryachev wrote: >> as determined by SkinMemoryLeakTest (remove line 170) and a leak tester >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java >> >> Also applies to SplitMenuButton, since they share the same base class >> MenuButtonSkinBase. >> >> Make sure to configure the current test in LeakTest: >> protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or >> SPLIT_MENU_BUTTON >> >> In addition, there seems to be another failure scenario when simply >> replacing the skin - no menu is shown upon a click. To reproduce, launch >> LeakTest and click once on the [Replace Skin] button. Second click restores >> the function. >> >> caused by: >> - adding and not removing EventHandlers >> - setting and not clearing onAction handlers >> - incorrect logic in setting mousePressed/mouse/Released handlers > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 37 commits: > > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - Merge remote-tracking branch 'origin/master' into >8295426.menu.button.skin > - 8295426: listener helper update > - Merge remote-tracking branch 'origin/8294809.listener.helper' into > 8295426.menu.button.skin > - 8294809: review comments > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - 8294809: whitespace > - 8294809: no public api > - 8294809: map change listener > - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper > - ... and 27 more: https://git.openjdk.org/jfx/compare/0a785ae0...800d3f1e @aghaisas : Could you please review this next? There is another PR that touches the same area, #937. Thanks! - PR: https://git.openjdk.org/jfx/pull/919
Re: RFR: 8295426: MenuButtonSkin: memory leak when changing skin [v2]
> as determined by SkinMemoryLeakTest (remove line 170) and a leak tester > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java > > Also applies to SplitMenuButton, since they share the same base class > MenuButtonSkinBase. > > Make sure to configure the current test in LeakTest: > protected final Type WE_ARE_TESTING = Type.MENU_BUTTON; // or > SPLIT_MENU_BUTTON > > In addition, there seems to be another failure scenario when simply replacing > the skin - no menu is shown upon a click. To reproduce, launch LeakTest and > click once on the [Replace Skin] button. Second click restores the function. > > caused by: > - adding and not removing EventHandlers > - setting and not clearing onAction handlers > - incorrect logic in setting mousePressed/mouse/Released handlers Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 37 commits: - Merge remote-tracking branch 'origin/master' into 8295426.menu.button.skin - Merge remote-tracking branch 'origin/master' into 8295426.menu.button.skin - 8295426: listener helper update - Merge remote-tracking branch 'origin/8294809.listener.helper' into 8295426.menu.button.skin - 8294809: review comments - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper - 8294809: whitespace - 8294809: no public api - 8294809: map change listener - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper - ... and 27 more: https://git.openjdk.org/jfx/compare/0a785ae0...800d3f1e - Changes: https://git.openjdk.org/jfx/pull/919/files Webrev: https://webrevs.openjdk.org/?repo=jfx=919=01 Stats: 175 lines in 4 files changed: 109 ins; 32 del; 34 mod Patch: https://git.openjdk.org/jfx/pull/919.diff Fetch: git fetch https://git.openjdk.org/jfx pull/919/head:pull/919 PR: https://git.openjdk.org/jfx/pull/919
