On Thu, 9 May 2024 19:48:19 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 incrementally with one additional
> commit since the last revision:
>
> Add more type info
The new test passed with both the old and the new version (I reverted the code,
aside from leaving `setMenuBindings` `protected`). Is the test testing the
right thing?
(If relevant, this was done on a Windows platform)
tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
line 287:
> 285:
> 286: @Test
> 287: public void testJDK8309935() {
minor: Not sure what our policies are, but I find test names like this pretty
useless, perhaps a short indication what this is doing?
tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
line 346:
> 344: });
> 345: Util.runAndWait(() -> {
> 346: });
I checked the code that `runAndWait` would do when you pass in 0 runnables, and
it basically does no waiting at all (less than a microsecond for sure). If
this is really essential (I removed it and the test still passed) then I think
a `sleep` might be better.
-------------
Changes requested by jhendrikx (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2114917800
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637684395
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637687030
