On Wed, 7 Feb 2024 17:16:55 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Renjith Kannath Pariyangad has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> Disposed g2D object and similar test parm into one line
>
> test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 57:
>
>> 55: "was properly cancelled.\n" +
>> 56: "You will need to kill the application manually since
>> regression\n" +
>> 57: "tests apparently aren't supposed to call System.exit()";
>
> The tester cannot see the console. The test must handle this situation and
> provide the feedback to the tester as required or fail the test automatically.
>
> The tester should not terminate the application manually, the test must exit
> gracefully in either case: success or failure.
>
> It could be reasonable to submit a new bug for this test only and use the
> changes in this PR as the starting point. I didn't analyse thoroughly the
> tests when I submitted this bug. You already deemed, there were too many
> tests for a single changeset.
Update the instructions to explain the test will continue automatically.
> test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 180:
>
>> 178: });
>> 179:
>> 180: Button pageButton = new Button("Page Setup..");
>
> Suggestion:
>
> Button pageButton = new Button("Page Setup...");
>
> Three dots are usually used to indicate a button opens a dialog to request
> more details.
It's still relevant.
> test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 195:
>
>> 193: });
>> 194:
>> 195: Button chooseButton = new Button("Printer..");
>
> Suggestion:
>
> Button chooseButton = new Button("Printer...");
It's still relevant.
> test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 229:
>
>> 227: add(panel);
>> 228: TextArea ta = new TextArea(10, 45);
>> 229: String ls = System.getProperty("line.Separator", "\n");
>
> Does `\n` not work in `TextArea` on all the platforms? In particular on
> Windows? If it does, just embed it into the text below.
It does work correctly with `\n`; please embed `\n` into the string and remove
`ls` variable.
> test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 259:
>
>> 257: o == PageFormat.LANDSCAPE ? "LANDSCAPE" :
>> 258: o == PageFormat.REVERSE_LANDSCAPE ?
>> "REVERSE_LANDSCAPE" :
>> 259: "<invalid>"));
>
> The code for converting orientation to string is repeated here. Create a
> helper method which does it — eliminate duplicate code.
Create a helper method which returns the string representation of the
orientation and use the helper method in both places where you convert
orientation to string, specifically lines 83–88 and 251–256.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487599250
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487943617
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487943861
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487956260
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487928320
