On Fri, 30 Sep 2022 00:19:46 GMT, Harshitha Onkar <hon...@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
