RFR: 8224261: JProgressBar always with border painted around it [v3]

[Abhishek Kumar]

On Mon, 20 Nov 2023 14:22:49 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Minor fix
>
> test/jdk/javax/swing/JProgressBar/TestProgressBarBorder.java line 33:
> 
>> 31:  *          painting is set to false
>> 32:  * @run main TestProgressBarBorder
>> 33:  */
> 
> Could you move the tags closer to the class declaration, after the imports, please?
> 
> When the tags are before the class, they're not collapsed along with the copyright header when viewed in an IDE, which makes them easily visible.

Updated.

> test/jdk/javax/swing/JProgressBar/TestProgressBarBorder.java line 51:
> 
>> 49:     private static volatile boolean isImgEqual;
>> 50:     private static BufferedImage borderPaintedImg;
>> 51:     private static BufferedImage borderNotPaintedImg;
> 
> All these could be local variables.

`boolean` variable is changed to local variable. Others are used in EDT and other method, so kept it as class variables.

> test/jdk/javax/swing/JProgressBar/TestProgressBarBorder.java line 63:
> 
>> 61:                 setLookAndFeel(laf);
>> 62:                 createAndShowUI();
>> 63:             });
> 
> Do everything on EDT or don't bother with EDT: mixing it in this way isn't good.
> 
> In this case, it's safe to call the methods you use on the main thread. However, I heard that there's a convention to run all the code on EDT. Whatever you choose… but at least don't mix the threads.

Updated.

> test/jdk/javax/swing/JProgressBar/TestProgressBarBorder.java line 67:
> 
>> 65:             borderPaintedImg = paintToImage(progressBar);
>> 66:             progressBar.setBorderPainted(false);
>> 67:             borderNotPaintedImg = paintToImage(progressBar);
> 
> ~~You may want to call `progressBar.setBorderPainted(true)` explicitly—this way the test will not depend on the default value. If whatever reason the default value changes, the test will become useless… it will be testing with `isBorderPainted == false` in both cases.~~
> You're setting it in `createAndShowUI`; I still think it's better to move the call to `progressBar.setBorderPainted(true)` here.
> 
> Aren't `withBorder` and `withoutBorder` more succinct names?

Updated.

> test/jdk/javax/swing/JProgressBar/TestProgressBarBorder.java line 93:
> 
>> 91:         progressBar = new JProgressBar();
>> 92:         progressBar.setSize(100, 50);
>> 93:         // set initial value
> 
> The comment is redundant, isn't it?

Yeah.. updated.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1400199006
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1400202704
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1400202971
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1400198329
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1400197734