On Thu, 7 Nov 2024 11:01:55 GMT, Alexey Ivanov <[email protected]> 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
