On Mon, 28 Feb 2022 17:28:45 GMT, DamonGuy <d...@openjdk.java.net> wrote:
>> Html does not fit in JButton at certain sizes because default Insets cause
>> html to be displayed off-center.
>>
>> Changes made to SwingUtilities.java layoutCompoundLabelImpl method to enable
>> clipping if html does not fit, similar to regular text. AquaButtonUI.java
>> now detects when html does not fit, and an implementation for alternate
>> insets that are recursively tested for regular text inside
>> layoutAndGetText() are now also being used for html content.
>>
>> Created test (Bug8015854.java) with the same format as the test described on
>> the issue. The button is of size 37x37 with an image of a red square sized
>> 19x19. The test checks for red pixels on the edges of where the square image
>> should be from the center of the button. The test fails with the non-changed
>> jdk, but passes with the changes.
>
> DamonGuy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated exception propagation. Added button location to EDT. Updated
> testImageCentering for variable arguments. Moved object instantiation to the
> same line.
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
33:
> 31: */
> 32:
> 33: import java.awt.*;
I prefer explicit imports rather than wildcard. In recent years, wildcard
imports are usually replaced with explicit ones in the JDK code, including
tests.
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
87:
> 85: testImageCentering(leftClr, rightClr, topClr, botClr);
> 86: } catch (ClassNotFoundException | InstantiationException |
> IllegalAccessException e) {
> 87: throw new RuntimeException("Unsupported LookAndFeel: " + e);
What I meant is that can omit this catch block altogether.
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
90:
> 88: } catch (IOException e) {
> 89: //save image for troubleshooting
> 90: BufferedImage failImg = robot.createScreenCapture(new
> Rectangle(point.x - BUTTON_WIDTH / 2, point.y - BUTTON_HEIGHT / 2,
> BUTTON_WIDTH, BUTTON_HEIGHT));
Could you please wrap this long line? It's far beyond 80 columns.
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
92:
> 90: BufferedImage failImg = robot.createScreenCapture(new
> Rectangle(point.x - BUTTON_WIDTH / 2, point.y - BUTTON_HEIGHT / 2,
> BUTTON_WIDTH, BUTTON_HEIGHT));
> 91: ImageIO.write(failImg, "png", new File(testDir +
> "/fail_square.png"));
> 92: throw new RuntimeException("Failed image generation: " + e);
This doesn't make sense. I mean `IOException` can be thrown from try-block only
from `generateImage` method. In this case, there's nothing to display on screen
and therefore nothing to capture. You should not catch `IOException` at all.
You should create the screenshot if the conditions you verify aren't met. The
screenshot should be taken inside `if (!checkRedness(c))` block before you
throw the `RuntimeException` to indicate failure.
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
94:
> 92: throw new RuntimeException("Failed image generation: " + e);
> 93: } finally {
> 94: // dispose frame when done testing for a LAF before continuing
I think this comment is misleading: there's only one LaF tested. I propose
removing it.
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
95:
> 93: } finally {
> 94: // dispose frame when done testing for a LAF before continuing
> 95: SwingUtilities.invokeAndWait(() -> frame.dispose());
`frame` can be null here, if `generateImage` thrown the exception. You should
check if frame is null in the lambda expression.
test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line
141:
> 139: else {
> 140: System.out.println("-- Passed");
> 141: }
You don't need the else block. Now this message is printed four times. Just
print the message after the for-loop.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7310
