On Wed, 12 Jul 2023 04:59:30 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
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 45:
> 43: * @bug 8280482
> 44: * @key headful
> 45: * @requires (os.family == "linux")
Did you check it on other systems? What prevents this test to be available on
all systems?
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 46:
> 44: * @key headful
> 45: * @requires (os.family == "linux")
> 46: * @library /java/awt/regtesthelpers
Is the regtesthepers actually being used?
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 62:
> 60: createGUI();
> 61: } finally {
> 62: for (Window win : windowList) {
Looks like you don't wait for possible `graphicsConfiguration` change events to
come here, and start closing all windows.
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 98:
> 96: });
> 97:
> 98: Thread.sleep(100);
I think that we can show all the windows at once without this delay to speed up
the test execution.
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 116:
> 114: windowList.add(window);
> 115:
> 116: window.addPropertyChangeListener("graphicsConfiguration",
Probably it is worth adding the listener before calling `setVisible`.
test/jdk/java/awt/Multiscreen/MultiScreenCheckScreenIDTest.java line 128:
> 126: //Check if GC is changed for windows positioned in
> screen 0.
> 127:
> 128: if (windowXPos < screen1Width) {
This type of check fails for following display arrangement
I suggest to check if window's GraphicsConfiguration is associated with correct
screen device
(e.g. check if the window bounds within the
`window.getGraphicsConfiguration().getDevice().getBounds()`).
It also seems more reliable to me if we don't check the GraphicsConfiguration
only within the `graphicsConfiguration` change event, but unconditionally after
some delay after the window is displayed (in case the event doesn't occur).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1269501381
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1269413142
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1269505202
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1269519493
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1269475424
PR Review Comment: https://git.openjdk.org/jdk/pull/14825#discussion_r1269410573
