Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 20:03:20 GMT, Phil Race wrote: >> All I wanted is to bring up the inconsistency so that a few people would >> take a look at it while reviewing this change. > > It does look odd. Focus would need transferring in both cases I'd expect. > It goes back to the very beginning of open source JDK so I can't see a > changeset to point to in order to explain it. > And I also can't find any bug reports that might be related - either one > about adding it, or one about things not working because it is not always > executed. I looked at the history before what's available in Git. I looks this has always been this way. Yet it doesn't look right. `AwtDialog::CheckInstallModalHook()` is called right before the page dialog is displayed by using `::PageSetupDlg`. I expect `AwtDialog::CheckUninstallModalHook()` needs to be called after it. Likely, the early returns (inside `if (ret)`) are very rare, if any of these has ever occurred at all. I'll submit a bug to include calling `AwtDialog::CheckUninstallModalHook()` in error cleanup. The `done` label which were before [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) should've been before the line which calls `CheckUninstallModalHook`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1649289792
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Fri, 21 Jun 2024 03:17:38 GMT, Prasanta Sadhukhan wrote: >> The way it was "before" is that we always returned the value of "doIt". Why >> not restore that for consistency ? > > I believe that's what this PR is doing, it returns value of "doIt" at end, > isn't it? > The way it was "before" is that we always returned the value of "doIt". Why > not restore that for consistency ? I think it's clearer with explicit `JNI_FALSE` even though it's inconsistent. You don't have to track in your mind what value `doIt` has. > I believe that's what this PR is doing, it returns value of "doIt" at end, > isn't it? Yes, it does *now*. You changed it after Phil had left his comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1649205235
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 20:04:47 GMT, Phil Race wrote: >> 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. > > The way it was "before" is that we always returned the value of "doIt". Why > not restore that for consistency ? I believe that's what this PR is doing, it returns value of "doIt" at end, isn't it? - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1648364406
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 10:39:48 GMT, Alexey Ivanov wrote: >> 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. The way it was "before" is that we always returned the value of "doIt". Why not restore that for consistency ? >> 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. It does look odd. Focus would need transferring in both cases I'd expect. It goes back to the very beginning of open source JDK so I can't see a changeset to point to in order to explain it. And I also can't find any bug reports that might be related - either one about adding it, or one about things not working because it is not always executed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1648070159 PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1648068766
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Thu, 20 Jun 2024 03:39:47 GMT, Prasanta Sadhukhan
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
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 08:38:29 GMT, Tejesh R wrote: > Have you ever noticed focus going out of dialog/frame when application starts > in CI system? In this test, since you are directly using TAB to navigate till > Cancel button, any changes that focus might not be on the Dialog on startup? > I roughly remember a test where the focus was not on the Frame and we then > setFocus or something was used.. It works on CI...Job link in JBS.. - PR Comment: https://git.openjdk.org/jdk/pull/19786#issuecomment-2179824967
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 10:39:35 GMT, Alexey Ivanov 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
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 10:32:28 GMT, Alexey Ivanov wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Test fix > > test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 57: > >> 55: robot.waitForIdle(); >> 56: robot.delay(500); >> 57: }); > > These key presses are sent to whatever window has focus… before the Page > Dialog is shown and after it's hidden. Is pressing `VK_ESCAPE` not enough? > Pressing Esc is the same as clicking Cancel button. Yes, VK_ESCAPE worked..Modified to use that...Thanks.. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1646881869
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 08:30:42 GMT, Prasanta Sadhukhan
wrote:
>> On cancelling PageDialog, same PageFormat object should be returned which
>> stopped working after
>> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160).
>> Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct
>> value is returned from PageDialog.show action..
>> An automated printing testcase is created since the issue was caught by
>> manual test and so having another manual test run the risk of not being
>> executed during CI testing..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Test fix
Changes requested by aivanov (Reviewer).
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.
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.
test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 57:
> 55: robot.waitForIdle();
> 56: robot.delay(500);
> 57: });
These key presses are sent to whatever window has focus… before the Page Dialog
is shown and after it's hidden. Is pressing `VK_ESCAPE` not enough? Pressing
Esc is the same as clicking Cancel button.
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.
-
PR Review: https://git.openjdk.org/jdk/pull/19786#pullrequestreview-2127813477
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645895912
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645900131
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645886899
PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645894197
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 08:30:42 GMT, Prasanta Sadhukhan wrote: >> On cancelling PageDialog, same PageFormat object should be returned which >> stopped working after >> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160). >> Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct >> value is returned from PageDialog.show action.. >> An automated printing testcase is created since the issue was caught by >> manual test and so having another manual test run the risk of not being >> executed during CI testing.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Test fix Have you ever noticed focus going out of dialog/frame when application starts in CI system? In this test, since you are directly using TAB to navigate till Cancel button, any changes that focus might not be on the Dialog on startup? I roughly remember a test where the focus was not on the Frame and we then setFocus or something was used.. - PR Review: https://git.openjdk.org/jdk/pull/19786#pullrequestreview-2127529049
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
> On cancelling PageDialog, same PageFormat object should be returned which > stopped working after > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160). > Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct > value is returned from PageDialog.show action.. > An automated printing testcase is created since the issue was caught by > manual test and so having another manual test run the risk of not being > executed during CI testing.. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Test fix - Changes: - all: https://git.openjdk.org/jdk/pull/19786/files - new: https://git.openjdk.org/jdk/pull/19786/files/921651b3..7daa416d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19786=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19786=00-01 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19786/head:pull/19786 PR: https://git.openjdk.org/jdk/pull/19786
Re: RFR: 8334509: Cancelling PageDialog does not return the same PageFormat object [v2]
On Wed, 19 Jun 2024 06:49:03 GMT, Abhishek Kumar wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Test fix > > test/jdk/java/awt/print/PrinterJob/PageDialogCancelTest.java line 51: > >> 49: robot.keyRelease(KeyEvent.VK_TAB); >> 50: robot.waitForIdle(); >> 51: robot.delay(500); > > I think shorter delay of 100ms should be ok here and below as well. It might be ok for statndalone test but jtreg execution is failing sometimes with shorter delay so kept that way.. - PR Review Comment: https://git.openjdk.org/jdk/pull/19786#discussion_r1645677261
