On Tue, 11 Jan 2022 20:58:05 GMT, Alisen Chung <ach...@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
