-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8224261: JProgressBar always with border painted around it #16467
Conversation
👋 Welcome back abhiscxk! A progress list of the required criteria for merging this PR into |
@kumarabhi006 The following label will be automatically applied to this pull request:
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. |
Webrevs
|
}); | ||
|
||
BufferedImage borderPaintedImg = | ||
robot.createScreenCapture(new Rectangle(pt.x, pt.y, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test can be simplified by rendering the progress bar into the buffered image directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't understand. Can you please explain a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to add the progress bar to the frame and then capture the pixels by the robot. you can render content of the progress bar directly to the BufferedImage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of tests which do this, scaled borders are those that come to my mind first:
jdk/test/jdk/javax/swing/border/EtchedBorder/ScaledEtchedBorderTest.java
Lines 300 to 311 in 1d96886
private static void paintToImages(final JComponent content, | |
final boolean saveImages) { | |
for (double scaling : scales) { | |
BufferedImage image = | |
new BufferedImage((int) Math.ceil(content.getWidth() * scaling), | |
(int) Math.ceil(content.getHeight() * scaling), | |
BufferedImage.TYPE_INT_ARGB); | |
Graphics2D g2d = image.createGraphics(); | |
g2d.scale(scaling, scaling); | |
content.paint(g2d); | |
g2d.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example:
jdk/test/jdk/javax/swing/text/FlowView/6318524/bug6318524.java
Lines 176 to 185 in b7425b6
private static void paintToImage(final int step, boolean saveImage) { | |
BufferedImage im = new BufferedImage(bounds.width, bounds.height, | |
TYPE_INT_RGB); | |
Graphics g = im.getGraphics(); | |
textPane.paint(g); | |
g.dispose(); | |
if (saveImage) { | |
saveImage(im, String.format("%02d.png", step)); | |
} | |
} |
Here textPane
component is painted to the BufferedImage
. Use the image to analyse rendering.
(In many cases, such a test can run headless.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrserb @aivanov-jdk Updated the test to render component to BufferedImage
directly.
private static void createAndShowUI() { | ||
frame = new JFrame("Test JProgressBar Border"); | ||
JPanel p = new JPanel(new FlowLayout()); | ||
progressBar = new JProgressBar(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progressBar = new JProgressBar(); | |
progressBar = new JProgressBar(); |
* @summary Verifies if JProgressBar border is painted even though border | ||
* painting is set to false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @summary Verifies if JProgressBar border is painted even though border | |
* painting is set to false | |
* @summary Verifies JProgressBar border is not painted when border | |
* painting is set to false |
if (laf.getName().contains("Nimbus") || laf.getName().contains("GTK")) { | ||
System.out.println("Testing LAF: " + laf.getName()); | ||
SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf)); | ||
} else { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest inverting the condition to skip other L&Fs:
if (laf.getName().contains("Nimbus") || laf.getName().contains("GTK")) { | |
System.out.println("Testing LAF: " + laf.getName()); | |
SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf)); | |
} else { | |
continue; | |
} | |
if (!laf.getName().contains("Nimbus") && !laf.getName().contains("GTK")) { | |
continue; | |
} | |
System.out.println("Testing LAF: " + laf.getName()); | |
SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf)); | |
* not equal, method returns true; false otherwise. | ||
*/ | ||
|
||
private static boolean compareImage(BufferedImage img1, BufferedImage img2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the method from regtesthelpers/Util
:
jdk/test/jdk/javax/swing/regtesthelpers/Util.java
Lines 59 to 80 in 9faead1
/** | |
* Compares two bufferedImages pixel-by-pixel. | |
* return true if all pixels in the two areas are identical | |
*/ | |
public static boolean compareBufferedImages(BufferedImage bufferedImage0, BufferedImage bufferedImage1) { | |
int width = bufferedImage0.getWidth(); | |
int height = bufferedImage0.getHeight(); | |
if (width != bufferedImage1.getWidth() || height != bufferedImage1.getHeight()) { | |
return false; | |
} | |
for (int y = 0; y < height; y++) { | |
for (int x = 0; x < width; x++) { | |
if (bufferedImage0.getRGB(x, y) != bufferedImage1.getRGB(x, y)) { | |
return false; | |
} | |
} | |
} | |
return true; | |
} |
If you don't want to use, I suggest reversing the conditions so that compareImage
returns true
when images are the same — it's more common and therefore less confusing. In addition to that, return quickly if the sizes are different (can they ever be?) and avoid nesting the for
-loops inside the if
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test to use the method from regtesthelpers/Util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look logical and work as expected.
|
||
private static void createAndShowUI() { | ||
progressBar = new JProgressBar(); | ||
progressBar.setSize(100,50); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progressBar.setSize(100,50); | |
progressBar.setSize(100, 50); |
private static volatile boolean isImgSame; | ||
private static BufferedImage borderPaintedImg; | ||
private static BufferedImage borderNotPaintedImg; | ||
|
||
public static void main(String[] args) throws Exception { | ||
for (UIManager.LookAndFeelInfo laf : | ||
UIManager.getInstalledLookAndFeels()) { | ||
if (!laf.getName().contains("Nimbus") && !laf.getName().contains("GTK")) { | ||
continue; | ||
} | ||
System.out.println("Testing LAF: " + laf.getName()); | ||
SwingUtilities.invokeAndWait(() -> { | ||
setLookAndFeel(laf); | ||
createAndShowUI(); | ||
}); | ||
|
||
borderPaintedImg = paintToImage(progressBar); | ||
progressBar.setBorderPainted(false); | ||
borderNotPaintedImg = paintToImage(progressBar); | ||
isImgSame = Util.compareBufferedImages(borderPaintedImg, borderNotPaintedImg); | ||
|
||
if (isImgSame) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth renaming isImgSame
to something that reads slightly smoother like isImgEqual
, isEqual
, or isMatching
. Could just even inser the Util.compareBufferedImages()
into the if statement for the failure check if you prefer. But I like the variable for better readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to isImgEqual
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatically, it should be areImgsEqual
or areImagesEqual
.
Would boolean equal = Util.compareBufferedImages
be enough to convey the meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, equal should be ok.
/* | ||
* @test | ||
* @bug 8224261 | ||
* @key headful | ||
* @library ../regtesthelpers | ||
* @build Util | ||
* @summary Verifies JProgressBar border is not painted when border | ||
* painting is set to false | ||
* @run main TestProgressBarBorder | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the tags closer to the class declaration, after the imports, please?
When the tags are before the class, they're not collapsed along with the copyright header when viewed in an IDE, which makes them easily visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
SwingUtilities.invokeAndWait(() -> { | ||
setLookAndFeel(laf); | ||
createAndShowUI(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do everything on EDT or don't bother with EDT: mixing it in this way isn't good.
In this case, it's safe to call the methods you use on the main thread. However, I heard that there's a convention to run all the code on EDT. Whatever you choose… but at least don't mix the threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
private static JProgressBar progressBar; | ||
private static volatile boolean isImgEqual; | ||
private static BufferedImage borderPaintedImg; | ||
private static BufferedImage borderNotPaintedImg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these could be local variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean
variable is changed to local variable. Others are used in EDT and other method, so kept it as class variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean
variable is changed to local variable. Others are used in EDT and other method, so kept it as class variables.
Do they need to be? I mean you can do everything on EDT, even throw the exception from there. It could something like this:
public class TestProgressBarBorder {
public static void main(String[] args) throws Exception {
for (UIManager.LookAndFeelInfo laf :
UIManager.getInstalledLookAndFeels()) {
if (!laf.getName().contains("Nimbus") && !laf.getName().contains("GTK")) {
continue;
}
System.out.println("Testing LAF: " + laf.getName());
SwingUtilities.invokeAndWait(() -> test(laf));
}
}
private static void test(UIManager.LookAndFeelInfo laf) {
setLookAndFeel(laf);
JProgressBar progressBar = createProgressBar();
progressBar.setBorderPainted(true);
BufferedImage withBorder = paintToImage(progressBar);
progressBar.setBorderPainted(false);
BufferedImage withoutBorder = paintToImage(progressBar);
boolean equal = Util.compareBufferedImages(withBorder, withoutBorder);
if (equal) {
try {
ImageIO.write(withBorder, "png", new File("withBorder.png"));
ImageIO.write(withoutBorder, "png", new File("withoutBorder.png"));
} catch (IOException ignored) {}
throw new RuntimeException("JProgressBar border is painted when border\n" +
" painting is set to false");
}
}
private static JProgressBar createProgressBar() {
JProgressBar progressBar = new JProgressBar();
progressBar.setSize(100, 50);
progressBar.setValue(0);
progressBar.setStringPainted(true);
return progressBar;
}
Because the exception is thrown from EDT, the exception thrown from main is InvocationTargetException
, yet the CI will still show you the cause. If you want to avoid it, you can return the boolean value from the test
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do they need to be? I mean you can do everything on EDT, even throw the exception from there.
Yeah, everything can be done on EDT but what is the advantage of that?
I mean as per current test, swing UI components are accessed on EDT and then the comparison is done on main thread.
Curious to know what we achieve extra by doing the way you suggested above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do they need to be? I mean you can do everything on EDT, even throw the exception from there.
Yeah, everything can be done on EDT but what is the advantage of that?
Less code, all the variables are local in one method, no need to care about any synchronisation whatsoever.
I mean as per current test, swing UI components are accessed on EDT and then the comparison is done on main thread.
Curious to know what we achieve extra by doing the way you suggested above?
What do we achieve by separating painting and comparing?
There's no only right way. Both do the same. Yet a single test-method which does everything is easier to understand: set look-and-feel, get a progress bar, paint it with and without border and, finally, compare the images. I'm always for this style.
When dealing with multiple threads you have to pass objects between threads; to do so, you store them as static fields — it somewhat complicates the data flow.
However, there's still one advantage to this — a clearer exception about the failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Updated.
borderPaintedImg = paintToImage(progressBar); | ||
progressBar.setBorderPainted(false); | ||
borderNotPaintedImg = paintToImage(progressBar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to call progressBar.setBorderPainted(true)
explicitly—this way the test will not depend on the default value. If whatever reason the default value changes, the test will become useless… it will be testing with isBorderPainted == false
in both cases.
You're setting it in createAndShowUI
; I still think it's better to move the call to progressBar.setBorderPainted(true)
here.
Aren't withBorder
and withoutBorder
more succinct names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
} | ||
} | ||
|
||
private static void createAndShowUI() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't show the UI anymore, createProgressBar
or initProgressBar
will be a better name.
private static void createAndShowUI() { | ||
progressBar = new JProgressBar(); | ||
progressBar.setSize(100, 50); | ||
// set initial value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is redundant, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.. updated.
throw new RuntimeException("JProgressBar border is painted when border\n" + | ||
" painting is set to false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new RuntimeException("JProgressBar border is painted when border\n" + | |
" painting is set to false"); | |
throw new RuntimeException("JProgressBar border is painted when border " + | |
"painting is set to false"); |
You should probably remove the line break from the error message.
@kumarabhi006 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 530 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@DamonGuy Can you please re-review the latest changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional changes look good to me. Cleaner and more readable
import java.io.File; | ||
import java.io.IOException; | ||
import java.lang.reflect.InvocationTargetException; | ||
import java.awt.Graphics; | ||
import java.awt.image.BufferedImage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDEA at my end disagrees with the current sorting order:
import java.io.File; | |
import java.io.IOException; | |
import java.lang.reflect.InvocationTargetException; | |
import java.awt.Graphics; | |
import java.awt.image.BufferedImage; | |
import java.awt.Graphics; | |
import java.awt.image.BufferedImage; | |
import java.io.File; | |
import java.io.IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the sorting order is not correct but I received a comment on my other PR #16674 (comment) which suggests it is long standing convention that the "core" packages (easily distinguished these days as those in the java.base module) are listed before the desktop / AWT / Swing ones.
That's the reason I didn't sorted the imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the sorting order is not correct but I received a comment on my other PR #16674 (comment) which suggests
it is long standing convention that the "core" packages (easily distinguished these days as those in the java.base module) are listed before the desktop / AWT / Swing ones.
That's the reason I didn't sorted the imports.
Let us discuss it there. This is news to me. I am for sorting the imports alphabetically — an IDE does it perfectly and inserts new imports into the correct place, managing the list of imports manually is tedious and error-prone.
public static void main(String[] args) throws InvocationTargetException, | ||
InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static void main(String[] args) throws InvocationTargetException, | |
InterruptedException { | |
public static void main(String[] args) throws Exception { |
In a test we usually don't care about checked exception, therefore throws Exception
is perfectly fine — it's what you usually see instead of the pair of InvocationTargetException
and InterruptedException
thrown by invokeAndWait
.
try { | ||
ImageIO.write(withBorder, "png", new File("withBorder.png")); | ||
ImageIO.write(withoutBorder, "png", new File("withoutBorder.png")); | ||
} catch (IOException e) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} catch (IOException e) {} | |
} catch (IOException ignored) {} |
Naming the exception parameter ignored
lets IDE know the exception is ignored, which removes the warning about ignoring the exception.
(I used the name ignored
in the snippet I pasted specifically for this reason.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed somehow in previous commit. Updated now.
ImageIO.write(withBorder, "png", new File("withBorder.png")); | ||
ImageIO.write(withoutBorder, "png", new File("withoutBorder.png")); | ||
} catch (IOException e) {} | ||
throw new RuntimeException("JProgressBar border is painted when border " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a blank line before the throw
statement, it'll stand out better from the preceding code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I would sort the imports though.
/dummy |
@kumarabhi006 Unknown command |
/integrate |
Going to push as commit cb95e39.
Your commit was automatically rebased without conflicts. |
@kumarabhi006 Pushed as commit cb95e39. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi @rubyFeedback, thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 rubyFeedback for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
JProgressBar is always painted with border irrespective of the value set via the API
setBorderPainted(boolean value)
in Synth (Nimbus and GTK) LAF. Proposed fix is to add a check before painting the component.CI jobs are green after the fix. Links attached to JBS.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16467/head:pull/16467
$ git checkout pull/16467
Update a local copy of the PR:
$ git checkout pull/16467
$ git pull https://git.openjdk.org/jdk.git pull/16467/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16467
View PR using the GUI difftool:
$ git pr show -t 16467
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16467.diff
Webrev
Link to Webrev Comment