8280818: Expand bug8033699.java to iterate over all LaFs #23964

rajamah · 2025-03-10T14:48:09Z

Added code to Iterates over all installed Look and Feel environments and added related exception handling.

Testing done.

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-8280818: Expand bug8033699.java to iterate over all LaFs (Enhancement - P4)

Reviewers

Damon Nguyen (@DamonGuy - Committer)
Alexey Ivanov (@aivanov-jdk - Reviewer)
Harshitha Onkar (@honkar-jdk - Reviewer)
Abhishek Kumar (@kumarabhi006 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23964

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-10T14:49:08Z

👋 Welcome back rmahajan! 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-03-10T14:49:23Z

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

8280818: Expand bug8033699.java to iterate over all LaFs

Reviewed-by: dnguyen, aivanov, honkar, abhiscxk

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

37ec796: 8351500: G1: NUMA migrations cause crashes in region allocation
4e51a8c: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long)
7e3bc81: 8351216: ZGC: Store NUMA node count
82eb780: 8351349: GSSUtil.createSubject has outdated access control context and policy related text
c3db667: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings
375722f: 8351839: RISC-V: Fix base offset calculation introduced in JDK-8347489
4c5956d: 8350866: [x86] Add C1 intrinsics for CRC32-C
9c00331: 8330469: C2: Remove or change "PrintOpto && VerifyLoopOptimizations" as printing code condition
c18494d: 8351108: ImageIO.write(..) fails with exception when writing JPEG with IndexColorModel
86860ca: 8346916: [REDO] align_up has potential overflow
... and 186 more: https://git.openjdk.org/jdk/compare/9ec46968fbfddf99a8349cb6903d24b1c2fdaf1d...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@DamonGuy, @aivanov-jdk, @honkar-jdk, @kumarabhi006) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-03-10T14:50:09Z

@rajamah 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-03-10T14:53:22Z

Webrevs

00: Full (65d95acf)

kumarabhi006 · 2025-03-11T06:28:07Z

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java

Minor changes can be done

mainFrame = new JFrame("Bug 8033699 - 9 Tests for Grouped / Non-Grouped Radio Buttons");

Non group can be changed to non-group (For consistency across test)

Please limit to 80 cols wherever applicable

Otherwise the changes look good. Verified test on macOS and it works as expected.

Should these be done as part of this change or a separate bug for refactoring /code cleanup of this test ?

I usually fix these when I touch the test anyway.

mainFrame = new JFrame("Bug 8033699 - 9 Tests for Grouped / Non-Grouped Radio Buttons");

Makes sense… However, a generic title would be good enough. Something like “Radio button focus tests”. The current title is too long, it doesn't fit in the title bar of the frame (at least on Windows), therefore I see no point in making it comprehensive and long.

I usually fix these when I touch the test anyway.

In majority of cases, I do too. Yet I tend not to change lines that I don't touch. From this point of view, additional changes aren't necessary — none of the lines that don't fit into 80-column limit aren't touched.

The problem I see with additional refactoring is that it adds noise to the code review and it makes it harder to understand what the real, important changes are.

Use your common sense.

Please limit to 80 cols wherever applicable

This is not applied strictly… I'm for following the 80-column limit where it doesn't reduce the readability. Yet I'm for stronger enforcement of 100-column limit. There are quite a few lines which are longer than 100 columns. The culprit is KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner() which accounts for 70 characters.

I'd like to make it shorter, and the focus manager can be cached after the first usage. At the same time, I'm unsure doing so in this code review is reasonable.

In short, I'd rather avoid doing additional refactoring, except for changing the frame title if Rajat wants to, because none of the lines that need updating are not touched by the current changes.

I have filed a new bug to incorporate the suggestions here and re-factor test code separately - JDK-8351884

DamonGuy

I also tested this and the test passes consistently on all LAFs.

DamonGuy · 2025-03-11T22:49:20Z

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java

+        } catch (ClassNotFoundException | InstantiationException |
+                 IllegalAccessException | UnsupportedLookAndFeelException e) {


This can just be set to Exception e rather than expanding it into separate exceptions.

This provides more targeted error messages and logging.
If you catch a broad Exception, you might inadvertently mask other unexpected errors that should be investigated.
Separate exception handlers make it easier to identify the root cause of an issue.

I'm fine with separate types.

You re-throwing any of the exceptions wrapped into RuntimeException. From this point of view, a single Exception won't change anything: if an exception is thrown, it's caught and re-thrown. These four exceptions are the only checked exceptions that can be thrown, catching Exception will catch them either way.

Either way is fine with me.

honkar-jdk · 2025-03-12T17:44:14Z

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java

+            SwingUtilities.invokeAndWait(() -> {
+                if (mainFrame != null) {
+                    mainFrame.dispose();
+                    mainFrame = null;


Assigning mainFrame = null is not required since we are disposing the frame at this point.

Assigning mainFrame = null is not required since we are disposing the frame at this point.

It's required here, because the loop is repeated, and setting mainFrame to null prevents access to a disposed frame… likely from disposing of the frame again.

Missed seeing the finally block is within the LaF loop. Looks fine then.

aivanov-jdk

Looks good to me.

kumarabhi006

Good to have a separate bug for re-factoring. Otherwise, looks good to me.

rajamah · 2025-03-13T16:37:28Z

/integrate

openjdk · 2025-03-13T16:38:52Z

@rajamah
Your change (at version 65d95ac) is now ready to be sponsored by a Committer.

aivanov-jdk · 2025-03-13T17:08:18Z

/sponsor

openjdk · 2025-03-13T17:09:20Z

Going to push as commit b50fe9b.
Since your change was applied there have been 198 commits pushed to the master branch:

771e160: 8351323: Parameterize compiler and linker flags for iconv
0ff1c08: 8349984: (jdeps) jdeps can use String.repeat instead of String.replaceAll
37ec796: 8351500: G1: NUMA migrations cause crashes in region allocation
4e51a8c: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long)
7e3bc81: 8351216: ZGC: Store NUMA node count
82eb780: 8351349: GSSUtil.createSubject has outdated access control context and policy related text
c3db667: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings
375722f: 8351839: RISC-V: Fix base offset calculation introduced in JDK-8347489
4c5956d: 8350866: [x86] Add C1 intrinsics for CRC32-C
9c00331: 8330469: C2: Remove or change "PrintOpto && VerifyLoopOptimizations" as printing code condition
... and 188 more: https://git.openjdk.org/jdk/compare/9ec46968fbfddf99a8349cb6903d24b1c2fdaf1d...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-13T17:09:37Z

@aivanov-jdk @rajamah Pushed as commit b50fe9b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

rajamah added 3 commits February 28, 2025 10:44

8280818: Expand bug8033699.java to iterate over all LaFs

815ff78

modified code to do exception handling better

b9c0d7c

code refactoring

df1878f

removed unnecessary import

65d95ac

openjdk bot added the rfr Pull request is ready for review label Mar 10, 2025

openjdk bot added the client client-libs-dev@openjdk.org label Mar 10, 2025

kumarabhi006 reviewed Mar 11, 2025

View reviewed changes

DamonGuy approved these changes Mar 11, 2025

View reviewed changes

honkar-jdk reviewed Mar 12, 2025

View reviewed changes

aivanov-jdk approved these changes Mar 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 12, 2025

honkar-jdk approved these changes Mar 12, 2025

View reviewed changes

kumarabhi006 approved these changes Mar 13, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 13, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 13, 2025

openjdk bot closed this Mar 13, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 13, 2025

aivanov-jdk mentioned this pull request Apr 17, 2025

8351884: Refactor bug8033699.java test code #24384

Closed

3 tasks

		} catch (ClassNotFoundException \| InstantiationException \|
		IllegalAccessException \| UnsupportedLookAndFeelException e) {

8280818: Expand bug8033699.java to iterate over all LaFs #23964

8280818: Expand bug8033699.java to iterate over all LaFs #23964

Uh oh!

Conversation

rajamah commented Mar 10, 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 Mar 10, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Mar 10, 2025

Uh oh!

mlbridge bot commented Mar 10, 2025

Webrevs

Uh oh!

kumarabhi006 Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DamonGuy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

kumarabhi006 left a comment

Choose a reason for hiding this comment

Uh oh!

rajamah commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

aivanov-jdk commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

rajamah commented Mar 10, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 10, 2025 •

edited

Loading

kumarabhi006 Mar 11, 2025 •

edited

Loading