On Fri, 8 Dec 2023 03:23:22 GMT, Renjith Kannath Pariyangad
<rkannathp...@openjdk.org> wrote:
> Hi Reviewers,
>
> Updated the test and it will generate required images on the fly so storing
> and loading image from repo could be avoided. Please review and let me know
> your suggestions.
>
> Regards,
> Renjith.
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 44:
> 42: import java.awt.Font;
> 43: import java.awt.Graphics;
> 44: import java.awt.image.BufferedImage;
Please put `java.awt` imports above `javax.*` with an optional blank line
between them.
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 44:
> 42: public class bug5049549 {
> 43:
> 44: private static ImageIcon DE = new
> ImageIcon(bug5049549.class.getResource("DE1.gif"));
private static ImageIcon DE = generateImage("DE");
Preserve the original code style? I have no strong preference between the two
in this case.
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 54:
> 52:
> 53: BufferedImage img = new BufferedImage(40, 30,
> 54: BufferedImage.TYPE_INT_RGB);
Suggestion:
private static Icon generateImage(String str) {
BufferedImage img = new BufferedImage(40, 30,
BufferedImage.TYPE_INT_RGB);
A blank line between a method declaration and its first statement doesn't make
much sense, I'm for removing this blank line.
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 59:
> 57: g.fillRect(0, 0, img.getWidth(), img.getHeight());
> 58: g.setColor(Color.RED);
> 59: Font font = new Font("SANS_SERIF", Font.BOLD, 22);
Suggestion:
Font font = new Font(Font.SANS_SERIF, Font.BOLD, 22);
`"SANS_SERIF"` is wrong, you should use the constant `SANS_SERIF` in the `Font`
class, and its value is `"SansSerif"`,
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 61:
> 59: Font font = new Font("SANS_SERIF", Font.BOLD, 22);
> 60: g.setFont(font);
> 61: g.drawString(str,5,25);
Suggestion:
g.drawString(str, 5, 25);
Spaces after commas.
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 116:
> 114: KButton button;
> 115:
> 116: Icon DE = generateImage("DE");
If you prefer local variables, I'd move the declaration of `KButton` below the
icons.
test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java line 122:
> 120: Icon RS = generateImage("DI");
> 121: Icon SE = generateImage("DS");
> 122: Icon PR = generateImage("RO");
Suggestion:
Icon RS = generateImage("RS");
Icon SE = generateImage("SE");
Icon PR = generateImage("PR");
Fix copy-and-paste mistakes. Otherwise, the test fails because a _wrong_ icon
is displayed.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17029#pullrequestreview-1791194297
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432904403
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432906687
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432903396
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432897807
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432898170
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432901376
PR Review Comment: https://git.openjdk.org/jdk/pull/17029#discussion_r1432886628
