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

[Alexey Ivanov]

On Wed, 21 Dec 2022 22:09:37 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 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