RFR: 8357305: Compilation failure in javax/swing/JMenuItem/bug6197830.java [v3]

[Alexey Ivanov]

On Tue, 20 May 2025 18:34:28 GMT, Manukumar V S <mvs at openjdk.org> wrote:

>> There are some compilation failures noticed in the recently open sourced test javax/swing/JMenuItem/bug6197830.java. The compilation failures are due to missing import statements and a missing dependency file MenuItemTest.java.
>> 
>> Fix: I have added the required import statements and added the code-piece from MenuItemTest.java as a method getMenuItemTestFrame().
>
> Manukumar V S has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments fixed : Formatting changes,copyright,cosmetic changes,etc

There are still unresolved comments, and I added new ones.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 148:

> 146: 
> 147:     private record ColoredIcon(Color color, int width,
> 148:                                int height) implements Icon {

Suggestion:

    private record ColoredIcon(Color color, int width, int height)
            implements Icon {

This would be better. Otherwise, I'd wrap all the parameters, including `width` because both `width` and `height` are tightly connected.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 150:

> 148:                                int height) implements Icon {
> 149:         @Override
> 150:             public void paintIcon(Component c, Graphics g, int x, int y) {

Suggestion:

        @Override
        public void paintIcon(Component c, Graphics g, int x, int y) {

You have reduce indentation of the entire `ColoredIcon` class.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 161:

> 159:             }
> 160:             @Override
> 161:             public int getIconHeight() {

I'm still for having blank lines between the methods: it makes scanning the code easier as you clearly see where a method ends and another starts.

There's no penalty for additional lines, but they improve readability, in my opinion. In an IDE, you can always collapse unimportant sections, like this helper class, and get it out of your view.

-------------

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25319#pullrequestreview-2855300606
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098678566
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098679939
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098683738