RFR: 8015854: [macosx] JButton's HTML ImageView adding unwanted padding [v6]

[Alexey Ivanov]

On Mon, 28 Feb 2022 17:28:45 GMT, DamonGuy <duke at 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