RFR: JDK-8294680: Refactor scaled border rendering [v6]

[Alexey Ivanov]

On Wed, 11 Jan 2023 23:20:02 GMT, Harshitha Onkar <honkar at 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