On Thu, 23 Jan 2025 22:29:22 GMT, anass baya <d...@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
