On Thu, 1 Feb 2024 07:09:00 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/Collate2DPrintingTest.java line 40:
> 38: import java.awt.event.ActionListener;
> 39: import java.awt.event.WindowAdapter;
> 40: import java.awt.event.WindowEvent;
`WindowAdapter` and `WindowEvent` aren't used.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 113:
> 111: } catch (Exception e) {
> 112: e.printStackTrace();
> 113: }
Display an error message?
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 139:
> 137: private static final String INSTRUCTIONS =
> 138: "You must have a printer available to perform this test\n" +
> 139: "The print result should be collated.";
Expand the instructions to explain what to do?
There are two buttons, which do I click?
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 54:
> 52: protected BufferedImage _image;
> 53:
> 54: protected PageFormat _pageFormat;
I suggest getting rid of the leading underscores. The fields could be `private`
(and `final` too), because test classes aren't extended, some classes are even
marked as `final` too.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 63:
> 61:
> 62: protected int printImage(Graphics g, PageFormat pf, BufferedImage
> image) {
> 63:
A blank at the start of a method is redundant.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 68:
> 66:
> 67: int paperW = (int) pf.getImageableWidth(), paperH =
> 68: (int) pf.getImageableHeight();
Suggestion:
int paperW = (int) pf.getImageableWidth();
int paperH = (int) pf.getImageableHeight();
Declare the variable separately.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 70:
> 68: (int) pf.getImageableHeight();
> 69:
> 70: int x = (int) pf.getImageableX(), y = (int) pf.getImageableY();
Suggestion:
int x = (int) pf.getImageableX();
int y = (int) pf.getImageableY();
Declare the variables separately.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 80:
> 78: g2D.drawImage(image, scaleOp, x + _objectBorder, y +
> _objectBorder);
> 79:
> 80: g2D.dispose();
Did you create the `Graphics` object? If not, do not dispose of it.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 94:
> 92: }
> 93: return result;
> 94: });
Suggestion:
pj.setPrintable(this::printImage);
Move the logic to return `NO_SUCH_PAGE` into the `printImage`, modify the
declaration of `printImage`:
private int printImage(Graphics g, PageFormat pf, int pageIndex) {
It doesn't need the `image` parameter — it can access the member `_image`.
Fail the test immediately if the image cannot be created.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 101:
> 99: } catch (Exception e) {
> 100: e.printStackTrace();
> 101: }
Display an error message? Let the fail?
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 107:
> 105: "You must have a printer available to perform this test.\n" +
> 106: "\n" +
> 107: "The test passes if you get a printout of a gray
> rectangle\n" +
Expand the instructions to explain that a print job is started automatically.
This kind of tests could benefit from having a secondary UI for the tester to
initiate printing explicitly. Unfortunately, the changes for split UI in
`PassFassJFrame` ([JDK-8294148](https://bugs.openjdk.org/browse/JDK-8294148))
aren't ready yet.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 108:
> 106: "\n" +
> 107: "The test passes if you get a printout of a gray
> rectangle\n" +
> 108: "with white text without any exception.";
Which means that you should make the test fail if any exception occurs rather
swallowing the exception and printing its stack trace.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 114:
> 112: if (PrinterJob.lookupPrintServices().length == 0) {
> 113: throw new SkippedException("Printer not configured or
> available."
> 114: + " Test cannot continue.");
Suggestion:
throw new SkippedException("Printer not configured or available.");
test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 1:
> 1: /*
Replace C-style (`type a[]`) array declarations with Java-style (`type[] a`).
This applies to `main` declaration and `byte data[]` at line 111.
test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 53:
> 51: " Confirm that the methods are printed.\n" +
> 52: " For Graphics: drawString, drawString, drawChars,
> drawBytes\n" +
> 53: " For Graphics2D: drawString, drawString, drawGlyphVector";
Expand the instructions to explain that a print job is automatically created an
printed. The tester is expected to verify that the listed methods are present
on the page.
test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 134:
> 132: iy += 30;
> 133: s = "drawString(AttributedCharacterIterator iterator, "+
> 134: "float x, float y)";
Suggestion:
s = "drawString(AttributedCharacterIterator iterator, "
+ "float x, float y)";
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 32:
> 30: import java.awt.Panel;
> 31: import java.awt.event.ActionEvent;
> 32: import java.awt.event.ActionListener;
`ActionEvent` and `ActionListener` aren't used.
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 72:
> 70: } catch (PrinterException pe) {
> 71: pe.printStackTrace();
> 72: }
Fail the test or show an error message?
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 93:
> 91: g2d.drawString("Y THIS WAY", 60, 200);
> 92: g2d.drawRect(0, 0, (int) pageFormat.getImageableWidth(),
> 93: (int) pageFormat.getImageableHeight());
Suggestion:
g2d.drawRect(0, 0,
(int) pageFormat.getImageableWidth(),
(int) pageFormat.getImageableHeight());
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 100:
> 98: }
> 99: g2d.drawRect(1, 1, (int) pageFormat.getImageableWidth() - 2,
> 100: (int) pageFormat.getImageableHeight() - 2);
Suggestion:
g2d.drawRect(1, 1,
(int) pageFormat.getImageableWidth() - 2,
(int) pageFormat.getImageableHeight() - 2);
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 115:
> 113: " Press the print button, which brings up a print dialog
> and\n" +
> 114: " in the dialog select a printer and press the print
> button\n" +
> 115: " in the dialog. Repeat for as many printers as you have
> installed\n" +
Suggestion:
" Press the print button, which brings up a print dialog.\n" +
" In the dialog select a printer and press the print button.\n" +
" Repeat for all the printers as you have installed\n" +
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 116:
> 114: " in the dialog select a printer and press the print
> button\n" +
> 115: " in the dialog. Repeat for as many printers as you have
> installed\n" +
> 116: " On solaris and linux just one printer is sufficient\n" +
Suggestion:
" On Solaris and Linux just one printer is sufficient.\n" +
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 124:
> 122: " are positioned identically on both pages\n" +
> 123: " The test fails if the JRE crashes, or if the output from
> the two\n" +
> 124: " pages of a job is aligned differently";
If JRE crashes, the test obviously fails. Should we remove this? The test has
no control over it, so list the actions that the tester needs to do.
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 131:
> 129: throw new SkippedException("Printer not configured or
> available."
> 130: + " Test cannot continue.");
> 131: }
Suggestion:
public static void main(String[] args) throws Exception {
if (PrinterJob.lookupPrintServices().length == 0) {
throw new SkippedException("Printer not configured or available.");
}
Remove the blank line; shorten the message.
test/jdk/java/awt/print/PrinterJob/JobName/PrinterJobName.java line 41:
> 39: public class PrinterJobName implements Printable {
> 40:
> 41: static String theName = "Testing the Jobname setting";
Suggestion:
private static final String theName = "Testing the Jobname setting";
Maybe even follow the constant naming convention, `THE_NAME`.
test/jdk/java/awt/print/PrinterJob/JobName/PrinterJobName.java line 45:
> 43: private static final String INSTRUCTIONS =
> 44: "You must have a printer available to perform this test\n" +
> 45: "This test prints a page with a banner/job name of\n" +
> theName;
Suggestion:
"This test prints a page with a banner/job name of\n" +
theName;
It makes it clear there's more text from a variable.
test/jdk/java/awt/print/PrinterJob/NumCopies.java line 47:
> 45: "You must have a printer available to perform this test\n" +
> 46: "This test should print a total of four pages which are
> two\n" +
> 47: " copies of each of two pages which consist of the text :-\n"
> +
This does not look good, I can hardly understand. Could you simplify the
sentence/description.
Two pages, two copies of each…
test/jdk/java/awt/print/PrinterJob/NumCopies.java line 56:
> 54: throw new SkippedException("Printer not configured or
> available."
> 55: + " Test cannot continue.");
> 56: }
Suggestion:
public static void main(String[] args) throws Exception {
if (PrinterJob.lookupPrintServices().length == 0) {
throw new SkippedException("Printer not configured or available.");
}
test/jdk/java/awt/print/PrinterJob/NumCopies.java line 79:
> 77: g.translate((int) pf.getImageableX(), (int) pf.getImageableY());
> 78: g.setColor(Color.black);
> 79: g.drawString("This is page number " +
> Integer.toString(pageIndex), 50, 50);
Suggestion:
g.drawString("This is page number " + pageIndex, 50, 50);
-------------
PR Review: https://git.openjdk.org/jdk/pull/17608#pullrequestreview-1868598536
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481970506
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481974930
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481976037
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481981619
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481978577
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481979095
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481979501
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481986259
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482001069
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481988074
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482004679
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481991260
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1481991884
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482025772
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482021629
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482019949
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482027387
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482028325
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482030221
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482030547
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482034040
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482034690
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482037017
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482039473
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482042531
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482041636
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482047669
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482048310
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482051739
