On Fri, 21 Jul 2023 10:56:28 GMT, Tejesh R <[email protected]> wrote:
>> The bug mentions about transparency issue on Linux which actually got
>> resolved with [JDK-8006421](https://bugs.openjdk.org/browse/JDK-8006421)
>> fix. Now there is another problem related to the bug, which is screen
>> selection going wrong during checking for new screen when the test is run.
>> The problem is that (As seen in the pictures attached in bug) the
>> transparency is lost for windows which are in screen 0 (default screen) too,
>> which is not supposed to happen where windows on screen 1 should have lost
>> there transparency. The main reason being the call to `toGlobal()` when
>> window bounds are passed to `checkIfOnNewScreen` method. I didn't get the
>> actual reason for `toGlobal` being used here (actually not required to check
>> monitor number), but it actually doubled the X position of the window
>> [here](
>> https://github.com/openjdk/jdk/blob/4b1403d06b99b91ddd89ad6e54669b0595f1f8e5/src/java.desktop/unix/classes/sun/awt/X11/XBaseWindow.java#L776).
>> Removing `toGlobal` solve the issue and neith
er didn't cause any regression (existing test + the reason `toGlobal()` was
added [JDK-8143295](https://bugs.openjdk.org/browse/JDK-8143295)).
>>
>> The automated test fails if GC is changed for windows within screen 0
>> (default screen). CI testing is green.
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review fix
Marked as reviewed by azvegint (Reviewer).
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 102:
> 100: robot.delay(50);
> 101: robot.waitForIdle();
> 102: if (windowList.get(windowList.size() -
> 1).getBounds().intersects
Maybe it's better to make `createWindow(r)` return a window and cache it
instead of several querying of windowList?
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 103:
> 101: robot.waitForIdle();
> 102: if (windowList.get(windowList.size() -
> 1).getBounds().intersects
> 103: (screen.getDefaultConfiguration().getBounds())) {
Looks like you can reuse `screenBounds` variable for lines 103 and 106.
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 130:
> 128: public void mouseClicked(MouseEvent e) {
> 129: ((Window) e.getSource()).dispose();
> 130: }
New line missing.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14825#pullrequestreview-1551942043
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1277439644
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1277435451
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1277439874
