New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed

Renjithkannath wants to merge 9 commits into openjdk:master from Renjithkannath:8320343-v1

Contributor

Renjithkannath commented Dec 8, 2023 •

edited by openjdk bot

Loading

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.

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-8320343: Generate GIF images for AbstractButton/5049549/bug5049549.java (Bug - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17029

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

Using diff file

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

Webrev

Link to Webrev Comment

Renjithkannath added 3 commits

December 5, 2023 14:21


          JDK-8320343 : Generate GIF images for AbstractButton/5049549/bug50495…

ccaee2b

…49.java


          Removed commented code

efeab98


          Updated font and copyright year

8a747cb

bridgekeeper bot commented Dec 8, 2023

👋 Welcome back rkannathpari! 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 bot added the rfr label

openjdk bot commented Dec 8, 2023

@Renjithkannath 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.

openjdk bot added the client label

mlbridge bot commented Dec 8, 2023 •

edited

Loading

Webrevs

aivanov-jdk suggested changes

View reviewed changes

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

Comment on lines 120 to 122

+                      Icon RS = generateImage("DI");
+                      Icon SE = generateImage("DS");
+                      Icon PR = generateImage("RO");

Member

aivanov-jdk Dec 20, 2023

Suggested change

      
                    Icon RS = generateImage("DI");
          
                    Icon SE = generateImage("DS");
          
                    Icon PR = generateImage("RO");
          
                    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.

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

+                      g.setColor(Color.WHITE);
+                      g.fillRect(0, 0, img.getWidth(), img.getHeight());
+                      g.setColor(Color.RED);
+                      Font font = new Font("SANS_SERIF", Font.BOLD, 22);

Member

aivanov-jdk Dec 20, 2023

Suggested change

      
                    Font font = new Font("SANS_SERIF", Font.BOLD, 22);
          
                    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 Outdated

+                      g.setColor(Color.RED);
+                      Font font = new Font("SANS_SERIF", Font.BOLD, 22);
+                      g.setFont(font);
+                      g.drawString(str,5,25);

Member

aivanov-jdk Dec 20, 2023

Suggested change

      
                    g.drawString(str,5,25);
          
                    g.drawString(str, 5, 25);

Spaces after commas.

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

Comment on lines 114 to 116

		KButton button;

		Icon DE = generateImage("DE");

Member

aivanov-jdk Dec 20, 2023

If you prefer local variables, I'd move the declaration of KButton below the icons.

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

Comment on lines 51 to 54

+                  private static Icon generateImage(String str) {
+                      BufferedImage img = new BufferedImage(40, 30,
+                              BufferedImage.TYPE_INT_RGB);

Member

aivanov-jdk Dec 20, 2023

Suggested change

      
                private static Icon generateImage(String str) {
          
                    BufferedImage img = new BufferedImage(40, 30,
          
                            BufferedImage.TYPE_INT_RGB);
          
                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 Outdated

Comment on lines 41 to 44

+              import java.awt.Color;
+              import java.awt.Font;
+              import java.awt.Graphics;
+              import java.awt.image.BufferedImage;

Member

aivanov-jdk Dec 20, 2023

Please put java.awt imports above javax.* with an optional blank line between them.

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java


		public class bug5049549 {

		private static ImageIcon DE = new ImageIcon(bug5049549.class.getResource("DE1.gif"));

Member

aivanov-jdk Dec 20, 2023

    private static ImageIcon DE = generateImage("DE");

Preserve the original code style? I have no strong preference between the two in this case.

Renjithkannath added 2 commits

December 21, 2023 09:23


          Fixed typo and integrated suggsions

3be7926


          Added space befor number

3c9a02e

openjdk bot removed the rfr label


          Added space befor number

fa44c8d

openjdk bot added the rfr label


          Reverted space befor line for minimal change

87bf809

aivanov-jdk approved these changes

View reviewed changes

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

Comment on lines 48 to 54

    
                  private static Icon DE = generateImage("DE");

                  private static Icon DI = generateImage("DI");

                  private static Icon DS = generateImage("DS");

                  private static Icon RO = generateImage("RO");

                  private static Icon RS = generateImage("RS");

                  private static Icon SE = generateImage("SE");

                  private static Icon PR = generateImage("PR");

Member

aivanov-jdk Dec 21, 2023

I suggest marking all of them final:

Suggested change

      
                private static Icon DE = generateImage("DE");
          
                private static Icon DI = generateImage("DI");
          
                private static Icon DS = generateImage("DS");
          
                private static Icon RO = generateImage("RO");
          
                private static Icon RS = generateImage("RS");
          
                private static Icon SE = generateImage("SE");
          
                private static Icon PR = generateImage("PR");
          
                private static final Icon DE = generateImage("DE");
          
                private static final Icon DI = generateImage("DI");
          
                private static final Icon DS = generateImage("DS");
          
                private static final Icon RO = generateImage("RO");
          
                private static final Icon RS = generateImage("RS");
          
                private static final Icon SE = generateImage("SE");
          
                private static final Icon PR = generateImage("PR");

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

    
                  private static Icon RO = generateImage("RO");

                  private static Icon RS = generateImage("RS");

                  private static Icon SE = generateImage("SE");

                  private static Icon PR = generateImage("PR");

                  private static Blocker blocker = new Blocker();

Member

aivanov-jdk Dec 21, 2023

I'd add final modifier to blocker too.

test/jdk/javax/swing/AbstractButton/5049549/bug5049549.java Outdated

Comment on lines 38 to 46

+              /* @test
+                 @bug 5049549 7132413
+                 @summary Tests that the proper icon is used for different states.
+                 @library ../../regtesthelpers
+                 @build Blocker
+                 @run main/manual bug5049549
+              */
               public class bug5049549 {

Member

aivanov-jdk Dec 21, 2023

Since you're moving it, I suggest formatting it according to conventions used in most tests:

Suggested change

      
            /* @test
          
               @bug 5049549 7132413
          
               @summary Tests that the proper icon is used for different states.
          
               @library ../../regtesthelpers
          
               @build Blocker
          
               @run main/manual bug5049549
          
            */
          
            public class bug5049549 {
          
            /*
          
             * @test
          
             * @bug 5049549 7132413
          
             * @summary Tests that the proper icon is used for different states.
          
             * @library ../../regtesthelpers
          
             * @build Blocker
          
             * @run main/manual bug5049549
          
             */
          
            public class bug5049549 {

That is with starting asterisk on each line, and align the last asterisk.

openjdk bot commented Dec 21, 2023 •

edited

Loading

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

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

Reviewed-by: aivanov

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

a5cf421: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown
f9aec02: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures
7455b1b: 8322159: ThisEscapeAnalyzer crashes for erroneous code
d4fb308: 8317846: Typo in API documentation of classes IdentityHashMap
d786c49: 8322751: ZGC: Fix comments about marking roots
5852f3e: 8322027: One XMLStreamException constructor fails to initialize cause
be0e1c7: 8297573: Parallel: Rename do_oop_nv to do_oop_work in subclasses of OopClosure
7c1d481: 8322765: Eliminate -Wparentheses warnings in runtime code
518ec97: 8322747: StringTable should be AllStatic
32d80e2: 8322772: Clean up code after JDK-8322417
... and 210 more: https://git.openjdk.org/jdk/compare/1cf7ef520b73321c9fe7856b2f55ca6ecb555126...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 (@aivanov-jdk) 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 bot added the ready label


          Updated test param format

14616c9

Contributor Author

Renjithkannath commented Dec 22, 2023

/integrate

openjdk bot added the sponsor label

openjdk bot commented Dec 22, 2023

@Renjithkannath
Your change (at version 14616c9) is now ready to be sponsored by a Committer.


          Added final modifier

02b2d3e

openjdk bot removed the sponsor label

Contributor Author

Renjithkannath commented Dec 27, 2023

/integrate

openjdk bot added the sponsor label

openjdk bot commented Dec 27, 2023

@Renjithkannath
Your change (at version 02b2d3e) is now ready to be sponsored by a Committer.

aivanov-jdk approved these changes

View reviewed changes

Member

aivanov-jdk commented Jan 2, 2024

/sponsor

openjdk bot commented Jan 2, 2024

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

a5cf421: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown
f9aec02: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures
7455b1b: 8322159: ThisEscapeAnalyzer crashes for erroneous code
d4fb308: 8317846: Typo in API documentation of classes IdentityHashMap
d786c49: 8322751: ZGC: Fix comments about marking roots
5852f3e: 8322027: One XMLStreamException constructor fails to initialize cause
be0e1c7: 8297573: Parallel: Rename do_oop_nv to do_oop_work in subclasses of OopClosure
7c1d481: 8322765: Eliminate -Wparentheses warnings in runtime code
518ec97: 8322747: StringTable should be AllStatic
32d80e2: 8322772: Clean up code after JDK-8322417
... and 210 more: https://git.openjdk.org/jdk/compare/1cf7ef520b73321c9fe7856b2f55ca6ecb555126...master

Your commit was automatically rebased without conflicts.

openjdk bot added the integrated label

openjdk bot closed this

openjdk bot removed ready rfr sponsor labels

openjdk bot commented Jan 2, 2024

@aivanov-jdk @Renjithkannath Pushed as commit 9481d06.

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

Renjithkannath deleted the 8320343-v1 branch

January 29, 2024 07:28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client integrated