8359108: Mac - When Swing starts First, native application menu doesn't work for JavaFX

[pabulaner]

This pull request fixes the system menu bar on Mac when combining windows of Swing and JavaFX.

The first issue was to get the native menu bar working simultaneously on Swing and JavaFX, which was done by just returning always true inside the supportsSystemMenu method.

The second issue was to remove all system menu items installed by a swing window. This was fixed by checking the system menu bar every time an item is inserted or removed and removing all menu items that are not owned by JavaFX. This check is done on every insert and remove as JavaFX does not have a clear method inside the MenuBarDelegate class that could be called every time the window gets the focus.

I tested the fix with two Swing and two JavaFX windows that are run inside the same application and it worked without any errors, but on further testing I noticed some issues with the menu bar. I am currently writing the test and fixes for it.

Co-Author: @FlorianKirmaier

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8359108: Mac - When Swing starts First, native application menu doesn't work for JavaFX (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1835/head:pull/1835
$ git checkout pull/1835

Update a local copy of the PR:
$ git checkout pull/1835
$ git pull https://git.openjdk.org/jfx.git pull/1835/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1835

View PR using the GUI difftool:
$ git pr show -t 1835

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1835.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back pabulaner! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 04: Full - Incremental (6bd1ef2b)
• 03: Full - Incremental (6f5ec818)
• 02: Full - Incremental (fc230981)
• 01: Full - Incremental (4caf9a1b)
• 00: Full (2e9cb756)

[prrace]

I'm not at all sure this fix is the right thing to do.
What if Swing removed all menu items that aren't owned by Swing ?

[pabulaner]

If You mean it why I didn't fix it there the answer is that when You have for example two screens and You switch from a Swing window that is in the first screen to a other unrelated window in another screen, the menu bar should stay in the first screen. If Swing would remove the menu items on focus loss, this menu would be gone as well.

If You mean it as a edge case and if my code would handle it correctly, it would, as it just removes all items not owned by JavaFX and so if there are no menus that fall in that category, it simply will do nothing.

[beldenfox]

Any fix that happens inside Glass is the wrong fix.

I think I have an idea of what’s happening under the hood with this PR. When a JavaFX window loses focus JavaFX removes most of the items from NSApp.mainMenu. I suppose when the Swing window gains focus Swing is re-writing most of NSApp.mainMenu without JavaFX knowing about it. And the same thing is happening when focus moves from a Swing window to a JavaFX window. So window focus is being used as an informal protocol to determine whether Swing or JavaFX controls most of the system menu.

This will all break down with the application menu, the first menu in the menu bar after the Apple logo menu. That one requires special handling since it must be present at all times. JavaFX sets that up once and never touches it again. I don’t know how Swing handles this menu but I’m guessing it does something similar. It looks like this PR is removing the Swing items from the application menu so JavaFX can re-populate it. Or something like that. It doesn’t matter.

If you want Swing and JavaFX to coordinate on the system menu there needs to be a formal protocol for doing so and that formal protocol must take into account the special needs of the application menu. Neither system should be messing around at the platform level adding and removing the other guy’s menu items. JavaFX is not designed to support that sort of manipulation and I doubt Swing is either. If it works it’s entirely by accident.

[pabulaner]

So if I understand Your suggestion correctly, I should implement an event like system that allows Swing and JavaFX to communicate with each other. So JavaFX can send an event to Swing when it wants to take control of the menu bar and vice versa. Did I understand this correctly?

[andy-goryachev-oracle]

I agree with @prrace and @beldenfox, this looks like the wrong fix.

The application should decide which part of it controls the system menu and coordinate within itself which part supplies and handles the menu items.

[pabulaner]

But how should it decide this?

Because if You have for example a Swing window and a JavaFX window both should be able to set the native menu bar on MacOS when they gain focus. At least this would make the application look consistent.

Although it is discouraged to use AWT / Swing and JavaFX together, in some cases it can't be avoided due to the need to use some AWT or Swing functionality alongside with JavaFX.

So I would be really grateful if You would have some suggestions or point me in the right direction what would be an acceptable approach to fix this issue.

[andy-goryachev-oracle]

what comes to mind:

1. decide which toolkit (swing or fx) controls the system menu (let's call it the system menu manager)
2. install window/stage focus listeners on each window/stage, informing the system menu manager (in the right thread) which menus to display

[beldenfox]

The interface you want to look at is TKSystemMenu. When JavaFX wants to install a set of Menus in the system menu this is the interface it uses.

The best way to do this is to take the menus passed into TKSystemMenu, create Swing equivalents, and then install them in a Swing menubar. That way only Swing is manipulating the system menu. Unfortunately this is a lot of work in part because it's not a one-time conversion. The system menu machinery is expected to track changes made to the JavaFX menus so there are a lot of delegates and listeners involved. Fortunately there's a working example in GlassSystemMenu you can use for reference. I haven't looked into the threading issues involved here (and I'm sure there are some).

I don't think it's a good idea to try to switch control of NSApp.mainMenu between JavaFX and Swing. Under the hood they have very different ways of manipulating the mainMenu and their own assumptions about how the application menu works. And frankly you should never allow JavaFX to show its application menu in a Swing app since the JavaFX version has serious limitations. I'm working on fixing that but for now it's best that Swing is in charge of the system menu.

[FlorianKirmaier]

The model we are trying to have here is quite simple.

Background
When a Window/Toolkit gets focused, it defines which menus should be shown.
This is done by removing the old menus, and adding their own.
This is basically, what the OS does, when you switch from Application A to B, just inside one process.

Actually, we only wanted to make the JavaFX menu work when Swing/AWT was started first.
But as far as we see, our solution also makes all combinations work.

Test
What is definitely missing, which we will add, are unit tests.
We have already investigated - there is an Apple Command line API, which allows us to read out which menus are currently shown. We will add some unit-tests, which should provide confidence that this is working reliably.

I hope these tests will improve the confidence, that this solution is correct and maintainable.

[bridgekeeper]

@pabulaner This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@pabulaner This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[pabulaner]

/open

[pabulaner]

/open

[openjdk]

@pabulaner This pull request is now open

[openjdk]

@pabulaner This pull request is already open

[openjdk]

@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.

[pabulaner]

I have rewritten the entire fix so that it requires only a few and clear changes in the source code. It is now able to handle multiple AWT and JFX windows that have all installed a native system menu without them interfering with each other.

I have also added 3 test cases to show that the change works as expected:

• one test for a single window
• one test for multiple windows
• one test for enabling / disabling the system menu on a JFX window while the application is running

[beldenfox]

Using osascript to check the state of the menus is a great idea (assuming there's no policy against using that sort of thing in the system tests). Far better than what I did in PR #1881.

You need to have folks on the AWT/Swift side review this new approach. If I understand this correctly when the JavaFX window assumes focus you're stashing away NSApp.mainMenu under the assumption that it was put there by AWT. Then you're replacing it with an NSMenu built by JavaFX. When the JavaFX window loses focus you're putting the stashed AWT NSMenu back into NSApp.mainMenu. The two big questions are:

a) Will AWT malfunction since NSApp.mainMenu no longer points to the NSMenu they expect?
b) During the period of time the JavaFX window has focus is there some chance AWT will change NSApp.mainMenu out from under us?

Put another way, JavaFX currently uses window focus to figure out which window should "own" NSApp.mainMenu and this PR extends that notion to include Swing windows. I'm not sure that's matches how AWT works internally so someone on the AWT side needs to weigh in.

What you're doing on the JavaFX side is just some pointer shuffling. Swapping NSApp.mainMenu in and out makes me uneasy but it's hard for me to point to a specific problem that will arise on the JavaFX side.

There's also the larger question of whether we want to support this configuration to begin with.

[pabulaner]

I decided to close this PR and to open a new one after discussing it with @FlorianKirmaier as we both think this will simplify things. This PR contains a lot of comments on the first version of the fix and since it was entirely rewritten makes it unnecessarily complex.

[pabulaner]

I have now created the new PR: #1904

I also have taken into account the very good advice to let the AWT devs take a look at the PR and stated what the fix does , how it works and why we need this fix.