On Mon, 20 Nov 2023 14:22:49 GMT, Alexey Ivanov <[email protected]> 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
