On Tue, 27 Feb 2024 09:30:14 GMT, Johan Vos <j...@openjdk.org> wrote:
>> A listener was added but never removed. >> This patch removes the listener when the menu it links to is cleared. Fix >> for https://bugs.openjdk.org/browse/JDK-8319779 > > Johan Vos has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 11 additional commits since > the last revision: > > - Merge branch 'master' into 8319779-systemmenu > - Add additional test for IOOBE detection. > This test comes from JDK-8323787 > - Revert some of the conditional bindings. > Clear menu construction when an menuitem that is a menu needs to be removed > Add a test for the latter > - Merge remote-tracking branch 'upstream/master' into 8319779-systemmenu > - Cleanup test > - Add shim class so that we can access the references to > com.sun.glass.ui.Menu instances. > Add a test to make sure those references are gone. > - Revert WeakInvalidationListeners and use new listener resource management > approach. > - Fix more memoryleaks due to listeners never being unregistered. > - These changes are related to JBS-8318841 so we want to have that code in > as well. > > Merge branch 'master' into 8319779-systemmenu > - process reviewers comments > - ... and 1 more: https://git.openjdk.org/jfx/compare/a68c0a68...ec7308df modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java line 226: > 224: mb.textProperty().when(active).addListener(valueModel -> > glassMenu.setTitle(parseText(mb))); > 225: mb.disableProperty().when(active).addListener(valueModel -> > glassMenu.setEnabled(!mb.isDisable())); > 226: mb.mnemonicParsingProperty().when(active).addListener(valueModel > -> glassMenu.setTitle(parseText(mb))); Since you are not calling `get()` from the `when` resulting observable (to allow for invalidation), these need to use a `ChangeListener` instead of an `InvalidationListener`, otherwise, while `active` doesn't change, any change in those menuBase properties will trigger just one notification. Alternative, just replace `addListener` with `subscribe` (as it internally uses a `ChangeListener`). @hjohn does this make sense to you? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1511687632
