8346753: Test javax/swing/JMenuItem/RightLeftOrientation/RightLeftOrientation.java fails on Windows Server 2025 x64 because the icons of RBMenuItem and CBMenuItem are not visible in Nimbus LookAndFeel #25907
Conversation
…entation.java fails on Windows Server 2025 x64 because the icons of RBMenuItem and CBMenuItem are not visible in Nimbus LookAndFeel
|
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
|
@prsadhuk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 219 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/touch |
|
@prsadhuk The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
/touch |
|
@prsadhuk The pull request is being re-evaluated and the inactivity timeout has been reset. |
Webrevs
|
There was a problem hiding this comment.
This is an invalid test. The L&F has to applied to entire component tree after the application changes it. One has not to expect anything to work correctly if the L&F is changed on the fly.
In this test, the L&F is set before menu items are created. Depending on the L&F internals, it may display well or not; for Windows L&F it doesn't.
The test has to be re-written so that a L&F is changed and the entire component tree is updated, at this pointer the tester verifies how menu items look.
Then the L&F can be changed to a next one. Ideally, all the installed L&Fs should be tested.
|
I dont think the test is invalid..It was in closed repo and tested for so many years before it got opensourced @prrace Any comments on this test regarding its validity and fix? |
I believe what fixed the test is that you removed the line This ensures the current L&F remains Windows L&F so that the missing resources are now found. If there were another L&F to test, or if Windows L&F weren't the last in the list, the test will continue to fail because the current L&F will again be different from Windows L&F. Indeed, it does. If I comment out At the same time, The menu items that are displayed in popups aren't visible and aren't part of the component hierarchy, therefore they remain at the assigned L&F which was active when they were created.
Yes, I've seen other tests which employ this strategy… where several components in a container are created with different L&Fs. And often everything seem to work… except for the cases where it doesn't. This is the case where it doesn't… Continuing with my example above where I commented out the line This makes sense, the Metal menu itself was created while Nimbus was the active L&F. (Yes, you moved the menu creation after the new L&F was set.) This just proves my point that multiple L&Fs aren't really supported.
I'm looking forward to seeing Phil's comments. |
But there isn't..The L&F the test wanted to test and had tested since closed days are now working with the fix and this allows the test to run ok...I dont think its a product bug since the L&F was not propagated to all nodes of the Swing components after L&F change so it should be a test issue which was solved.
I couldn't reproduce this problem with the current PR..What L&F you started the test with? I tested the test with Metal/Windows/Nimbus from the commandline and couldn't find any issue. |
I didn't say it was a product bug, it is rather a product feature: multiple L&Fs at the same time aren't really supported, have never been. As I said above, the problem wasn't that the L&F wasn't propagated to all nodes of the Swing components. The problem is and was that the current L&F doesn't match the L&F with which a component was created.
Why not now? We have a bug in the test, your fix doesn't fix the fundamental problem with the test, but rather tweaks the test so that the problem isn't visible any more.
Nimbus, as specified in the command line in the bug description:
You can't reproduce NPE if there's a call to |
I dont want to do it as I already told before..Its the same strategy used in other recently opensourced tests I mentioned before where |
But missing |
Adding this call and removing UIManager.setLookAndFeel(save) is what is used to not fallback to old L&F (this restoration to old L&F is also not used in other tests opensourced) and I stand by it.. |
| @@ -83,7 +86,7 @@ public static void main(String[] args) throws Exception { | |||
| } | |||
|
|
|||
| private static JFrame createTestUI() { | |||
| JFrame frame = new JFrame("RightLeftOrientation"); | |||
| frame = new JFrame("RightLeftOrientation"); | |||
| JMenuBar menuBar = new JMenuBar(); | |||
|
|
|||
| menuBar.add(createMenu("javax.swing.plaf.metal.MetalLookAndFeel", | |||
There was a problem hiding this comment.
I tried to extend the test (with this PR changes included) for all installed L&Fs and then create the menu for each L&F.
UIManager.LookAndFeelInfo[] lafs = UIManager.getInstalledLookAndFeels();
for (final UIManager.LookAndFeelInfo lafInfo : lafs) {
System.out.println("installed laf className : " + lafInfo.getClassName());
System.out.println("installed laf Name : " + lafInfo.getName());
menuBar.add(createMenu(lafInfo.getClassName(), lafInfo.getName()));
}
Then the test failed with this exception
java.lang.NullPointerException: Cannot invoke "java.awt.Font.hashCode()" because "font" is null
at java.desktop/sun.font.FontDesignMetrics$MetricsKey.init(FontDesignMetrics.java:213)
at java.desktop/sun.font.FontDesignMetrics.getMetrics(FontDesignMetrics.java:282)
at java.desktop/sun.swing.SwingUtilities2.getFontMetrics(SwingUtilities2.java:1235)
Otherwise the RBMI and CBMI does contain the icon after the fix.
There was a problem hiding this comment.
This is similar to the exception that I got above. It's just a result of mixing several L&Fs at the same time.
There was a problem hiding this comment.
Yes but it was not supposed to come when SwingUtilities.updateComponentTreeUI(frame) is used to update all UI components.
There was a problem hiding this comment.
Yes but it was not supposed to come when SwingUtilities.updateComponentTreeUI(frame) is used to update all UI components.
Why not?
In fact, I expected that all the menu items would switch to the latest L&F after updating the component tree.
But it doesn't happen… because the menu bar isn't part of the component hierarchy (yet). If you move the call frame.setJMenuBar(menuBar) before each L&F menu is created, all the popup menus are displayed in Windows L&F, the latest installed L&F.
Either way, the test doesn't work correctly.
|
One more thing I observed is "If we change the order of menu creation" then the icons rendered are different. then the icons are and when Windows L&F is at last pos then rendered icons are Shouldn't the icons for the L&Fs be same ? |
There was a problem hiding this comment.
Don't think it is required in the summary The tester is asked to compare left-to-right and right-to-left menus and judge whether they are mirror images of each other., it is mentioned in instruction.
Not mandatory but still for consistency, you can move This test checks if menu items lay out correctly when their on the same line as @summary.
@summary This test checks if menu items lay out correctly when their
That's the purpose of the test but it fails to test it. You've just found another proof that multiple L&Fs can't be used concurrently. I've been talking about it since the very start of this code review. When you change L&F, all the If a component or its UI doesn't store a setting, such as color, font or icon, in its field, the setting is retrieved from This is what happens in the current test without any changes. After Windows L&F is installed, the L&F is switched back to the whatever L&F was initially. As the result, there are no check marks and bullets in Windows L&F. The test has to re-create its UI after testing for a L&F is complete. Such a scenario isn't supported by The test doesn't receive events from |
So do you propose to test like what is being done in https://github.com/openjdk/jdk/pull/24439/files passing L&F argument to jtreg tag? |
Yes, this is the simplest way. Another option is to have a driver class. For example, if no arguments are passed to The first approach is simpler as it uses jtreg to run the test with different parameters. |
|
ok..test rewritten to have L&F run as different test via jtreg tags.. |
|
/integrate |
|
Going to push as commit 5205eae.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Issue is RadioButtonMenuItem and CheckBoxMenuItem bullet/checkmark icon is not displayed in WindowsL&F when the test is run with NimbusL&F.
This is because
WindowsIconFactory#VistaMenuItemCheckIcon.paintIconcalledgetLaFIcon()which returns a empty NimbusIcon which causes no icons to be drawn. This is because the test after setting WIndows L&F of the menuitem reverts back the Windows L&F to Nimbus L&F viaUIManager.setLookAndFeel(save);call in the test so when frame is made visible, the L&F resets back to Nimbus L&F resulting in null NimbusIcon.Fix is made to make sure the whole frame is updated to cater to L&F change via
SwingUtilities.updateComponentTreeUI(frame);call and keep the L&F without reverting back to original L&F..Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25907/head:pull/25907$ git checkout pull/25907Update a local copy of the PR:
$ git checkout pull/25907$ git pull https://git.openjdk.org/jdk.git pull/25907/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25907View PR using the GUI difftool:
$ git pr show -t 25907Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25907.diff
Using Webrev
Link to Webrev Comment