On Tue, 30 Sep 2025 23:31:20 GMT, Pabulaner IV <[email protected]> wrote:
>> This pull request fixes the system menu bar on MacOS when combining windows >> of Swing and JavaFX. >> >> # Behavior before >> >> If for some reason You needed to initialize AWT before JavaFX and You wanted >> to install the system menu bar from the JavaFX side, this wasn't possible. >> This issue is persistent even when You didn't open a Swing window. >> >> One scenario where this kind of issue happens is when You use install4j (see >> https://www.ej-technologies.com/install4j). In this case AWT is initialized >> by install4j and therefore You can't use the JavaFX system menu bar anymore. >> >> >> # Behavior after >> >> The fix allows JavaFX to install a system menu bar even if it is initialized >> after AWT. This is achieved by only changing code inside JavaFX. Each JavaFX >> window stores the previously installed menu bar when gaining focus and will >> restore this menu bar if the focus was lost. This only happens if the system >> menu bar installed by the JavaFX window is still unchanged. >> >> >> # Tests >> >> This PR introduces tests for the system menu bar in addition to verifying >> its own behavior / changes. The tests include single- and multi-window tests >> while interacting with Swing. The tests ensure that the menu bar stays the >> same for each window, no matter how You switch focus between them. >> >> >> # Additional benifits >> >> This fix is not specifically for AWT, but allows JavaFX to interact much >> more compatibly with other frameworks that make use of the system menu bar. >> >> >> # Review from AWT >> >> In the previous PR related to this one, the comment was made that the folks >> from AWT should take a look at this fix. It would be great and much >> appreciated if someone could initiate it. >> >> >> # Add disable flag? >> >> We could also add a flag to prevent JavaFX from installing a system menu bar >> for users who have found other fixes for their projects / setups. This could >> be used to restore the previous behavior when AWT is initialized first. >> >> >> Co-Author: @FlorianKirmaier > > Pabulaner IV has updated the pull request incrementally with two additional > commits since the last revision: > > - 8359108: Mac - When Swing starts First, native application menu doesn't > work for JavaFX > - 8359108: Mac - When Swing starts First, native application menu doesn't > work for JavaFX This is a very preliminary review. I haven't run it yet, but I will soon. I left a few inline comments of things I happened to spot. The fix itself looks good to me. The test framework and set of tests looks fine on first glance, but I haven't looked at them in detail. I will do so, along with running them modules/javafx.graphics/src/main/native-glass/mac/GlassWindow.h line 70: > 68: // This is used to allow JFX to install its own system menu > 69: // without interfering with the hosting application. > 70: NSMenu* hostMenu; Minor: align the field name to match the rest of the fields in this file tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuJFXPanelSwingFirstTest.java line 2: > 1: /* > 2: * Copyright (c) 2017, 2024, Oracle and/or its affiliates. All rights > reserved. Replace `2024` with `2025`. Also, unless this file was substantially copied from an existing file (which it doesn't look like it was), remove the `2017,`. This applies to all of the new files. tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuJFXPanelSwingFirstTest.java line 25: > 23: * questions. > 24: */ > 25: package test.javafx.stage; I recommend moving these tests to a new package, perhaps `test.com.sun.glass.ui.mac`, since this is testing macOS glass functionality. Or maybe, `test.robot...`, since even though they aren't robot tests, they rely on Window focus working. tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java line 1: > 1: package test.javafx.stage; This needs a copyright header block. tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java line 32: > 30: import javax.swing.SwingUtilities; > 31: > 32: import org.junit.jupiter.api.Test; This is an unused import and should be removed. Especially since if there ever were a method in this class annotated with `@Test` it wouldn't work correctly since the tests in this test group are written so that there can be no more than one `@Test` in a test class, including its superclasses. tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java line 146: > 144: /// Helpers for creation and focusing of windows > 145: /// > 146: ///////////////////////////////////////////////////////// Using `///` is reserved for markdown style javadoc comments. Replace this with something else. tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java line 406: > 404: /// Helpers for synchronization > 405: /// > 406: ///////////////////////////////////////////////////////// Replace `///` with something else. tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java line 422: > 420: } > 421: > 422: protected void sleep(long millis) { This is unused, and duplicates the functionality of `Util.sleep` anyway. I recommend to remove it. ------------- PR Review: https://git.openjdk.org/jfx/pull/1904#pullrequestreview-3324402471 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2420935478 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2420944384 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2421005431 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2421290204 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2421333115 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2421304289 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2421321910 PR Review Comment: https://git.openjdk.org/jfx/pull/1904#discussion_r2421314423
