Skip to content

Conversation

@DamonGuy
Copy link
Contributor

@DamonGuy DamonGuy commented Feb 1, 2022

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8015854: [macosx] JButton's HTML ImageView adding unwanted padding

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7310/head:pull/7310
$ git checkout pull/7310

Update a local copy of the PR:
$ git checkout pull/7310
$ git pull https://git.openjdk.java.net/jdk pull/7310/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7310

View PR using the GUI difftool:
$ git pr show -t 7310

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7310.diff

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Feb 1, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 1, 2022

Hi @DamonGuy, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user DamonGuy" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@DamonGuy
Copy link
Contributor Author

DamonGuy commented Feb 1, 2022

/covered

@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Feb 1, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 1, 2022

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@openjdk
Copy link

openjdk bot commented Feb 1, 2022

@DamonGuy The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Feb 1, 2022
…ng antialiased pixels and added buffer for slight offsets due to different resolutions.
@DamonGuy DamonGuy changed the title 8015854: JButton with html image for text has unwanted padding on the left 8015854: [macosx] JButton's HTML ImageView adding unwanted padding Feb 1, 2022
@prsadhuk
Copy link
Contributor

prsadhuk commented Feb 2, 2022

The JBS says "This is reproducible with the Mac default (Aqua) look and feel; using another LAF (Metal for instance) seems to position the ImageView correctly." so it seems to be a mac issue not generic, so fixing in shared code will have other repurcussions..You probably need to fix in Aqua specific class..

@bridgekeeper bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Feb 2, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 2, 2022
@mlbridge
Copy link

mlbridge bot commented Feb 2, 2022

@mrserb
Copy link
Member

mrserb commented Feb 2, 2022

I did not check the whole fix, but as an initial thing, I suggest generating the image at runtime and do not commit it to the repo.

@DamonGuy
Copy link
Contributor Author

DamonGuy commented Feb 3, 2022

The JBS says "This is reproducible with the Mac default (Aqua) look and feel; using another LAF (Metal for instance) seems to position the ImageView correctly." so it seems to be a mac issue not generic, so fixing in shared code will have other repurcussions..You probably need to fix in Aqua specific class..

@prsadhuk Sorry, I forgot to mention in the JBS issue that while testing if the issue was reproducible locally, I discovered that the issue exists for Aqua LAF as described, but also other LAFs (such as Metal and Basic).

This discovery shifted my attention away from AquaButtonUI alone, and AquaButtonUI's layoutAndGetText method using alternative insets turned out to be where the issue was coming from. This method relies on "SwingUtilities.layoutCompoundLabel" to generate alternative insets, so that's where I made my changes. Is this logic valid or should I contain my changes in AquaButtonUI?

@prsadhuk
Copy link
Contributor

prsadhuk commented Feb 3, 2022

The JBS says "This is reproducible with the Mac default (Aqua) look and feel; using another LAF (Metal for instance) seems to position the ImageView correctly." so it seems to be a mac issue not generic, so fixing in shared code will have other repurcussions..You probably need to fix in Aqua specific class..

@prsadhuk Sorry, I forgot to mention in the JBS issue that while testing if the issue was reproducible locally, I discovered that the issue exists for Aqua LAF as described, but also other LAFs (such as Metal and Basic).

Then it would be better to iterate through all installed L&F in the testcase to check this fix.

@DamonGuy
Copy link
Contributor Author

DamonGuy commented Feb 4, 2022

I noticed I mistakenly left the thrown exception for failures commented out for the most recent push while testing the new changes. The new test with the run-time generated image and cycled LAFs seems to fail for Nimbus LAF, so I will fix this and push to the branch again.

if (v != null) {
textR.width = Math.min(availTextWidth,
(int) v.getPreferredSpan(View.X_AXIS));
if(availTextWidth < (int) v.getPreferredSpan(View.X_AXIS)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Images will be attached.

This code runs for html images as well as html text. I found that the extra padding issue exists for anything bound by the tags.

Syntax has been fixed locally and will appear in next commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prrace Also no space after (int) for the cast. lines 1129 and 1134 ..

I'd like to clarify what style is followed by the client code.

Code Conventions for the Java published in 1999 in its section for white-space state:

“Casts should be followed by a blank space.”

A more recent draft for Java Style that is commonly referred to during code reviews also says about using the space after the cast in its Horizontal Whitespace section:

“A single space should be used…
After the closing parenthesis of a cast.”

As far as I can see, neither style — no space after the cast vs a space after the cast — is followed consistently. Let's agree on one option and follow it then.

Shall I bring it up for a discussion on the client-libs mailing list?

@@ -0,0 +1,182 @@
/*
* Copyright 2022 JetBrains s.r.o.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

srcDir = Path.of(System.getProperty("test.src", "."));

// generate red_square.png image to use as JButton text
SwingUtilities.invokeAndWait(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cg.fillRect(0, 0, SQUARE_WIDTH, SQUARE_HEIGHT);
try {
if (ImageIO.write(bImg, "png", new File(srcDir + "/red_square.png")))
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

System.out.println("-- Saved Image");
}
} catch (IOException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// close frame when complete
SwingUtilities.invokeAndWait(() -> {
frame.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@prrace
Copy link
Contributor

prrace commented Feb 5, 2022

" 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.

…ved generate image out of EDT. Changed to only test on MacOS Aqua LAF. Changed file location for generated image. Exit test with IOException on failed image generation.
if (v != null) {
// use zero insets for view since layout only handles text calculations
text = layoutAndGetText(g, b, aquaBorder, new
Insets(0,0,0,0), viewRect, iconRect, textRect);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is only html text instead of image in JButton, will it not be a problem using 0 insets or is there a problem with text too being shifted like image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text was also unexpectedly shifted, so this zero inset fixes that issue as well. Setting the insets to zero for all HTML fixes the unwanted padding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create a separate test for HTML text? Probably manual only but still. If it is a known issue and it's also resolved by this fix, I find such a test useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can definitely try to do so. The newest version of the test is now headless using BufferedImage. By manual, do you mean making a headful test where the tester clicks pass or fail according to the position of HTML text on the button?

}

Robot robot = new Robot();
robot.setAutoDelay(2000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autodelay is normally set to 100. Dont think one would want to wait 2 secs between robot events.

SwingUtilities.invokeAndWait(() -> {
createAndShowGUI();
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally it is recommended to wait for a sec after frame is visible and before start of robot events.

// retrieve color of pixels at each edge of square image by starting at the center of the button
robot.mouseMove(frame.getLocationOnScreen().x, frame.getLocationOnScreen().y);
robot.mouseMove(button.getLocationOnScreen().x, button.getLocationOnScreen().y);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is no need of this staggard mouseMove. One can move straight to "point" as done below, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for mouseMove at all. However, it helps to analyse the image visually whether the red square is centered in the button or not.

// dispose frame when done testing for a LAF before continuing
SwingUtilities.invokeAndWait(() -> {
frame.dispose();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally we dispose frame in try-finally block

Color leftClr = robot.getPixelColor(point.x - (SQUARE_WIDTH/2) + PIXEL_BUFFER, point.y);
Color rightClr = robot.getPixelColor(point.x + (SQUARE_WIDTH/2) - PIXEL_BUFFER, point.y);
Color topClr = robot.getPixelColor(point.x, point.y - (SQUARE_HEIGHT/2) + PIXEL_BUFFER);
Color botClr = robot.getPixelColor(point.x, point.y + (SQUARE_HEIGHT/2) - PIXEL_BUFFER);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was an issue of using getPixelColor on retina screen on mac laptop as @honkar-jdk found out in #7219..She has to use BufferedImage..Are you sure this is working on all mac CI system, mac external monitor and mac laptop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I ran into this issue as well a few weeks ago when I made the test. My solution was to check for redness by checking for 250 or greater R values with 10 or less G and B values instead of checking for exactly 255,0,0. When tested in mach5, some Macs failed with values that had pixels that fit within this color range, but wasn't exactly 255,0,0.

@mlbridge
Copy link

mlbridge bot commented Feb 28, 2022

Mailing list message from Jeremy Wood on client-libs-dev:

Damon:

Sorry for joining this chat late. Is the core problem here that Swing is
positioning graphic elements incorrectly -- and those bad positions are
platform-independent?

If so: this might be a good candidate for me to try to rewrite a test to
avoid the Robot class. I have a Graphics2D implementation
<https://github.com/mickleness/pumpernickel/blob/master/src/main/java/com/pump/graphics/vector/VectorGraphics2D.java>
that catalogs rendering operations. If just the act of reviewing those
catalogued instructions can detect the underlying problem: this might be
a simpler/faster alternative. (I?m assuming tests written using the
Robot class are more expensive to run and more brittle.)

Does this sound like something that might benefit this test case? (I can
have a look at it in a few days.)

Regards,
- Jeremy


public final class HtmlButtonImageTest {
private static JFrame frame;
private static Point point;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

point must be volatile as it's accessed from two threads.

@aivanov-jdk
Copy link
Member

aivanov-jdk commented Mar 1, 2022

A general comment: You, @DamonGuy, have quite a few comments in the test. Most of these comments are redundant because they repeat what the code does. The code should be self-documenting by using meaningful method and variable names, and I think you follow this rule.

The problem with comments is that they may be left unmodified when the code changes. Thus, the comments become misleading, confusing and therefore don't help a reader understand the code but make the reader guess whether they should believe the comment or the code.

A couple of points from Programming Practices section from the Java Style Guide:

“First and foremost, try to make the code simple enough that it’s self explanatory. While comments explaining the code are good, not having to explain the code is better.”

“Avoid comments that run the risk of getting out of sync as the code evolves. (If a comment runs the risk of getting out of sync, it’s often a sign that it comments how the code works rather than what the code achieves.)”

@aivanov-jdk
Copy link
Member

Hi Jeremy,

Mailing list message from Jeremy Wood on client-libs-dev:

Damon:

Sorry for joining this chat late. Is the core problem here that Swing is positioning graphic elements incorrectly -- and those bad positions are platform-independent?

Yes, it looks that this issue is platform-independent and can be reproduced with other Look-and-Feels on other platforms.

If so: this might be a good candidate for me to try to rewrite a test to avoid the Robot class. I have a Graphics2D implementation https://github.com/mickleness/pumpernickel/blob/master/src/main/java/com/pump/graphics/vector/VectorGraphics2D.java that catalogs rendering operations. If just the act of reviewing those catalogued instructions can detect the underlying problem: this might be a simpler/faster alternative. (I?m assuming tests written using the Robot class are more expensive to run and more brittle.)

Does this sound like something that might benefit this test case? (I can have a look at it in a few days.)

Regards, - Jeremy

The code you reference cannot be used in OpenJDK until the author of the code contributes it to OpenJDK.

You're right headful tests are more expensive to run.

@prsadhuk mentioned that Robot.getPixelColor may produce an incorrect result which @honkar-jdk came across. In fact, @DamonGuy sees the artefacts too: on a High DPI display (and built-in Retina display of MacBook is a High DPI display), one pixel in user's space is represented with several physical pixels. In such a case, getPixelColor still returns only one pixel.

In Harshitha's #7219, the table header border had 1px width. The robot correctly captured the specified part of the screen but the line was drawn on the second row of the physical pixels.

Damon needs to account for antialiasing. The image needs to be scaled up for Retina display, its borders aren't sharp anymore.

Painting the JButton to BufferedImage could resolve these problems, the test could check for the red color without adding a tolerance. If there's no frame and button is painted to BufferedImage, the test could be made headless.

Perhaps, we should advise this approach when the test doesn't need to process any input.

@aivanov-jdk
Copy link
Member

The JBS says "This is reproducible with the Mac default (Aqua) look and feel; using another LAF (Metal for instance) seems to position the ImageView correctly." so it seems to be a mac issue not generic, so fixing in shared code will have other repurcussions..You probably need to fix in Aqua specific class..

@prsadhuk Sorry, I forgot to mention in the JBS issue that while testing if the issue was reproducible locally, I discovered that the issue exists for Aqua LAF as described, but also other LAFs (such as Metal and Basic).

This discovery shifted my attention away from AquaButtonUI alone, and AquaButtonUI's layoutAndGetText method using alternative insets turned out to be where the issue was coming from. This method relies on "SwingUtilities.layoutCompoundLabel" to generate alternative insets, so that's where I made my changes. Is this logic valid or should I contain my changes in AquaButtonUI?

Since this fix addresses AquaButtonUI only and we know for sure that other Look-and-Feels are also affected (Metal at least according to the comments in JBS and here), has anyone submitted a new bug for other LaFs?

@DamonGuy
Copy link
Contributor Author

DamonGuy commented Mar 1, 2022

I have updated the test to be headless by using BufferedImage.

There is no new bug submitted yet for other LAFs that I know of. I tested other LAFs and the issue does persist, so I can submit the bug for them if necessary. The other LAFs I tested were Metal, Nimbus, and Motif.

generateRedSquare();

SwingUtilities.invokeAndWait(HtmlButtonImageTest::createButton);
SwingUtilities.invokeAndWait(HtmlButtonImageTest::createImage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paintButton would be clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, createButton then paintButton :)

graphics2D.dispose();
}

private static boolean checkRedness(int rgb) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkRed or checkRedColor?

}
System.out.println("Passed");
}
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usually recommended to end the file with a new line.

import java.nio.file.Path;
import java.io.IOException;
import java.io.File;
import javax.imageio.ImageIO;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you sort the imports, please? Optimize Imports or a similar command in your IDE should do the magic.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than reversed method names to create a button and then paint the button into an image.

generateRedSquare();

SwingUtilities.invokeAndWait(HtmlButtonImageTest::createButton);
SwingUtilities.invokeAndWait(HtmlButtonImageTest::createImage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, createButton then paintButton :)

@DamonGuy
Copy link
Contributor Author

DamonGuy commented Mar 4, 2022

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 4, 2022
@openjdk
Copy link

openjdk bot commented Mar 4, 2022

@DamonGuy
Your change (at version 93f19ca) is now ready to be sponsored by a Committer.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Mar 4, 2022

Going to push as commit 9c817d3.
Since your change was applied there have been 809 commits pushed to the master branch:

  • 733c790: 8282081: java.time.DateTimeFormatter: wrong definition of symbol F
  • f9f9c0a: 8252769: Warn in configure if git config autocrlf has invalid value
  • 603050b: 8282661: [BACKOUT] ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y)
  • 5247153: 8282615: G1: Fix some includes
  • a584c90: 8282573: ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y)
  • d5e8e52: 8282532: Allow explicitly setting build platform alongside --openjdk-target
  • b383780: 8282343: Create a regression test for JDK-4518432
  • b629782: 8279886: C1: Turn off SelectivePhiFunctions in presence of irreducible loops
  • 7e1c67d: 8282608: RawNativeLibraryImpl can't be passed to NativeLibraries::findEntry0
  • 8478173: 8282583: Update BCEL md to include the copyright notice
  • ... and 799 more: https://git.openjdk.java.net/jdk/compare/022e4f0f1c4862315b34595d6df228a49f67cb2e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 4, 2022
@openjdk openjdk bot closed this Mar 4, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 4, 2022
@openjdk
Copy link

openjdk bot commented Mar 4, 2022

@aivanov-jdk @DamonGuy Pushed as commit 9c817d3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@aivanov-jdk
Copy link
Member

There is no new bug submitted yet for other LAFs that I know of. I tested other LAFs and the issue does persist, so I can submit the bug for them if necessary. The other LAFs I tested were Metal, Nimbus, and Motif.

Please, submit a new bug so that we don't forget about this issue in other LAFs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client client-libs-dev@openjdk.org integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

5 participants