On Mon, 9 May 2022 22:04:38 GMT, Alisen Chung <ach...@openjdk.org> wrote:
>> Changed the drawing area to be increased by 0.5 on the left side to prevent
>> clipping
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> made suggested changes
Overall, it looks very good. The test catches bad rendering of both horizontal
and vertical border. It passes successfully with the fix.
Except for the minor notes I left, I have one more suggestion: run the test for
all the scales / images that we have rather than failing the test at a
encountered first error. It'll give the an overview of all border rendering.
On the other hand, we don't expect the test to fail in the future…
One more thing: the test now uses `EtchedBorder` only, shall it be moved to
`test/jdk/java/awt/EtchedBorder`? And adding the word _Scaled_ to the title —
`ScaledEtchedBorderTest` — would clarify the purpose of the test.
What do you think?
src/java.desktop/share/classes/javax/swing/border/EtchedBorder.java line 177:
> 175: : getShadowColor(c),
> w, h, stkWidth);
> 176: paintBorderRect(g, (etchType == LOWERED) ? getShadowColor(c)
> 177: :
> getHighlightColor(c), w, h, stkWidth);
I suggest aligning the colon `:` with `?` and wrapping following parameters:
Suggestion:
paintBorderShadow(g, (etchType == LOWERED) ? getHighlightColor(c)
: getShadowColor(c),
w, h, stkWidth);
paintBorderRect(g, (etchType == LOWERED) ? getShadowColor(c)
: getHighlightColor(c),
w, h, stkWidth);
I think it's clearer this way. Alternatively, leave the ternary operator on the
first line and wrap the following parameters to the second line.
Should `paintBorderRect` be `paintBorderHighlight`?
The first method is `paintBorderShadow`.
test/jdk/java/awt/TitledBorder/TitledBorderTest.java line 95:
> 93: BufferedImage img = images.get(i);
> 94: double scaling = scales[i];
> 95:
Does it make sense to print the current scale being tested?
System.out.printf("Scaling: %.2f\n", scaling);
It may be especially useful for error message which don't include the current
scale.
test/jdk/java/awt/TitledBorder/TitledBorderTest.java line 112:
> 110: boolean checkHighlight = false;
> 111: for (int i = 0; i < img.getWidth(); i++) {
> 112: int color = img.getRGB(i,y);
Suggestion:
for (int x = 0; x < img.getWidth(); x++) {
int color = img.getRGB(x, y);
The loop is for `x` coordinate.
test/jdk/java/awt/TitledBorder/TitledBorderTest.java line 124:
> 122: thickness++;
> 123: } else if (color == highlight.getRGB()) {
> 124: verifyThickness(y, thickness, scaling, "Horizontal");
Here, the IDE warns `y` probably shouldn't be passed as `x` parameter.
In fact, `verifyThickness` doesn't use it at the moment. To make the error
message more informative, you may pass both `x` and `y` and also include the
real vs expected thickness.
test/jdk/java/awt/TitledBorder/TitledBorderTest.java line 157:
> 155: boolean checkHighlight = false;
> 156: for (int i = 0; i < img.getHeight(); i++) {
> 157: int color = img.getRGB(x,i);
Suggestion:
for (int y = 0; y < img.getHeight(); y++) {
int color = img.getRGB(x, y);
Here we're changing `y` coordinate.
test/jdk/java/awt/TitledBorder/TitledBorderTest.java line 248:
> 246: }
> 247:
> 248: private static void setLookAndFeel(UIManager.LookAndFeelInfo laf) {
Please remove this unused method.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7449
