RFR: 8277240: java/awt/Graphics2D/ScaledTransform/ScaledTransform.java dialog does not get disposed

[Alexey Ivanov]

On Wed, 6 Nov 2024 23:59:24 GMT, Phil Race <prr at openjdk.org> wrote:

> This test has had reports of failures over the years notably on Linux.
> I think the large number of XVisuals may have something to do with it.
> I've updated it quite a bit and limited the number of dialogs it creates - we really don't need to test 300 of them
> This new version has passed hundreds of iterations.

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 38:

> 36: /*
> 37:  * @test
> 38:  * @bug 8069362

Is it accidental or intentional?

[JDK-8069361](https://bugs.openjdk.org/browse/JDK-8069361): _SunGraphics2D.getDefaultTransform() does not include scale factor_ is the correct bug. The new bug id, JDK-8069362, seems unrelated.

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 49:

> 47:     private static volatile Dialog dialog;
> 48:     private static volatile long startTime = 0;
> 49:     private static volatile long endTime = 0;

Initialisers aren't needed: they set fields to their default values, and the values are reset before each test.

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 57:

> 55:         if (ge.isHeadlessInstance()) {
> 56:             return;
> 57:         }

Shall we remove `ge.isHeadlessInstance()`? The test is marked with `@key headful`, we can let it fail in headless environment.

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 92:

> 90: 
> 91:     private static void showDialog(final GraphicsConfiguration gc) {
> 92: 

Can you remove this blank line at the start of the method, please?

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 93:

> 91:     private static void showDialog(final GraphicsConfiguration gc) {
> 92: 
> 93:         System.out.println("Creating dialog for gc="+gc+" with tx="+gc.getDefaultTransform());

Suggestion:

        System.out.println("Creating dialog for gc=" + gc + " with tx=" + gc.getDefaultTransform());

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 95:

> 93:         System.out.println("Creating dialog for gc="+gc+" with tx="+gc.getDefaultTransform());
> 94: 
> 95:         dialog = new Dialog((Frame) null, "Test", true, gc);

Suggestion:

        dialog = new Dialog((Frame) null, "ScaledTransform", true, gc);

Set a unique title.

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 97:

> 95:         dialog = new Dialog((Frame) null, "Test", true, gc);
> 96: 
> 97:         dialog.setSize(100, 100);

Suggestion:

        dialog.setSize(300, 100);

Larger width ensures the longer title is fully seen and not truncated.

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 108:

> 106:                     System.out.println("GTX = " + gTx);
> 107:                     passed = (gcTx.getScaleX() == gTx.getScaleX()) &&
> 108:                              (gcTx.getScaleY() == gTx.getScaleY());

Suggestion:

                    passed = (gcTx.getScaleX() == gTx.getScaleX())
                             && (gcTx.getScaleY() == gTx.getScaleY());

[Java Code Conventions](https://www.oracle.com/technetwork/java/codeconventions-150003.pdf)¹ recommend wrapping before the binary operator; if a line starts with an operator, it is clearly a continuation line rather than a new statement.

¹ Section 4.2 Wrapping Lines, bullet 2: _“Break before an operator.”_

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 113:

> 111:                 }
> 112:                 painted.countDown();
> 113:                 endTime = System.currentTimeMillis();

Suggestion:

                endTime = System.currentTimeMillis();
                painted.countDown();

Both are volatile, yet it may still be possible that the main thread read a stale value of `endTime` before it's updated on EDT.

test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 126:

> 124:             dialog.setVisible(false);
> 125:             dialog.dispose();
> 126:        }

Suggestion:

       if (dialog != null) {
           System.out.println("Disposing dialog");
           dialog.setVisible(false);
           dialog.dispose();
       }

Amend indentation to 4 spaces as everywhere.

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

PR Review: https://git.openjdk.org/jdk/pull/21942#pullrequestreview-2420639676
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832483434
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832630857
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832602281
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832605490
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832495798
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832488803
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832490104
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832616170
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832610325
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832613055