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

[Phil Race]

On Thu, 7 Nov 2024 11:01:55 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8277240
>
> 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.

I have no idea how that happened. Fixed.

> 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.

I know. I just preferred it. But I can change it.

> 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.

I don't know why that is there but I agree it is better to remove it.

> 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?

OK .. but I don't think it is a big deal

> 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());

ok

> 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.

ok

> 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.

ok

> 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.”_

hmm. I don't agree with that convention, it disrupts the indentation. So I prefer to leave it as it always was.

> 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.

ok

> 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.

ok

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833419610
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833421241
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833422070
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833428559
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833423460
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833423856
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833424169
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833425628
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833426479
PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1833426779