8365389: Remove static color fields from SwingUtilities3 and WindowsMenuItemUI #26783

prrace · 2025-08-14T16:19:08Z

This refactors some Swing code to pass args instead of using statics
The bug report suggests some further refactoring which could be considered later, but the most
important thing to do is to eliminate using statics to pass args.
I've added one other suggestion from the bug report to have the windows case call SU3 directly rather than via newly added static methods, but for the basic case, I left the pre-existing private instance methods.

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-8365389: Remove static color fields from SwingUtilities3 and WindowsMenuItemUI (Bug - P3)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer) Review applies to a1f59db1
Damon Nguyen (@DamonGuy - Committer) Review applies to a1f59db1
Prasanta Sadhukhan (@prsadhuk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26783

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26783.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-14T16:20:09Z

👋 Welcome back prr! 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 · 2025-08-14T16:20:14Z

@prrace 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:

8365389: Remove static color fields from SwingUtilities3 and WindowsMenuItemUI

Reviewed-by: psadhukhan, aivanov, dnguyen

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 77 new commits pushed to the master branch:

2e06a91: 8365841: RISC-V: Several IR verification tests fail after JDK-8350960 without Zvfh
ecab52c: 8365610: Sort share/jfr includes
ed7d5fe: 8360304: Redundant condition in LibraryCallKit::inline_vector_nary_operation
... and 74 more: https://git.openjdk.org/jdk/compare/b0f98df75aee1e94a8c4b3eb8d0b1f4e715011ae...master

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 master branch, type /integrate in a new comment.

openjdk · 2025-08-14T16:21:19Z

@prrace The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-08-14T16:24:23Z

Webrevs

prrace · 2025-08-14T16:55:35Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java

-                        acceleratorSelectionForeground);
-        SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
-        SwingUtilities3.paintAccText(g, lh, lr);
+        SwingUtilities3.paintAccText(g, lh, lr,


I left this private method here rather than calling the SU3 code directly because this method was here all along, before the previous fix. Same for a couple of others.

Previously, these private methods did the work that's now performed by the new methods in SwingUtilities3.

It's fine to postpone removing these private methods.

aivanov-jdk · 2025-08-14T19:57:54Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java

-                        acceleratorSelectionForeground);
-        SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
-        SwingUtilities3.paintAccText(g, lh, lr);
+        SwingUtilities3.paintAccText(g, lh, lr,


Previously, these private methods did the work that's now performed by the new methods in SwingUtilities3.

It's fine to postpone removing these private methods.

aivanov-jdk · 2025-08-15T15:18:01Z

There are still static fields to store colors in WindowsMenuItemUI, do you want to get rid of those too? My bug report implied removing them, yet currently the static fields with colors remain intact in WindowsMenuItemUI.

DamonGuy

Removal looks correct when I compare it to the original implementation PR. Double-checked this by building and testing the changes on all OS's and I don't see any issues. LGTM

prrace · 2025-08-15T21:21:05Z

There are still static fields to store colors in WindowsMenuItemUI, do you want to get rid of those too? My bug report implied removing them, yet currently the static fields with colors remain intact in WindowsMenuItemUI.

Yes I do. I overlooked them.

aivanov-jdk · 2025-08-15T21:24:54Z

There are still static fields to store colors in WindowsMenuItemUI, do you want to get rid of those too? My bug report implied removing them, yet currently the static fields with colors remain intact in WindowsMenuItemUI.

Yes I do. I overlooked them.

If you do, this will resolve JDK-8365625 that I submitted a few minutes ago.

prrace · 2025-08-15T22:09:15Z

There are still static fields to store colors in WindowsMenuItemUI, do you want to get rid of those too? My bug report implied removing them, yet currently the static fields with colors remain intact in WindowsMenuItemUI.

Yes I do. I overlooked them.

If you do, this will resolve JDK-8365625 that I submitted a few minutes ago.

oh. ok. I guess I will look at your test case and confirm that .. and looks like it is a fully done regression test.
Maybe you want to push that yourself ?

aivanov-jdk · 2025-08-18T16:23:07Z

There are still static fields to store colors in WindowsMenuItemUI, do you want to get rid of those too? My bug report implied removing them, yet currently the static fields with colors remain intact in WindowsMenuItemUI.

Yes I do. I overlooked them.

If you do, this will resolve JDK-8365625 that I submitted a few minutes ago.

oh. ok. I guess I will look at your test case and confirm that .. and looks like it is a fully done regression test.

Yes, it is. It was easier to make it a full regression test.

Maybe you want to push that yourself ?

Yeah, I guess so. I didn't look to update this myself initially. When I left my first comment above, I didn't come up with a test that proves there's a bug even though I had a hunch.

Now I looked at the code and resolved the bug myself. I also submitted my PR #26826 that's based on yours.

I edited the subject of the JBS issue that this PR resolves, now it doesn't reference WindowsMenuItemUI.

aivanov-jdk · 2025-08-18T19:27:59Z

@prrace Your latest commit 1b67651 doesn't address the problem fully, you should also remove WindowsMenuItemUI.installDefaults.

I posted a comment where I said that I went ahead and submitted my own PR #26826 to resolve JDK-8365625. My PR removes the static fields from WindowsMenuItemUI and adds the regression test attached to the JBS issue.

I propose you revert commit 1b67651 and integrate this PR in the state it was last week with the updated subject that doesn't mention WindowsMenuItemUI.

prsadhuk · 2025-08-20T04:58:26Z

@prrace Your latest commit 1b67651 doesn't address the problem fully, you should also remove WindowsMenuItemUI.installDefaults.

Shouldn't we keep it in WIndowMenuItemUI
as in BasicLookAndFeel the fields are initialized as

"MenuItem.disabledForeground", null,
            "MenuItem.acceleratorForeground", menuText,
            "MenuItem.acceleratorSelectionForeground", textHighlightText,

whereas in WIndowsLookAndFeel it is

"MenuItem.disabledForeground", InactiveTextColor,
            "MenuItem.acceleratorForeground", MenuTextColor,
            "MenuItem.acceleratorSelectionForeground", SelectionTextColor,

so I think it would be better to initialize the defaults in the L&F it is currently on

prrace · 2025-08-20T20:23:09Z

@prrace Your latest commit 1b67651 doesn't address the problem fully, you should also remove WindowsMenuItemUI.installDefaults.

You are right. What's there just duplicates the super-class. Although I don't know what "problem" it causes.

I posted a comment where I said that I went ahead and submitted my own PR #26826 to resolve JDK-8365625. My PR removes the static fields from WindowsMenuItemUI and adds the regression test attached to the JBS issue.

I thought that was supposed to be a follow-on ONCE this was done and I indicated last week I'd be removing these.

I propose you revert commit 1b67651 and integrate this PR in the state it was last week with the updated subject that doesn't mention WindowsMenuItemUI.

I think this should go as it is, and that follow-on can be updated.
You can address the duplicate installDefaults()

Also I just noticed this comment
"I edited the subject of the JBS issue that this PR resolves, now it doesn't reference WindowsMenuItemUI"

I'd prefer you not have done that. In fact I was puzzled at the PR/JBS mismatch and restored the subject.

8365389

f991a86

openjdk bot changed the title ~~8365389~~ 8365389: Remove static color fields from SwingUtilities3 and WindowsMenuItemUI Aug 14, 2025

openjdk bot added client [email protected] rfr Pull request is ready for review labels Aug 14, 2025

8365389

a1f59db

prrace commented Aug 14, 2025

View reviewed changes

aivanov-jdk approved these changes Aug 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 14, 2025

DamonGuy approved these changes Aug 15, 2025

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Aug 18, 2025

openjdk bot mentioned this pull request Aug 18, 2025

8365625: Can't change accelerator colors in Windows L&F #26826

Open

3 tasks

openjdk bot removed the rfr Pull request is ready for review label Aug 18, 2025

8365389

1b67651

8365389

89d6bfc

openjdk bot added the rfr Pull request is ready for review label Aug 19, 2025

prsadhuk approved these changes Aug 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 21, 2025

8365389: Remove static color fields from SwingUtilities3 and WindowsMenuItemUI #26783

Are you sure you want to change the base?

8365389: Remove static color fields from SwingUtilities3 and WindowsMenuItemUI #26783

Conversation

prrace commented Aug 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 14, 2025

Uh oh!

openjdk bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 14, 2025

Uh oh!

mlbridge bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

prrace Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Aug 15, 2025

Uh oh!

DamonGuy left a comment

Choose a reason for hiding this comment

Uh oh!

prrace commented Aug 15, 2025

Uh oh!

aivanov-jdk commented Aug 15, 2025

Uh oh!

prrace commented Aug 15, 2025

Uh oh!

aivanov-jdk commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aivanov-jdk commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prsadhuk commented Aug 20, 2025

Uh oh!

prrace commented Aug 20, 2025

Uh oh!

Uh oh!

prrace commented Aug 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 14, 2025 •

edited

Loading

mlbridge bot commented Aug 14, 2025 •

edited

Loading

aivanov-jdk commented Aug 18, 2025 •

edited

Loading

aivanov-jdk commented Aug 18, 2025 •

edited

Loading