On Thu, 5 Jan 2023 19:03:17 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>> In this fix, the common code required for scaled border rendering is unified >> and added to SwingUtilities3. This is achieved by adding a functional >> interface to SwingUtilities3 and calling the the respective paintBorder >> function passed as functional parameter to the method. >> >> Following are the usual steps for any border scaling - >> >> - Resetting transform. >> - Calculating new width, height, x & y-translations. >> - Perform the required border rendering. >> - And at the end restore the previous transform. >> >> To test the refactored code, 3 separate border scaling instances were taken >> (details below) and the SwingUtilities3.paintBorder, (containing the common >> functionality) was applied. All the tests associated with the respective >> border changes pass. >> >> 1. EtchedBorder - [PR#7449](https://github.com/openjdk/jdk/pull/7449) - >> Test: ScaledEtchedBorderTest.java >> 2. LineBorder - [PR#10681](https://github.com/openjdk/jdk/pull/10681) - >> Test: ScaledLineBorderTest & ScaledTextFieldBorderTest.java >> 3. JInternalFrame Border - >> [PR#10274](https://github.com/openjdk/jdk/pull/10274) - Test: >> InternalFrameBorderTest.java >> >> The advantage of this solution is - it avoids code repetition and can be >> reused across all the border classes requiring border scaling fix. > > Harshitha Onkar has updated the pull request incrementally with one > additional commit since the last revision: > > moved strokeWidth logic to individual classes src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 46: > 44: import sun.awt.SunToolkit; > 45: > 46: One line is enough between regular imports and static ones. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 176: > 174: // or if scale=1, skip resetting the transform > 175: resetTransform = ((at.getShearX() == 0) && (at.getShearY() > == 0)) && > 176: ((at.getScaleX() > 1) || (at.getScaleY() > 1)); Suggestion: resetTransform = ((at.getShearX() == 0) && (at.getShearY() == 0)) && ((at.getScaleX() > 1) || (at.getScaleY() > 1)); The operator at the start of a line makes it clear it's an continuation line. This style was advocated in the Java Style Guidelines. I don't insist though. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 196: > 194: > 195: // Step 2: Call respective paintBorder with transformed values > 196: painter.paintUnscaledBorder(c, g, x, y, width, height, > scaleFactor); Why do you pass the original `x` and `y`? They must be 0 inside `paintUnscaledBorder`. As such, `x` and `y` parameters can be dropped. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 203: > 201: Graphics2D g2d = (Graphics2D) g; > 202: g2d.setTransform(at); > 203: g2d.setStroke(oldStk); The stroke should always be restored even if the transform wasn't reset. Or it should be done in the implementation of `paintUnscaledBorder`. src/java.desktop/share/classes/javax/swing/border/EtchedBorder.java line 156: > 154: > 155: private void paintUnscaledBorder(Component c, Graphics g, int x, int > y, > 156: int w, int h, double scaleFactor) { Suggestion: private void paintUnscaledBorder(Component c, Graphics g, int x, int y, int w, int h, double scaleFactor) { One more space is missing to align. src/java.desktop/share/classes/javax/swing/border/LineBorder.java line 27: > 25: package javax.swing.border; > 26: > 27: import com.sun.java.swing.SwingUtilities3; Let's be consistent: it should go after `java.*` and `javax.*` packages. Either there should be a blank line before `com.*` or `sun.*` imports or there shouldn't be. src/java.desktop/share/classes/javax/swing/border/LineBorder.java line 155: > 153: int offs = this.thickness * (int) scaleFactor; > 154: Color oldColor = g.getColor(); > 155: g.setColor(this.lineColor); I'd rather leave the lines unchanged. I mean `getColor` and `setColor` could still use `g2d` as before. src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 69: > 67: import static sun.java2d.pipe.Region.clipRound; > 68: > 69: It's probably unnecessary. ------------- PR: https://git.openjdk.org/jdk/pull/11571
