On Mon, 2 Jun 2025 09:33:34 GMT, Khalid Boulanouare <d...@openjdk.org> wrote:
>> Fixes issue in which the test fails when run on multi-screen machine.
>>
>> Tested on Ubuntu 24.04, MacOS 15 and Windows 11
>>
>> JTREG
>>
>> runner starting test: java/awt/Frame/MultiScreenTest.java
>> runner finished test: java/awt/Frame/MultiScreenTest.java
>> Passed. Execution successful
>
> Khalid Boulanouare has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Removes unnecessary lines and keep consistent code format
There are other code clean-up that can be done, but these issues are close to
lines which are modified.
test/jdk/java/awt/Frame/MultiScreenTest.java line 1:
> 1: /*
throw new SkippedException("You have only one monitor in your system - test
passed");
Please remove `" - test passed"`, the test is skipped.
test/jdk/java/awt/Frame/MultiScreenTest.java line 1:
> 1: /*
Could you make the other classes nested inside `MultiScreenTest`?
test/jdk/java/awt/Frame/MultiScreenTest.java line 85:
> 83: "Actively drag the DitherTest frames on the secondary
> screen and " +
> 84: "if you see garbage appearing on your primary screen " +
> 85: "test failed otherwise it passed.";;
Suggestion:
"test failed otherwise it passed.";
test/jdk/java/awt/Frame/MultiScreenTest.java line 103:
> 101: JFrame f = new JFrame(gc[i]); // test JFrame( gc )
> 102: GCCanvas c = new GCCanvas(gc[i]); // test canvas( gc
> )
> 103: Rectangle gcBounds = gc[i].getBounds(); // test
> getBounds()
I suggest removing the comments altogether — they don't add anything to what's
in the code.
Suggestion:
JFrame f = new JFrame(gc[i]);
GCCanvas c = new GCCanvas(gc[i]);
Rectangle gcBounds = gc[i].getBounds();
test/jdk/java/awt/Frame/MultiScreenTest.java line 108:
> 106:
> 107: f.getContentPane().add(c);
> 108: f.setTitle("Screen# " + Integer.toString(j) + ",
> GC#" + Integer.toString(i));
Can be simplified:
Suggestion:
f.setTitle("Screen# " + j + ", GC#" + i);
test/jdk/java/awt/Frame/MultiScreenTest.java line 111:
> 109: f.setSize(300, 200);
> 110: f.setLocation(400 + xoffs, (i * 150) + yoffs); //
> test
> 111: // displaying in right location
Suggestion:
// test displaying in right location
f.setLocation(400 + xoffs, (i * 150) + yoffs);
The comment applies to this line, however, does it add anything, does it
clarify anything? Should we remove the comment?
test/jdk/java/awt/Frame/MultiScreenTest.java line 114:
> 112: list.add(f);
> 113:
> 114: Frame ditherfs = new Frame("DitherTest GC#" +
> Integer.toString(i), gc[i]);
Can be simplified:
Suggestion:
Frame ditherfs = new Frame("DitherTest GC#" + i, gc[i]);
test/jdk/java/awt/Frame/MultiScreenTest.java line 115:
> 113:
> 114: Frame ditherfs = new Frame("DitherTest GC#" +
> Integer.toString(i), gc[i]);
> 115: ditherfs.setLayout(new BorderLayout()); //
> showDitherTest
The comment is misleading: setting a layout doesn't show anything.
Suggestion:
ditherfs.setLayout(new BorderLayout());
test/jdk/java/awt/Frame/MultiScreenTest.java line 140:
> 138: super(gc);
> 139: this.gc = gc;
> 140: bounds = gc.getBounds();
Remove
Graphics g = this.getGraphics();
This field is unused.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24752#pullrequestreview-2897710693
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127210738
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127220721
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127225063
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127197384
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127201901
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127200491
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127203043
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127204010
PR Review Comment: https://git.openjdk.org/jdk/pull/24752#discussion_r2127224690
