RFR: 8320343 : Generate GIF images for AbstractButton/5049549/bug5049549.java

[Alexey Ivanov]

On Fri, 8 Dec 2023 03:23:22 GMT, Renjith Kannath Pariyangad <rkannathpari at 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