RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]

[Alexey Ivanov]

On Thu, 20 Jun 2024 03:39:47 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> 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..

The only problem with the flag is that the data flow is not as clear.

I'm not insisting, it worked before and it will work in the future.

>> 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..

All I wanted is to bring up the inconsistency so that a few people would take a look at it while reviewing this change.

>> 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 specially when the dialog is modal so it will only be cancelled (ie pageDialog will return) by VK_ESCAPE in the thread

You've resolved the problem.

In previous iteration, the keys were sent in a loop, which meant that the thread could continue to run even after the page dialog was closed.

Now the `t1` thread presses `VK_ESCAPE` once and exits.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647370601
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647372620
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1647377851