On Wed, 19 Jun 2024 10:39:35 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Test fix > > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 694: > >> 692: ::GlobalUnlock(setup.hDevMode); >> 693: } >> 694: doIt = JNI_TRUE; > > Another option would be to return `JNI_TRUE` here and to return `JNI_FALSE` > in the end of the function. Yes, it could have been done that way and my intiial fix in JBS is that only but I wanted to reinstate the flag as it is before [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) which was working till now.. > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 697: > >> 695: } >> 696: >> 697: AwtDialog::CheckUninstallModalHook(); > > I wonder why the block of code at lines 697–709 is not needed if `JNI_FALSE` > is returned as a result of an error condition. > > No, it is not directly connected to the bug you're fixing, yet it doesn't > look right to me. We can have a followup bug on this I guess since it was existing before [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) also.. > test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 60: > >> 58: t1.start(); >> 59: PageFormat newFormat = pj.pageDialog(oldFormat); >> 60: if (!newFormat.equals(oldFormat)) { > > You should interrupt the `t1` thread after `pj.pageDialog` returns to stop > robot from sending `keyPress` and `keyRelease` after the dialog is hidden. > You can even use `Thread.sleep` instead of `Robot.delay` so that interrupting > the thread throws `InterruptedException` and its handler just exits. > (`Robot.delay` catches `InterruptedException` and then restores the > interrupted flag.) > > I wonder if any AWT event is sent when the Page Dialog is shown on the > screen, an event is more reliable and key presses won't be sent to an > arbitrary window in the system. I am not sure I understand this..I guess this thread execution will be a one-time activity so I guess we dont need to do any thread cleanup... ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1646883814 PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1646884122 PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1646883283
