RFR: JDK-8015739: Background of JInternalFrame is located out of JInternalFrame [v4]
Alexey Ivanov
aivanov at openjdk.org
Mon Oct 3 21:56:54 UTC 2022
On Fri, 30 Sep 2022 00:19:46 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:
>> JInternalFrame background color seems to overflow into the border region. This issue is more prominently seen on Windows - Metal LAF (with fractional scaling, as shown below). The primary reason is border scaling issue as observed in - [JDK-8279614](https://bugs.openjdk.org/browse/JDK-8279614)
>>
>> The fix involves a similar approach as described here https://github.com/openjdk/jdk/pull/7449#issuecomment-1068218648. The test checks the midpoint and corners of borders to check if the internal frame's background color is located out of JInternalFrame.
>>
>> 
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
>
> removed redundant jtreg header
src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 32:
> 30:
> 31: import javax.swing.AbstractButton;
> 32: import javax.swing.ButtonModel;
I'm unsure if we all agree on the order of imports in client area, however, the common order is
import java.*;
// optional blank line
import javax.*;
// blank line
import sun.*; // other packages
// blank line
import static *; // if applicable
src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 313:
> 311:
> 312: // border and corner scaling
> 313: int scaledCorner = (int) Math.round(corner * at.getScaleX());
Just a suggestion, you can call this variable `corner` but make the constant above upper case `CORNER`. This way the old code would remain basically the same and continue using `corner` (which was previously a field).
src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 326:
> 324: g.drawLine( 0, 1, 0, height-1);
> 325: g.drawLine( width-1, 1, width-1, height-1);
> 326: g.drawLine( 1, height-1, width-1, height-1);
I suggest removing the space after the opening parenthesis.
There's usually a space on either side of binary operators, the `-` in this case.
I believe this is existing code which was just moved around, so maybe its style is not worth touching.
src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 343:
> 341: width-midPoint, height- scaledCorner); //right
> 342: g.drawLine(scaledCorner + 1, height-midPoint,
> 343: width- scaledCorner, height-midPoint); //bottom
This looks a bit weird to me. The old code used no spaces around the binary operators, the new code is inconsistent: some cases do, some don't, some have space only after the operator.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 74:
> 72: private static final int FRAME_SIZE = 300;
> 73: private static final int INTFRAME_SIZE = 200;
> 74: private static final int MIDPOINT = INTFRAME_SIZE/2;
Suggestion:
private static final int MIDPOINT = INTFRAME_SIZE / 2;
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 112:
> 110:
> 111: // Check Borders
> 112: SwingUtilities.invokeAndWait(() -> {
None of the `check*` are required to be run on EDT. They use `robot` to read pixels from the screen, it can be done on the main thread.
It's probably to call `jFrame.getBounds()`. However, you can pre-save the bounds of the frame in the block above.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 139:
> 137:
> 138: private static void checkBorderMidPoints(String borderDirection) {
> 139: int x = 0, y = 0, start = 0, stop = 0;
The initialisers are redundant.
It's not recommended to have several local variable declarations on one line.
When using two lines
int x, y;
int start, stop;
the coordinates and loop control variables are on separate lines, easier to parse.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 180:
> 178: isHorizontal ? i : (iFrameLoc.y + MIDPOINT)))) {
> 179: saveScreenCapture(borderDirection + "_" + uiScale + ".png");
> 180: errorLog.append("uiScale: "+ uiScale +
The body of `if` shouldn't be indented compared to the indentation of the condition, it should start at the regular four-column indent.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 181:
> 179: saveScreenCapture(borderDirection + "_" + uiScale + ".png");
> 180: errorLog.append("uiScale: "+ uiScale +
> 181: " Red background color" + " detected at "
Another nit:
Suggestion:
errorLog.append("uiScale: " + uiScale +
" Red background color detected at "
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 189:
> 187:
> 188: private static void checkCorners(String cornerLocation) {
> 189: int x, y = 0;
Suggestion:
int x, y;
Initialising `y` _only_ is redundant.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 211:
> 209: + cornerLocation);
> 210: }
> 211: robot.mouseMove(x, y);
Moving the mouse cursor is not required. However, it gives a visual feedback of where the check is happening.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 240:
> 238: }
> 239: // for debugging purpose, saves screen capture when test fails.
> 240: private static void saveScreenCapture(String filename) {
The comment is probably redundant. Yet a blank line between methods is recommended.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 241:
> 239: // for debugging purpose, saves screen capture when test fails.
> 240: private static void saveScreenCapture(String filename) {
> 241: BufferedImage image = robot.createScreenCapture(jFrame.getBounds());
I guess we lose pixel details here. If uiScale > 1, `createScreenCapture` scales down the captured image. Do we want to preserve the original image too? [`Robot.createMultiResolutionScreenCapture`](https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/java/awt/Robot.html#createMultiResolutionScreenCapture(java.awt.Rectangle)) can be useful.
test/jdk/javax/swing/JInternalFrame/InternalFrameBorderTest.java line 243:
> 241: BufferedImage image = robot.createScreenCapture(jFrame.getBounds());
> 242: try {
> 243: ImageIO.write(image,"png", new File(filename));
Suggestion:
ImageIO.write(image, "png", new File(filename));
-------------
PR: https://git.openjdk.org/jdk/pull/10274
More information about the client-libs-dev
mailing list