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

[Alexey Ivanov]

On Mon, 20 Nov 2023 05:28:50 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> JProgressBar is always painted with border irrespective of the value set via the API `setBorderPainted(boolean value)` in Synth (Nimbus and GTK) LAF. Proposed fix is to add a check before painting the component.
>> 
>> CI jobs are green after the fix. Links attached to JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Minor fix

Changes requested by aivanov (Reviewer).

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.

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.

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.

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?

test/jdk/javax/swing/JProgressBar/TestProgressBarBorder.java line 90:

> 88:     }
> 89: 
> 90:     private static void createAndShowUI() {

It doesn't show the UI anymore, `createProgressBar` or `initProgressBar` will be a better name.

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?

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

PR Review: https://git.openjdk.org/jdk/pull/16467#pullrequestreview-1739937455
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1399275705
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1399285139
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1399284233
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1399289964
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1399294763
PR Review Comment: https://git.openjdk.org/jdk/pull/16467#discussion_r1399295252