8359108: Mac - When Swing starts First, native application menu doesn't work for JavaFX #1904
Conversation
|
👋 Welcome back pabulaner! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
This approach seems much better than the previous in that it is activated only when a JavaFX Stage becomes the focused (key) window, and it saves / restores the menu such that whenever an different (e.g., AWT) window is focused, the AWT menu will be installed. In addition to reviewing and checking that it only touches the menu when it should, testing will be critical. Reviewers: @kevinrushforth @andy-goryachev-oracle @beldenfox /reviewers 3 |
|
As mentioned above, testing will be key here. This needs to work reliably in the case of a multi-window app when the application is hidden and restored, and when switching between the windows of the app. The automated tests should already cover some of the cases. There may be others to consider. Here are a few cases that I can think of to check:
I presume that the automated tests will cover some of these, but we might want additional tests. |
|
@kevinrushforth |
|
Thanks for the test case ideas, my tests cover the AWT initialized first tests, but implementing the other tests You proposed won't be that difficult, as it is just copying the code and moving and deleting some lines. |
|
So I added most of the tests You proposed, except for the last three. For the last two I just need some more time to implement them. For the other test with AWT being initialized last and still installing the menu is something not directly implemented in this fix. I think AWT won't install a system menu if it is running embedded and therefore this would need its own fix inside AWT itself (but I don't know exactly). |
|
I have now also added the JFXPanel test, but only for the first of the two tests involving the JFXPanel (so where AWT is initialized first). In the other case AWT won't install a menu bar, since JavaFX was initialized first, so nothing will happen. |
|
I’ve been reviewing how the platform code interacts with the rest of the system. For others who want to take a look at this PR here’s a quick run-down of how the current master branch works. When JavaFX is not embedded the toolkit builds a single NSMenu to use as the system menu. It installs that NSMenu as NSApp.mainMenu when the first top-level window is shown but before it becomes the key window. This happens inside the _setMenubar call in GlassWindow.m. That menu stays in place as NSApp.mainMenu for the lifetime of the JavaFX application. Subsequent calls to _setMenubar and the logic that sets NSApp.mainMenu inside windowDidBecomeKey: are basically no-ops since it’s always the same NSMenu and it has already been installed. It might look like the system is setting a separate menubar for each window but it’s not. There’s only one, it stays in place, and the toolkit rebuilds its content as windows gain and lose focus. And it always contains at least one menu, namely the default application menu. The system also calls _setMenubar to hand the system NSMenu to each PopupWindow when it’s first shown. There doesn’t seem to be any point to this; the NSMenu was already installed by the popup’s owner and MenuBarSkin ignores popups when updating the system menu bar. When JavaFX is embedded none of the above happens. This PR doesn’t seem to change any behavior in the non-embedded case. In the embedded case everything changes. JavaFX now builds a system-wide NSMenu and hands it off to each top-level window but it only gets installed when that window becomes key. The menu is un-installed when the window resigns key (and the previous mainMenu is restored). That’s all new behavior but it seems to be working reliably. Also new with this PR: if a popup is displayed in an FXPanel the system will call _setMenubar to set the system menu. The platform code is handling this correctly (basically ignoring it) but the checks it’s making shouldn’t be necessary. It would be far better if createTKPopupStage stopped calling _setMenubar. This PR is turning on some code further up in the system that can lead to some minor bugs. For example, if you add a MenuBar to an FXPanel’s scene and then turn on useSystemMenuBar the entire menubar will become inaccessible. This is similar to another bug that’s already present in the master branch; if you add a MenuBar to a PopupWindow and then turn on useSystemMenuBar the entire menubar will disappear. These issues are all contrived and it’s easy for developers to avoid them but it would be good to tighten up the code a bit. The bugs I’ve found so far are minor enough not to block this PR and could be addressed in a separate issue. This should be easy to clean up though I can’t personally provide further assistance until November. (The primary problem is in MenuBarSkin.setSystemMenu. It crawls up the stage ownership chain to decide which MenuBar to use to populate the system menu bar. It should ignore windows that aren’t eligible for the system menu bar like embedded and popup stages. One possible solution is to call getMenubar on each stage to see if the toolkit initialized it with the system menu.) Still no show-stoppers, just some unnecessary complications from the way PopupWindows keep trying to force the system menu onto the platform code and a few very minor bugs that no developer is likely to notice. |
|
Thanks for the detailed feedback, much appreciated. So as far as I understood Your feedback it means that the PR from Your side would be good to go, but maybe in the future some minor bug / issue fixes would be nice to have? |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuJFXPanelSwingFirstTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuJFXPanelSwingFirstTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/stage/MacOSSystemMenuTestBase.java
Outdated
Show resolved
Hide resolved
|
Thanks for the improvements. I should have implemented all of them, please take a look again. |
I need to do a more detailed review of the platform code but for now I haven't seen any problems with it. To review the changes to, say, _setMenubar I needed to first understand when it is called and that's what I was focusing on. There is an unexpected testing burden. When a popup appears in an FXPanel (as a tooltip, combo box, or menu) the core code will try to set the system menu bar for no good reason and it's up to the platform code to make sure it doesn't succeed. I'm not sure it warrants a system test but some testing needs to be done for these cases. This core behavior really should be addressed in a follow up issue but that's not something I can do until November. |
There was a problem hiding this comment.
I ran the tests today on my Mac M1 system. I left one comment inline about needing to ensure that these tests only run on macOS.
The following two tests fail consistently for me:
MacOSSystemMenuMultiWindowFXOnlySwingLast
MacOSSystemMenuMultiWindowTest
The following fail intermittently:
MacOSSystemMenuSetWithSwingTestFirst
MacOSSystemMenuJFXPanelSwingFirstTest
Also, I noticed that running all 7 tests takes over 6 minutes on my system. I tracked down the reason to the large number of calls to fork the osascript process.
Here is the count by test:
| Test name | Number of calls |
|---|---|
| MacOSSystemMenuJFXPanelSwingFirstTest | 253 |
| MacOSSystemMenuMultiWindowFXOnlySwingFirst | 241 |
| MacOSSystemMenuMultiWindowFXOnlySwingLast | 276 |
| MacOSSystemMenuMultiWindowTest | 276 |
| MacOSSystemMenuMultiWindowWithSwingFirstTest | 407 |
| MacOSSystemMenuSetWithSwingTestFirst | 466 |
| MacOSSystemMenuSingleWindowWithSwingFirstTest | 165 |
It isn't a deal-breaker, but it would be nice to cut this down a bit if without sacrificing test coverage (e.g,. I think it's good to test with at least one sub-menu and menus with different number of items)
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
|
|
||
| public class MacOSSystemMenuTestBase { |
There was a problem hiding this comment.
Somewhere in this base class, add a static method annotated with @BeforeAll that does an assumeTrue(PlatformUtil.isMac()) to ensure that these tests don't execute on the other platforms.
|
I note that your branch is very out of date. Please merge the latest master into your branch. There is a minor merge conflict in |
…'t work for JavaFX # Conflicts: # modules/javafx.graphics/src/main/native-glass/mac/GlassWindow+Overrides.m
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
…'t work for JavaFX
pabulaner
force-pushed
the
system-menu
branch
from
7e6e9a6 to
358f670
Compare
|
I have rebased to the master and ran the tests again on another MacBook, but for me everything seems to work. Could You maybe provide me with some more information what exactly doesn't work with the tests for You? |
|
@pabulaner Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
To echo what the Skara said: next time, please merge master instead of rebasing to master.. Rebasing makes reviewing new change since last review difficult at best.
Sure. I'll provide the logs. |
|
Here are the assertion failures for the two failing tests: I instrumented the code and it is picking up the menus of the Terminal app from which I ran I fired off a headful test on our Jenkins system and none of the tests pass. They all timeout or fail with an error from All of this leads me to think that it might be better to convert these tests to manual test. Instead of using |
| // just store a default menu | ||
| self->hostMenu = [NSApp mainMenu]; | ||
|
|
||
| //NSLog(@"windowDidBecomeKey: %p", self); |
There was a problem hiding this comment.
Minor merge issue: This should be the first line of this method.
| [[NSApp mainMenu] update]; | ||
| } | ||
|
|
||
| //NSLog(@"windowDidResignKey: %p", self); |
There was a problem hiding this comment.
Minor merge issue: This should be the first line of this method.
3 participants
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
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1904/head:pull/1904$ git checkout pull/1904Update a local copy of the PR:
$ git checkout pull/1904$ git pull https://git.openjdk.org/jfx.git pull/1904/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1904View PR using the GUI difftool:
$ git pr show -t 1904Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1904.diff
Using Webrev
Link to Webrev Comment