RFR: 6899304 : java.awt.Toolkit.getScreenInsets(GraphicsConfiguration) returns incorrect values [v2]

[Alexey Ivanov]

On Thu, 23 Jan 2025 22:29:22 GMT, anass baya <duke at openjdk.org> wrote:

>> Screen number 0 is not always the primary screen, so we’ve removed the code that assumes it is.
>> 
>> We used an existing test and took the following considerations into account for Windows:
>> 
>> - On Windows, undecorated maximized frames are placed over the taskbar.
>> - On Windows, the top-left corner of an undecorated maximized frame may have negative coordinates (x, y).
>> - Consider the fractional part after scaling.
>
> anass baya has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update copyright

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 30:

> 28:   @summary Test to check if the frame is created on the specified GraphicsDevice
> 29:   and if getScreenInsets()returns the correct values across multiple monitors.
> 30:   @author Oleg Pekhovskiy

Suggestion:


We're removing `@author` tags from tests.

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 50:

> 48: 
> 49:     public static void main(String[] args) throws InterruptedException {
> 50: 

There's no need in a blank right at the start of a method.

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 52:

> 50: 
> 51:         GraphicsEnvironment ge = GraphicsEnvironment.getLocalGraphicsEnvironment();
> 52:         GraphicsDevice[] gds = ge.getScreenDevices();

We throw `jtreg.SkippedException` if test cannot be run in the current environment.

You can find a sample usage in #22729.

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 73:

> 71:              * Use a decorated frame instead.
> 72:              */
> 73:             if(Platform.isWindows()) {

Suggestion:

            if (Platform.isWindows()) {

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 84:

> 82:             frame.setExtendedState(java.awt.Frame.MAXIMIZED_BOTH);
> 83:             Thread.sleep(2000);
> 84: 

I suggest using `Robot.waitForIdle` to ensure all events are handled, along with `Robot.delay`. On the other hand, if the test is stable as it is, this piece of code may remain unchanged.

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 93:

> 91:              */
> 92:             if (frameBounds.x < bounds.x)
> 93:             {

Suggestion:

            if (frameBounds.x < bounds.x) {

Use Java code style.

test/jdk/java/awt/Multiscreen/MultiScreenInsetsTest/MultiScreenInsetsTest.java line 117:

> 115:         System.out.println("Test PASSED!");
> 116:     }
> 117:     static int getMarginForScaleX(GraphicsConfiguration gc) {

I'd add the `private` modifier to both methods as well as a blank line between the methods, especially after the `main` method.

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

PR Review: https://git.openjdk.org/jdk/pull/23183#pullrequestreview-2573405498
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929128385
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929128930
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929145080
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929129276
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929147230
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929130357
PR Review Comment: https://git.openjdk.org/jdk/pull/23183#discussion_r1929148652