On Wed, 21 Dec 2022 22:09:37 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 parameter to 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 with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - minor changes > - Merge branch 'master' into RefactorBorder_8294484 > - Merge branch 'master' into RefactorBorder_8294484 > - review changes > - Refactoring changes > - Merge branch 'master' into RefactorBorder_8294484 > - revert MetalBorder changes > - merge master > - refactoring changes - initial commit Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 151: > 149: void paintUnscaledBorder(Component c, Graphics g, int x, int y, > 150: int w, int h, double scale, int > strokeWidth); > 151: } I think the word _interface_ is redundant in the type name. `UnscaledBorderPainter` is a better name which is more specific and is consistent with Java naming conventions. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 154: > 152: > 153: public static void paintBorder(Component c, Graphics g, int x, int y, > 154: int w, int h, PaintInterface paintFunction) { `painter` could be a better name than `paintFunction`. Even though a method reference is used as the parameter, here it's still an object that implements the specified interface. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 157: > 155: > 156: //STEP 1: RESET TRANSFORM > 157: //save old values Comments usually have a space after the start of comment delimiter. Suggestion: // Step 1: Reset Transform // Save old values Avoid capitals, text in all capitals is harder to read. I suggest adding a blank line between the two comment lines to separate the section header from a comment which goes to a shorter block of code. Or rather remove the second line, this should be self-explanatory. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 173: > 171: * pixel coordinate system instead of the logical coordinate > system. > 172: */ > 173: resetTransform = ((at.getShearX() == 0) && (at.getShearY() > == 0)); @rajamah's code in `LineBorder.java` used a better condition: if the scale is 1, resetting transform can be safely skipped. https://github.com/openjdk/jdk/blob/9911405e543dbe07767808bad88534abbcc03c5a/src/java.desktop/share/classes/javax/swing/border/LineBorder.java#L152-L156 src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 179: > 177: stkWidth = c instanceof JInternalFrame ? > 178: clipRound(scaleFactor) : (int) > Math.floor(scaleFactor); > 179: g2d.setStroke(new BasicStroke((float) stkWidth)); This is supposed to be a generic helper. If `strokeWidth` depends on the type of component, it's better to handle it in each component separately. `EtchedBorder` uses the stoke, `LineBorder` doesn't use it; however, it uses the value. So, my suggestion is to calculate the `strokeWidth` (can't we spell it out, it's easier to read this way, isn't it?) as `Math.floor(scaleFactor)`. The stroke itself isn't created, nor is set here. The stroke can still be restored here as a convenience. What do others think? src/java.desktop/share/classes/javax/swing/border/EtchedBorder.java line 27: > 25: package javax.swing.border; > 26: > 27: import com.sun.java.swing.SwingUtilities3; In JDK code, the convention is to put `com.*` and `sun.*` packages after the standard public API ones under `java.*` and `javax.*`. src/java.desktop/share/classes/javax/swing/border/EtchedBorder.java line 159: > 157: paintBorderShadow(g, (etchType == LOWERED) ? getHighlightColor(c) > 158: : getShadowColor(c), > 159: w, h, stroke); Perhaps, you can preserve the name of the parameter, this will reduce the number of different lines as well as use consistent name in the source file: both `paintBorderShadow` and `paintBorderHighlight` use `stkWidth` as the name for this parameter. src/java.desktop/share/classes/javax/swing/border/LineBorder.java line 151: > 149: private void paintUnscaledBorder(Component c, Graphics g, int x, int > y, > 150: int w, int h, double scale, int > stroke) { > 151: if (this.thickness > 0) { I believe this condition should remain the same: if ((this.thickness > 0) && (g instanceof Graphics2D)) { After all, the border will not be painted if `g` isn't a `Graphics2D`, as such it makes sense to skip calculating the width and paths. If you like, pattern matching for `instanceof` can still be used. I'm pretty sure @rajamah didn't use it to ease backporting. src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 274: > 272: // Draw the bulk of the border > 273: for (int i = 0; i <= thickness; i++) { > 274: g.drawRect(i, i, w - (i * 2), h - (i * 2)); The keep the old code intact, use `width` and `height` as parameter names in `paintUnscaledBorder` method. The same is true for `scaleFactor` above. ------------- PR: https://git.openjdk.org/jdk/pull/11571
