On Mon, 4 Mar 2024 19:26:37 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> 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/f65f11d2...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? This could be a problem normally, but I think in this case you won't be able to get this to produce incorrect results. `parseText` is accessing the property (and it calls `isMnemonicParsing`), but only if the text is not empty. It's sort of "safe" as an empty text can't contain a mnemonic anyway, and you'd need to change the text at a minimum first to see the effect of turning mnemonic parsing on/off. Changing the text would trigger the `textProperty` listener, which will call `parseText` and read the mnemonic property. A change listener would be more obviously correct, but it is not strictly needed here. (not all `subscribe`s use change listener, the one that takes a `Runnable` uses invalidation) ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1512252882
