On Mon, 4 Mar 2024 06:33:09 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:
>
> Updated instruction
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 1:
> 1: /*
Could you please add `@Override` annotations to all the methods which implement
the interfaces.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 59:
> 57: */
> 58: public class Collate2DPrintingTest
> 59: extends Frame implements Doc, Printable {
Suggestion:
implements Doc, Printable {
`Frame` used to be the container to display the buttons, now it's not needed.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 74:
> 72: if (pj.printDialog(prSet)) {
> 73: pj.print(prSet);
> 74: }
In this case, the print dialog has *Collated* checked and disabled; the number
of copies is set to 1 and I can't change the value.
Does it work for you? Is it a bug in JDK?
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 102:
> 100: main.add(Box.createVerticalGlue());
> 101: main.add(print2D);
> 102: main.add(printMerlin);
Suggestion:
main.add(print2D);
main.add(Box.createVerticalStrut(4));
main.add(printMerlin);
Add some margin between the buttons.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 49:
> 47: public class DrawImage {
> 48: private static final int OBJECT_BORDER = 15;
> 49: private static final String INSTRUCTIONS =
Suggestion:
private static final int OBJECT_BORDER = 15;
private static final String INSTRUCTIONS =
I think it's better with a blank line, the two constants aren't closely related.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 57:
> 55: private final PageFormat pageFormat;
> 56:
> 57: public DrawImage(BufferedImage image) {
Let's make it private too.
Suggestion:
private DrawImage(BufferedImage image) {
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 73:
> 71: int y = (int) pageFormat.getImageableY();
> 72:
> 73: // make slightly smaller (25) than max possible width
Suggestion:
// Make the image slightly smaller (25) than max possible width
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 87:
> 85: }
> 86:
> 87: public void print() throws PrinterException {
Suggestion:
private void print() throws PrinterException {
Make it `private` to avoid any confusion with implementation of `Printable`
interface which must be `public`.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 93:
> 91: if (pj.printDialog()) {
> 92: pj.print();
> 93: }
Fail the test if the tester clicks Cancel in the print dialog?
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 104:
> 102: if (image == null) {
> 103: throw new RuntimeException("Image creation failed");
> 104: }
It seems impossible for the `image` to be `null`. Let's drop this condition. If
for whatever reason it is `null`, printing will fail with NPE.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 117:
> 115: }
> 116:
> 117: public static BufferedImage prepareFrontImage() {
Let's make it private too.
Suggestion:
private static BufferedImage prepareFrontImage() {
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 121:
> 119: BufferedImage result = new BufferedImage(400, 200,
> 120: BufferedImage.TYPE_BYTE_GRAY);
> 121: int w = result.getWidth(), h = result.getHeight();
Suggestion:
int w = result.getWidth();
int h = result.getHeight();
Avoid declaring several variables at the same line.
test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 50:
> 48: " This test will automatically initiate a print\n" +
> 49: "\n" +
> 50: " Confirm that the methods are printed.\n" +
Suggestion:
" This test will automatically initiate a print.\n" +
"\n" +
" Confirm that the following methods are printed:\n" +
test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 76:
> 74: }
> 75:
> 76: public static AttributedCharacterIterator getIterator(String s) {
Suggestion:
private static AttributedCharacterIterator getIterator(String s) {
Let's make it private.
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 49:
> 47: * @run main/manual InvalidPage
> 48: */
> 49: public class InvalidPage extends Frame implements Printable {
Suggestion:
public class InvalidPage implements Printable {
No `Frame` is needed.
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 116:
> 114: " Repeat for all the printers as you have installed\n" +
> 115: " On Solaris and Linux just one printer is sufficient.\n" +
> 116: " Collect the output and examine it, each print job has two
> pages\n" +
Suggestion:
"\n" +
" Repeat for all the printers you have installed.\n" +
" On Solaris and Linux just one printer is sufficient.\n" +
"\n" +
" Collect the output and examine it, each print job has two
pages\n" +
Add blank lines to separate paragraphs for easier parsing / reading.
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 123:
> 121: " are positioned identically on both pages\n" +
> 122: " The test fails if the output from the two\n" +
> 123: " pages of a job is aligned differently";
Suggestion:
"\n" +
" The test fails if the output from the two\n" +
" pages of a job is aligned differently";
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 132:
> 130: PassFailJFrame.builder()
> 131: .instructions(INSTRUCTIONS)
> 132: .splitUI(InvalidPage::createTestUI)
Suggestion:
.instructions(INSTRUCTIONS)
.testTimeOut(10)
.splitUI(InvalidPage::createTestUI)
Increase the timeout as the tester needs to perform this operation for all the
printers installed and verify the printouts.
test/jdk/java/awt/print/PrinterJob/JobName/PrinterJobName.java line 40:
> 38: */
> 39: public class PrinterJobName implements Printable {
> 40: private static final String THE_NAME = "Testing the Jobname setting";
Suggestion:
private static final String THE_NAME = "Testing the Job name setting";
Spell it with space.
test/jdk/java/awt/print/PrinterJob/JobName/PrinterJobName.java line 41:
> 39: public class PrinterJobName implements Printable {
> 40: private static final String THE_NAME = "Testing the Jobname setting";
> 41: private static final String INSTRUCTIONS =
Keep the blank line between these two constants.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17608#pullrequestreview-1916838124
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512798993
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512791435
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512798212
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512792702
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512803855
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512811330
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512812240
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512807572
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512808081
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512822219
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512808749
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512809270
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512828005
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512829017
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512839764
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512836220
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512836833
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512837873
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512881276
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512872881
