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

[Alexey Ivanov]

On Thu, 5 Jan 2023 19:03:17 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 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