RFR: 8190264: JScrollBar ignores its border when using macOS Mac OS X Aqua look and feel [v13]

[Alexey Ivanov]

On Tue, 11 Jan 2022 20:58:05 GMT, Alisen Chung <achung at openjdk.org> wrote:

>> Adjusted the AquaLF scrollbar to account for border inset settings when dragging the thumb and clicking on the track.
>
> Alisen Chung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   updated copyright dates, updated test and fix to also check vertical borders

Changes requested by aivanov (Reviewer).

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, 2022 Oracle and/or its affiliates. All rights reserved.

The must be a comma after the second year, it was there previously.
Suggestion:

 * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java line 239:

> 237:         syncState(fScrollBar);
> 238:         Insets insets = fScrollBar.getInsets();
> 239:         return JRSUIUtils.HitDetection.getHitForPoint(painter.getControl(), 0, 0,

Probably these zeroes should rather be `insets.left` and `insets.top` correspondingly.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, 2022 Oracle and/or its affiliates. All rights reserved.

There must be a comma after the second year.
Suggestion:

 * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 33:

> 31: import java.io.File;
> 32: import java.io.IOException;
> 33: import java.lang.reflect.InvocationTargetException;

Neither `ImageObserver` nor `InvocationTargetException` are used. These two imports must be removed.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 55:

> 53:     public static final int HEIGHT = 20;
> 54: 
> 55:     private static void setLookAndFeel(UIManager.LookAndFeelInfo laf) {

I'd suggest moving the `main` here, and placing the helper `setLookAndFeel` after both `test*` methods. This way a reader would know right away what the test does, the test methods follow `main` to see the actual test code.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 88:

> 86:         // check border for thumb
> 87:         for (int i = WIDTH - BORDER_WIDTH; i < WIDTH; i++) {
> 88:             for (int j = 0; j < HEIGHT; j++) {

Now that there are two test methods, I'd suggest using `x` and `y` as loop variables:
Suggestion:

        for (int x = WIDTH - BORDER_WIDTH; x < WIDTH; x++) {
            for (int y = 0; y < HEIGHT; y++) {


This would make it clearer how comparison works.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 90:

> 88:             for (int j = 0; j < HEIGHT; j++) {
> 89:                 int c1 = image1.getRGB(i,j);
> 90:                 int c2 = image2.getRGB(i,j);

Please put a space after the commas in parameter list.
Suggestion:

                int c1 = image1.getRGB(i, j);
                int c2 = image2.getRGB(i, j);

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 91:

> 89:                 int c1 = image1.getRGB(i,j);
> 90:                 int c2 = image2.getRGB(i,j);
> 91:                 if(c1 != c2) {

Please put a space between the keywords and an opening parenthesis.
 ```suggestion
                if (c1 != c2) {

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 126:

> 124:         // check border for thumb
> 125:         for (int i = WIDTH - BORDER_WIDTH; i < WIDTH; i++) {
> 126:             for (int j = 0; j < HEIGHT; j++) {

Using `x` and `y` would make the loops clearer:
Suggestion:

        for (int y = WIDTH - BORDER_WIDTH; y < WIDTH; y++) {
            for (int x = 0; x < HEIGHT; x++) {

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 129:

> 127:                 int c1 = image1.getRGB(j,i);
> 128:                 int c2 = image2.getRGB(j,i);
> 129:                 if(c1 != c2) {

Missing spaces
Suggestion:

                int c1 = image1.getRGB(j, i);
                int c2 = image2.getRGB(j, i);
                if (c1 != c2) {

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 133:

> 131:                                        + Integer.toHexString(c1));
> 132:                     System.out.println(i + " " + j + " " + "Color2: "
> 133:                                        + Integer.toHexString(c2));

The coordinates must be reversed in the case of vertical scrollbar.
However, if you rename the loop variables to `x` and `y` as suggested above, `getRGB` and the error messages would be the same and would use the natural order for `x` and `y`.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 160:

> 158: 
> 159:     // custom border
> 160:     private static class HorizontalCustomBorder implements Border {

Both `HorizontalCustomBorder` and `VerticalCustomBorder` could be implemented as a single parametrised class. However, it's okay to keep them separate, there's not much code; introducing a new superclass wouldn't make the code clearer.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 161:

> 159:     // custom border
> 160:     private static class HorizontalCustomBorder implements Border {
> 161:         public void paintBorder(Component c, Graphics g, int x, int y, int width, int height) {

I suggest adding `@Override` annotation to the methods implementing `Border` interface, that is to all the methods in both Both `HorizontalCustomBorder` and `VerticalCustomBorder` classes.

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 167:

> 165: 
> 166:         public Insets getBorderInsets(Component c) {
> 167:             return new Insets(0, 0, 0, 150);

Suggestion:

            return new Insets(0, 0, 0, BORDER_WIDTH);

test/jdk/java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java line 183:

> 181: 
> 182:         public Insets getBorderInsets(Component c) {
> 183:             return new Insets(0, 0, 150, 0);

Suggestion:

            return new Insets(0, 0, BORDER_WIDTH, 0);

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

PR: https://git.openjdk.java.net/jdk/pull/6374