RFR: JDK-8294484: MetalBorder's FrameBorder & DialogBorder have border rendering issues when scaled [v3]

[Alexey Ivanov]

On Fri, 3 Feb 2023 17:26:22 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> FrameBorder and DialogBorder had border scaling issues similar to JInternalFrame. This fix addresses it by creating `AbstractMetalBorder` class within MetalBorder, that contains the common steps required for painting border for `FrameBorder`, `DialogBorder` and `InternalFrameBorder`. 
>> 
>> All 3 cases - JFrame, JDialog and JInternalFrame are combined into a single test case - `ScaledMetalBorderTest` and hence the older `InternalFrameBorderTest` which is no longer required, is deleted.
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   minor change

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 239:

> 237: 
> 238:     @SuppressWarnings("serial")
> 239:     private abstract static sealed class AbstractMetalBorder

Suggestion:

    private abstract static sealed class AbstractMetalWindowBorder

or
Suggestion:

    private abstract static sealed class MetalWindowBorder

So that it's the class name is more specific: it applies only to window-like objects: Frame, Dialog and Internal Frame.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 255:

> 253:             SwingUtilities3.paintBorder(c, g,
> 254:                     x, y, w, h,
> 255:                     this::paintUnscaledBorder);

It's a matter of preference, however, I prefer the previous style where parameters are aligned after the opening parenthesis.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 272:

> 270:         protected abstract boolean isActive(Component c);
> 271: 
> 272:         protected abstract boolean isResizable(Component c);

What do you think if we declare these two abstract methods above `updateColors`? Or just `isActive` method?

It may help to understand the code inside `updateColors`.

It's not a strong suggestion.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 291:

> 289:                 //are positioned on the border
> 290:                 int midPoint = thickness / 2;
> 291:                 int stkWidth = clipRound(scaleFactor);

It's not part of this changeset, yet is possible to spell it out? When words spelled fully, like `strokeWidth`, they're easier to understand.

Maybe do it under a separate issue to make this one cleaner.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 359:

> 357:         }
> 358: 
> 359:         public void paintBorder(Component c, Graphics g, int x, int y,

Let's add `@Override`?

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 364:

> 362:         }
> 363: 
> 364:         public Insets getBorderInsets(Component c, Insets newInsets) {

Let's add @Override?

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 375:

> 373:      */
> 374:     @SuppressWarnings("serial") // Superclass is not serializable across versions
> 375:     static final class FrameBorder extends AbstractMetalBorder implements UIResource {

If the class isn't used anywhere but `MetalBorders` class, does it make sense to declare the class `private`? I think it does, it conveys that this class is used only here.

This applies to other classes too.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 396:

> 394:      */
> 395:     @SuppressWarnings("serial") // Superclass is not serializable across versions
> 396:     static sealed class DialogBorder

Probably `DialogBorder` should extend `FrameBorder` to inherit and re-use `isActive` and `isResizable` methods: they're the same in both classes.

src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java line 401:

> 399:             permits ErrorDialogBorder, QuestionDialogBorder, WarningDialogBorder {
> 400: 
> 401:         protected Color getActiveBackground() {

Either leave the opening brace on its own line it used to be, or update the style for the following methods as well.

test/jdk/javax/swing/plaf/metal/MetalBorders/ScaledMetalBorderTest.java line 167:

> 165: 
> 166:     private static void runTests(String windowType) throws InterruptedException,
> 167:                                                          InvocationTargetException {

Suggestion:

    private static void runTests(String windowType) throws InterruptedException,
                                                           InvocationTargetException {

Should they rather be aligned?

test/jdk/javax/swing/plaf/metal/MetalBorders/ScaledMetalBorderTest.java line 318:

> 316:         jDialog.setLayout(new GridBagLayout());
> 317:         jDialog.getContentPane().add(scale);
> 318:         jDialog.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

Suggestion:

        jDialog.setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE);

Would be more logical.

-------------

PR: https://git.openjdk.org/jdk/pull/12391