On Wed, 11 Jan 2023 23:20:02 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 with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - removed unused parameters, old stroke changes > - Merge branch 'master' into RefactorBorder_8294484 > - moved strokeWidth logic to individual classes > - review changes > - Revert "test changes" > > This reverts commit abed51bd420941d8efa7b779b86257978f56810e. > - test changes > - minor changes > - Merge branch 'master' into RefactorBorder_8294484 > - Merge branch 'master' into RefactorBorder_8294484 > - review changes > - ... and 5 more: https://git.openjdk.org/jdk/compare/43847c43...9caca6b2 Looks good to me. src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 173: > 171: > 172: // if m01 or m10 is non-zero, then there is a rotation or > shear > 173: // or if scale=1, skip resetting the transform Suggestion: // if m01 or m10 is non-zero, then there is a rotation or shear, // or if scale=1, skip resetting the transform in these cases The added comma separates the two sentences. The added “in these cases” clarifies that we don't reset in either of the two cases: m01 or m10 is non-zero or scale is 1. src/java.desktop/share/classes/javax/swing/border/EtchedBorder.java line 161: > 159: if (g instanceof Graphics2D) { > 160: Graphics2D g2d = (Graphics2D) g; > 161: g2d.setStroke(new BasicStroke((float) stkWidth)); There's only one statement left inside the `if` block, shall get rid of another local variable? ((Graphics2D) g).setStroke(new BasicStroke((float) stkWidth)); src/java.desktop/share/classes/javax/swing/border/LineBorder.java line 153: > 151: if ((this.thickness > 0) && (g instanceof Graphics2D)) { > 152: Graphics2D g2d = (Graphics2D) g; > 153: int offs = this.thickness * (int) scaleFactor; It looks good, however, I suggest re-arranging the code a bit: Graphics2D g2d = (Graphics2D) g; Color oldColor = g2d.getColor(); g2d.setColor(this.lineColor); Shape outer; Shape inner; int offs = this.thickness * (int) scaleFactor; int size = offs + offs; if (this.roundedCorners) { // following code That is add a blank line after `g2d` declaration. Then save the old color; the next block is declaration for `outer` and `inner`; followed by the block with `offs` and `size` right before they're used. src/java.desktop/share/classes/javax/swing/border/LineBorder.java line 173: > 171: path.append(inner, false); > 172: g2d.fill(path); > 173: g.setColor(oldColor); Suggestion: g2d.setColor(oldColor); Use `g2d` as it was originally and add a blank line to separate fill — painting the border — from the cleanup, restoring the color. src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 294: > 292: Graphics2D g2d = (Graphics2D) g; > 293: g2d.setStroke(new BasicStroke((float) stkWidth)); > 294: } Suggestion: if (g instanceof Graphics2D) { ((Graphics2D) g).setStroke(new BasicStroke((float) stkWidth)); } ? ------------- Marked as reviewed by aivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/11571
