On Tue, 20 May 2025 18:34:28 GMT, Manukumar V S <m...@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
