Skip to content
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

8279614: The left line of the TitledBorder is not painted on 150 scale factor #7449

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
c368173
initial commit
Feb 8, 2022
6e24762
finished test
Feb 9, 2022
b2e9240
8279614: The left line of the TitledBorder is not painted on 150 scal…
Feb 11, 2022
4c8d5bf
changed carriage return
Feb 14, 2022
f314462
changed carriage return
Feb 14, 2022
c0b3189
trailing whitespace, add extra newline at eof
Feb 14, 2022
47437d9
reverted old change, swapped order of painting to prevent overdrawing
Mar 3, 2022
88c1b1a
typo
Mar 3, 2022
6056faf
adjusted pixels to check for border
Mar 3, 2022
7b7732f
added functions for drawing border, fixed translate
Mar 14, 2022
06ac18d
changed approach to render border without transforms
Mar 17, 2022
3ecaf58
updated test
Mar 17, 2022
75ec24d
fixed apostrophe in comment of saveImage
Mar 17, 2022
9109b84
scale stroke width at higher scalings
Mar 23, 2022
5d4854d
fixed 1.25 scaling overdraw, fixed calcs for right and bottom side bo…
Apr 7, 2022
4dc1287
forgot to change starting x value of bottom line
Apr 20, 2022
6c2efaf
updated test
May 2, 2022
a793582
saved translation values in EtchedBorder, updated test
May 5, 2022
89e64e9
fixed bottom edge border starting x value, updated test
May 6, 2022
a50bda0
fixed coordinate checks
May 6, 2022
6767407
changed error message for gap between highlight and shadow
May 6, 2022
e7b2706
made changes according to comments
May 9, 2022
b84cf51
made suggested changes
May 9, 2022
e32e6c0
renamed test, renamed some methods, updated error messages, updated test
May 10, 2022
b17cdab
made suggested changes
May 20, 2022
65200a2
reverted copyright year
May 20, 2022
61fc0ae
updated test
alisenchung May 25, 2022
935ec43
changed test to headless
alisenchung May 25, 2022
cf91eb8
updated test
alisenchung May 31, 2022
1d6ac07
don't reset transform if it contains a non-scale transformation
alisenchung Jun 3, 2022
8f71aba
skip transform is m01 or m10 is nonzero
alisenchung Jun 6, 2022
adca6ef
made suggested changes
alisenchung Jun 8, 2022
24d7cee
fixed spacing
alisenchung Jun 8, 2022
325d09b
fixed declarations
alisenchung Jun 8, 2022
35e7d5a
removed rendering hints, changed condition to reapply old transform
alisenchung Jun 8, 2022
317fe35
removed initialization
alisenchung Jun 10, 2022
93f0b73
changed initializations to null
alisenchung Jun 10, 2022
610d8f8
fix typo
alisenchung Jun 10, 2022
d4ad493
removed unused import
alisenchung Jun 13, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -119,6 +119,20 @@ public EtchedBorder(int etchType, Color highlight, Color shadow) {
this.shadow = shadow;
}

private void paintBorderRect(Graphics g, Color c, int w, int h) {
g.setColor(c);
g.drawRect(0, 0, w-2, h-2);
}

private void paintBorderShadow(Graphics g, Color c, int w, int h) {
g.setColor(c);
g.drawLine(1, h-3, 1, 1);
g.drawLine(1, 1, w-3, 1);

g.drawLine(0, h-1, w-1, h-1);
g.drawLine(w-1, h-1, w-1, 0);
}

/**
* Paints the border for the specified component with the
* specified position and size.
Expand All @@ -136,15 +150,16 @@ public void paintBorder(Component c, Graphics g, int x, int y, int width, int he

g.translate(x, y);

g.setColor(etchType == LOWERED? getShadowColor(c) : getHighlightColor(c));
g.drawRect(0, 0, w-2, h-2);

g.setColor(etchType == LOWERED? getHighlightColor(c) : getShadowColor(c));
g.drawLine(1, h-3, 1, 1);
g.drawLine(1, 1, w-3, 1);
// 8279614: The dark line was being overdrawn by the light line. The fix was to
// make sure that the dark line is always drawn second.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 8279614: The dark line was being overdrawn by the light line. The fix was to
// make sure that the dark line is always drawn second.
// 8279614: The dark line was overdrawn by the light line.
// The fix makes sure that the dark line is always drawn second.

Does the opposite never happen? That the shadow overdraws the highlight.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a comment on the actual comment rather than whether the intent is correct.
I'm not in favour of putting bug ids into the comments. Our code will end up utterly cluttered with them
Also it is phrased to describe a fix.
So IF it were to stand as is, I would have phrased it as "what we are doing now and why", ie
// Make sure the shadow line always is drawn second because it needs to over draw the border.

Copy link
Member

Choose a reason for hiding this comment

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

I should've posted my question separately rather than as a comment to the comment.

if (etchType == LOWERED) {
alisenchung marked this conversation as resolved.
Show resolved Hide resolved
paintBorderShadow(g, getHighlightColor(c), w, h);
paintBorderRect(g, getShadowColor(c), w, h);
} else {
paintBorderRect(g, getHighlightColor(c), w, h);
paintBorderShadow(g, getShadowColor(c), w, h);
}
alisenchung marked this conversation as resolved.
Show resolved Hide resolved

g.drawLine(0, h-1, w-1, h-1);
g.drawLine(w-1, h-1, w-1, 0);

g.translate(-x, -y);
}
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -33,6 +33,7 @@
import java.awt.Insets;
import java.awt.Rectangle;
import java.awt.geom.Path2D;
import java.awt.geom.Rectangle2D;
Copy link
Member

Choose a reason for hiding this comment

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

This import isn't used. There are no other changes to this file but this added import.

In addition to it, you can also remove java.beans.PropertyChangeEvent from imports which is unused as well.

Copy link
Member

Choose a reason for hiding this comment

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

Either revert the changes to TitledBorder or remove the two unused imports: java.awt.geom.Rectangle2D and java.beans.PropertyChangeEvent.

import java.beans.ConstructorProperties;
import java.beans.PropertyChangeEvent;
import java.beans.PropertyChangeListener;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is removing an unused import the only change to TitledBorder ? I think you should revert this to make the change cleaner.

Expand Down
131 changes: 131 additions & 0 deletions test/jdk/java/awt/TitledBorder/TitledBorderTest.java
@@ -0,0 +1,131 @@
/*
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import javax.imageio.ImageIO;
import javax.swing.BorderFactory;
import javax.swing.JCheckBox;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.SwingUtilities;
import javax.swing.UIManager;
import javax.swing.UIManager.LookAndFeelInfo;
import javax.swing.UnsupportedLookAndFeelException;
import java.awt.BorderLayout;
import java.awt.Graphics2D;
import java.awt.Robot;
import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;

/*
* @test
* @key headful
* @bug 8279614
* @summary The left line of the TitledBorder is not painted on 150 scale factor
* @requires (os.family == "windows")
Copy link
Member

Choose a reason for hiding this comment

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

The test is not Windows specific, is it? Yet Java supports fractional scales on Windows only at the moment, right?

I still think it makes sense to run the test on Linux and macOS for 100%, 200% and 300% which are supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well the test is setting the Windows L&F, and evaluating its rendering.
Which seems to me to make it windows-specific, as written.
The test would would need to run through all the L&Fs if it is actually valid to do so.

Copy link
Member

Choose a reason for hiding this comment

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

Well the test is setting the Windows L&F, and evaluating its rendering. Which seems to me to make it windows-specific, as written. The test would would need to run through all the L&Fs if it is actually valid to do so.

Right, the bug was reported on Windows. The Windows L&F uses EtchedBorder for TitledBorder whereas other L&F don't. From this point of view, the bug is Windows-specific.

Yet the fix is in the shared code, in the javax.swing.border.EtchedBorder class which is not Look-and-Feel specific. I'm sure the problem can be reproduced if EtchedBorder is used directly rather than via TitledBorder. Yet you wouldn't see the bug on Linux and macOS because these two platforms support only integer scales, but the issue occurs with fractional scales only.

If the test uses EtchedBorder directly, there'll be no need to iterate L&Fs. The test sets EtchedBorder border to a panel, the panel renders the border.

* In fact, Windows 10 renders titled border flat: one grey line instead of etched border that was used in previous versions. Shall we update Swing's Windows L&F?

* @run main TitledBorderTest
*/
public class TitledBorderTest {

public static JFrame frame;
public static JPanel parentPanel;
public static JPanel childPanel;
Copy link
Member

Choose a reason for hiding this comment

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

Please correct the indentation in the test: each level is indented by four spaces.


public static void main(String[] args) throws Exception {
LookAndFeelInfo laf = UIManager.getInstalledLookAndFeels()[3];
Copy link
Member

Choose a reason for hiding this comment

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

Is the test needs to be run in a specific LookAndFeel? If so, you have to set explicitly rather than rely on the order of LAFs.

System.out.println(laf);
SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf));
SwingUtilities.invokeAndWait(() -> createAndShowGUI());

Robot robot = new Robot();

BufferedImage buff = new BufferedImage(frame.getWidth()*2, frame.getHeight()*2,
BufferedImage.TYPE_INT_ARGB);
Graphics2D graph = buff.createGraphics();
graph.scale(1.5, 1.5);
frame.paint(graph);
graph.dispose();
Copy link
Member

Choose a reason for hiding this comment

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

If we paint to BufferedImage, it makes sense to bypass creating and showing the frame, a panel with TitledBorder will be enough. In this case, the test can be headless.

Shall we run the test with a set of scales: 1.00, 1.25, 1.50, 1.75, 2.00?

Without the fix, the left line looks clipped at 2.0 scale too: the shadow is 1-pixel wide on the left whereas the highlight is 2-pixel wide on the four edges.


robot.waitForIdle();
boolean testFail = true;
for (int i = 15; i < 25 && testFail == true; i++) {
for (int j = 80; j < 100; j++) {
if (buff.getRGB(i, j) == -6250336) {
Copy link
Member

Choose a reason for hiding this comment

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

Using hex for colors is better.

Getting the highlight or shadow color from the border would make the test more generic. If the color used by border changes, the test will fail but it shouldn't.

System.out.println(i + " " + j + " Color " + buff.getRGB(i, j));
testFail = false;
break;
}
}
}

if (testFail) {
saveImage(buff, "test.png");
throw new RuntimeException("Border was clipped or overdrawn.");
}

frame.dispose();
}

private static void createAndShowGUI() {
frame = new JFrame("Swing Test");
frame.setSize(new java.awt.Dimension(300, 200));
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

parentPanel = new JPanel(new BorderLayout());
parentPanel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));

childPanel = new JPanel(new BorderLayout());
childPanel.setBorder(BorderFactory.createTitledBorder("Title"));
childPanel.add(new JCheckBox(), BorderLayout.CENTER);

parentPanel.add(childPanel, BorderLayout.CENTER);

frame.getContentPane().add(parentPanel, BorderLayout.CENTER);

frame.pack();
frame.setLocationRelativeTo(null);
frame.setVisible(true);
}

private static void saveImage(BufferedImage image, String filename) {
try {
ImageIO.write(image, "png", new File(filename));
} catch (IOException e) {
// Don’t propagate the exception
Copy link
Member

Choose a reason for hiding this comment

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

I recommend using a regular apostrophe (') instead of typographic one (’). It shouldn't cause issues with compiling because it's in the comment, yet the character may be displayed incorrectly if the default system encoding doesn't match the encoding of the file.

e.printStackTrace();
}
}

private static void setLookAndFeel(UIManager.LookAndFeelInfo laf) {
try {
UIManager.setLookAndFeel(laf.getClassName());
System.out.println(laf.getName());
} catch (UnsupportedLookAndFeelException ignored){
System.out.println("Unsupported LookAndFeel: " + laf.getClassName());
} catch (ClassNotFoundException | InstantiationException |
IllegalAccessException e) {
throw new RuntimeException(e);
}
}
}