On Thu, 1 Feb 2024 11:44:27 GMT, Renjith Kannath Pariyangad
<rkannathp...@openjdk.org> wrote:
>> Hi Reviewers,
>>
>> Updated manual printer test cases with 'PassFailJFrame', also removed unused
>> variables. Added 'SkippedException' in case of printer missing or not
>> configured.
>>
>> Please review and let me know your suggestions.
>>
>> Regards,
>> Renjith
>
> 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
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 32:
> 30: import java.awt.print.PrinterJob;
> 31:
> 32: import jtreg.SkippedException;
At this time, `SkippedException` isn't used at this time.
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 46:
> 44:
> 45: private static final String INSTRUCTIONS =
> 46: "For non-windows OS, this test PASSes.\n" +
If the test is for Windows only, `@requires` tag should be used.
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 48:
> 46: "For non-windows OS, this test PASSes.\n" +
> 47: "You must have at least 2 printers available to perform this
> test.\n" +
> 48: "This test brings up a native Windows page dialog.\n" +
Can we verify this too? That is the number of print services is at least 2.
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 50:
> 48: "This test brings up a native Windows page dialog.\n" +
> 49: "Click on the Printer... button and change the selected
> printer. \n" +
> 50: "Test passes if the printout comes from the new selected
> printer.";
Does it imply the user has to choose a non-default printer?
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 52:
> 50: "Test passes if the printout comes from the new selected
> printer.";
> 51:
> 52: public static void main (String[] args) throws Exception {
Suggestion:
public static void main(String[] args) throws Exception {
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 54:
> 52: public static void main (String[] args) throws Exception {
> 53:
> 54: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
Suggestion:
PassFailJFrame passFailJFrame = PassFailJFrame.builder()
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 60:
> 58: .build();
> 59:
> 60: new PageDlgPrnButton() ;
Suggestion:
new PageDlgPrnButton();
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 64:
> 62: }
> 63:
> 64: public PageDlgPrnButton() throws PrinterException {
It seems to me, `pageDialogExample` can be made `static` and be called directly
without using the constructor at all.
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 109:
> 107: g2d.fillRect(rect.x, rect.y, rect.width, rect.height);
> 108:
> 109: g2d.dispose();
Do not dispose of the `Graphics` in `print` or `paint`.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 53:
> 51: "You must have a printer available to perform this test\n" +
> 52: "This test should print a page which contains the same\n" +
> 53: "text message as in the test window on the screen\n" +
I can't see any message in the test window on the screen.
You should override `paint` in `TextCanvas`.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 57:
> 55: "were thrown\n" +
> 56: "If an exception is thrown, or the page doesn't print
> properly\n" +
> 57: "then the test fails";
The tester cannot see the console, you should handle all the exceptions
yourself and fail the test if an exception is thrown.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 61:
> 59: public static void main(String[] args) throws Exception {
> 60:
> 61: if (PrinterJob.lookupPrintServices().length == 0) {
Suggestion:
public static void main(String[] args) throws Exception {
if (PrinterJob.lookupPrintServices().length == 0) {
Remove the redundant blank line at the start of a method.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 63:
> 61: if (PrinterJob.lookupPrintServices().length == 0) {
> 62: throw new SkippedException("Printer not configured or
> available."
> 63: + " Test cannot continue.");
Suggestion:
throw new SkippedException("Printer not configured or available.");
Let's make it shorter as in other PRs.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 91:
> 89: } catch (PrinterException pe) {
> 90: pe.printStackTrace();
> 91: }
Fail the test?
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 98:
> 96: }
> 97:
> 98: class TextCanvas extends Panel implements Printable {
The class can be `private` and `static`.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 103:
> 101:
> 102: if (pgIndex > 0)
> 103: return Printable.NO_SUCH_PAGE;
Suggestion:
if (pgIndex > 0) {
return Printable.NO_SUCH_PAGE;
}
Add braces.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 111:
> 109: g2d.drawString(str, 20, 40);
> 110:
> 111: g2d.dispose();
Do not dispose of the `Graphics` object that you didn't create.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 43:
> 41:
> 42: /*
> 43: * @test %I %W
Suggestion:
* @test
These should be removed.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 61:
> 59: private static final String INSTRUCTIONS =
> 60: "You must have a printer available to perform this test,\n" +
> 61: "prefererably Canon LaserShot A309GII.\n" +
Typo
Suggestion:
"preferably Canon LaserShot A309GII.\n" +
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 62:
> 60: "You must have a printer available to perform this test,\n" +
> 61: "prefererably Canon LaserShot A309GII.\n" +
> 62: "Printing must be done in Win 98 Japanese 2nd Edition.\n" +
I don't think it's possible at all. Nor is it relevant at the moment.
You should look at the original bug. If the test is still relevant, please
update the description. If not, let's remove it.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 98:
> 96: pack();
> 97:
> 98: setSize(500, 500);
You either `pack` to use the preferred sizes of all the components or `setSize`.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 139:
> 137: } catch (PrinterException e) {
> 138: e.printStackTrace();
> 139: }
Fail the test?
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 141:
> 139: }
> 140: } else
> 141: printerJob.cancel();
There's nothing to cancel, it hasn't started yet.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 156:
> 154: } catch (PrinterException e) {
> 155: e.printStackTrace();
> 156: }
Fail the test?
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 158:
> 156: }
> 157: } else
> 158: printerJob.cancel();
Nothing to cancel, it's no-op unless you already called `print`.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 162:
> 160: }
> 161:
> 162: class PrintImageCanvas extends Canvas implements Printable {
I propose making the `PrintImageCanvas` class a nested (`private static`) class
inside `PrintImage`.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 165:
> 163:
> 164: public PrintImageCanvas(PrintImage pds) {
> 165: }
The parameter is unused. The explicit constructor can be removed.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 178:
> 176: if (pi >= 1)
> 177: return NO_SUCH_PAGE;
> 178: else {
Use braces.
The `else` block is redundant: you return from the body of `if`. Remove the
`else` to decrease the level of indentation.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 183:
> 181: Font drawFont = new Font("MS Mincho", Font.ITALIC, 50);
> 182: g.setFont(drawFont);
> 183: g.drawString("PrintSample!", 100, 150);
Move creating the font and drawing into a helper method to avoid duplicating
code between `paint` and `print`.
The font can be created once and stored as member of `PrintImageCanvas`.
You may want to verify if "MS Mincho" font is available and throw
`SkippedException` if it's not.
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 59:
> 57: "There should be no FAILURE messages.\n" +
> 58: "You should also monitor the command line to see if any
> exceptions\n" +
> 59: "were thrown\n" +
Again, the tester does not see the console. If an exception is thrown, jtreg
handles it and fails the test.
Perhaps, you should track is any of the cases produces an exception, and if it
does, also fail the test instead of printing a message on the paper.
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 67:
> 65: if (PrinterJob.lookupPrintServices().length == 0) {
> 66: throw new SkippedException("Printer not configured or
> available."
> 67: + " Test cannot continue.");
Suggestion:
throw new SkippedException("Printer not configured or available.");
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 95:
> 93: } catch (PrinterException pe) {
> 94: pe.printStackTrace();
> 95: }
Fail the test?
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 102:
> 100: }
> 101:
> 102: static class TextCanvas extends Panel implements Printable {
Make it a `private static` class inside `PrintNullString` class.
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 108:
> 106: AttributedString emptyAttStr = new AttributedString(emptyStr);
> 107: AttributedCharacterIterator nullIterator = null;
> 108: AttributedCharacterIterator emptyIterator =
> emptyAttStr.getIterator();
Mark them all `private final`.
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 113:
> 111:
> 112: if (pgIndex > 0)
> 113: return Printable.NO_SUCH_PAGE;
Suggestion:
if (pgIndex > 0) {
return Printable.NO_SUCH_PAGE;
}
Use braces.
test/jdk/java/awt/print/PrinterJob/PrintParenString.java line 91:
> 89: } catch (PrinterException pe) {
> 90: pe.printStackTrace();
> 91: }
Fail the test?
test/jdk/java/awt/print/PrinterJob/PrintParenString.java line 98:
> 96: }
> 97:
> 98: static class TextCanvas extends Panel implements Printable {
Make it a `private static` class inside the `PrintParenString` class.
test/jdk/java/awt/print/PrinterJob/PrintParenString.java line 103:
> 101:
> 102: if (pgIndex > 0)
> 103: return Printable.NO_SUCH_PAGE;
Suggestion:
if (pgIndex > 0) {
return Printable.NO_SUCH_PAGE;
}
Always use braces.
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 1:
> 1: /*
The same comments apply here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17609#pullrequestreview-1870496834
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483156713
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483141924
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483144240
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483156070
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483146282
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483146866
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483147420
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483153461
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483158144
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483207976
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483167386
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483167987
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483168508
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483169343
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483184324
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483171224
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483170644
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483220229
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483226916
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483224096
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483228698
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483232498
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483232274
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483232909
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483233779
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483236856
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483237893
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483244667
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483242133
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483258052
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483258543
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483258991
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483259722
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483262162
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483262589
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483265526
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483266374
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483266794
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1483268669
