On Fri, 3 Feb 2023 17:26:22 GMT, Harshitha Onkar <[email protected]> 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
