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

[Phil Race]

On Thu, 3 Feb 2022 23:35:42 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 two additional commits since the last revision:
> 
>  - Add cycling of all installed LAFs. Add image generation to avoid saving an image to repo.
>  - Changed frame to dispose regardless of passing or failing.

" I discovered that the issue exists for Aqua LAF as described, but also other LAFs (such as Metal and Basic)."

BTW there is no such L&F as "Basic" - it is just used as the basis for other concrete L&Fs such as Metal.

src/java.desktop/share/classes/javax/swing/SwingUtilities.java line 1129:

> 1127:             v = (c != null) ? (View) c.getClientProperty("html") : null;
> 1128:             if (v != null) {
> 1129:                 if(availTextWidth < (int) v.getPreferredSpan(View.X_AXIS)){

Can you attach "before" and "after" images into the bug report as part of the evaluation ?
Do this for Aqua and Metal too.

I presume this code only gets entered for labels with images ?
If this is all L&Fs then please manually test on mac. Windows and GTK L&Fs using SwingSet2 looking for problems as well as running all automated client tests using mach5.

We do not use syntax like "if(" and "else{".
It should always be "if (" and "else {"
Also no space after (int) for the cast. lines 1129 and 1134 .. 
And make sure lines are <=80 characters long which that one 1134 doesn't look to be.

test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line 2:

> 1: /*
> 2:  * Copyright 2022 JetBrains s.r.o.

I am reasonably sure you work for Oracle, not JetBrains. I don't know where you got this (c) from but it should be a standard Oracle copyright + GPL v2 as used by tests.

test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line 77:

> 75: 
> 76:         // generate red_square.png image to use as JButton text
> 77:         SwingUtilities.invokeAndWait(() -> {

generateImage isn't doing anything that needs to be run on the EDT. Get rid of invokeAndWait here.

test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line 103:

> 101:             // close frame when complete
> 102:             SwingUtilities.invokeAndWait(() -> {
> 103:                 frame.dispose();

unless I am missing something you are creating a new JFrame for every call to createAndShowGUI() and over-writing the frame variable each time . so only the last one can be being disposed by this call, no ?

In fact you aren't even hiding them, so doesn't the screen get cluttered with a stack of them ?

Maybe you should only create the UI once and just update its L&F each time through.

test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line 175:

> 173:         try {
> 174:             if (ImageIO.write(bImg, "png", new File(srcDir + "/red_square.png")))
> 175:             {

You do NOT want to write an image into the srcDir. In some cases it may not even be writable.
That would be a great place to get an image that comes along with the source but NOT for an image created on the fly. You should be using TESTCLASSES or the system property test.classes (verify the name of that - I am going from memory)

test/jdk/javax/swing/JButton/HtmlButtonImageTest/HtmlButtonImageTest.java line 179:

> 177:             }
> 178:         } catch (IOException e) {
> 179:             e.printStackTrace();

If this fails, your test fails, doesn't it ? Might as well propagate the exception and let the test exit  with the IOException ..

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

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