On Tue, 13 Feb 2024 08:46:19 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:
>
> Fixed compiler error
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 46:
> 44: * @key printer
> 45: * @summary Page setup dialog settings
> 46: * @library /test/lib /java/awt/regtesthelpers
Suggestion:
* @library /java/awt/regtesthelpers
The `/test/lib` isn't used any more. Please update other files as appropriate.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 81:
> 79: + myPageFormat.getImageableX());
> 80: myImageableRightLabel.setText("Format Right Margin = " +
> (myPageFormat.getWidth()
> 81: - (myPageFormat.getImageableX() +
> myPageFormat.getImageableWidth())));
So, you went ahead with updating the formatting… In this case, be consistent:
Suggestion:
myImageableRightLabel.setText("Format Right Margin = "
+ (myPageFormat.getWidth()
- (myPageFormat.getImageableX() +
myPageFormat.getImageableWidth())));
Wrap the line after the text label; wrap more if necessary.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 87:
> 85: + myPageFormat.getImageableY());
> 86: myImageableBottomLabel.setText("Format Bottom Margin = " +
> (myPageFormat.getHeight()
> 87: - (myPageFormat.getImageableY() +
> myPageFormat.getImageableHeight())));
Suggestion:
myImageableBottomLabel.setText("Format Bottom Margin = "
+ (myPageFormat.getHeight()
- (myPageFormat.getImageableY() +
myPageFormat.getImageableHeight())));
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 116:
> 114: pgih.setText("Paper Imageable Height = " +
> p.getImageableHeight());
> 115: pgbm.setText("Paper Bottom Margin = " + (p.getHeight()
> 116: - (p.getImageableY() + p.getImageableHeight())));
Suggestion:
pgrm.setText("Paper Right Margin = "
+ (p.getWidth()
- (p.getImageableX() + p.getImageableWidth())));
pgtm.setText("Paper Top Margin = " + p.getImageableY());
pgih.setText("Paper Imageable Height = " + p.getImageableHeight());
pgbm.setText("Paper Bottom Margin = "
+ (p.getHeight()
- (p.getImageableY() + p.getImageableHeight())));
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 177:
> 175: } catch (PrinterException pe) {
> 176: PassFailJFrame.forceFail( "Test Failed");
> 177: pe.printStackTrace();
Suggestion:
pe.printStackTrace();
PassFailJFrame.forceFail("Test failed because of
PrinterException");
First, print the stack trace; then fail the test. Otherwise, printing the stack
trace may not finish because jtreg will be stopping the test.
I suggest adding some details of why the test fails.
Please update all the cases.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 51:
> 49: "You must have a printer available to perform this test.\n" +
> 50: "This test silently starts a print job and while the job
> is\n" +
> 51: "still being printed, cancels the print job\n" +
Not silently: the tester has to click OK / Print button in the displayed dialog.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 74:
> 72: Thread.sleep(5000);
> 73: pjc.pj.cancel();
> 74: }
Should the test fail if the tester clicks Cancel instead of OK / Print?
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 97:
> 95: PassFailJFrame.forceFail("This is wrong .. we shouldn't be
> here, " +
> 96: "Looks like a test failure");
> 97: prex.printStackTrace();
Print the stack trace first.
The error message can be more concise.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 102:
> 100: if (!cancelWorked) {
> 101: PassFailJFrame.forceFail("Looks like the test failed -
> we didn't get " +
> 102: "the expected
> PrintAbortException ");
Suggestion:
PassFailJFrame.forceFail("Didn't get the expected
PrintAbortException ");
Straight to the point.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 119:
> 117: g2d.drawString(("This is page" + (pidx + 1)), 60, 80);
> 118: // Need to slow things down a bit .. important not to try this
> 119: // on the event dispathching thread of course.
Suggestion:
// on the event dispatching thread of course.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 123:
> 121: Thread.sleep(2000);
> 122: } catch (InterruptedException e) {
> 123: }
Suggestion:
} catch (InterruptedException ignored) {
}
To avoid an IDE warning.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 43:
> 41: public class PrintAllFonts implements Printable {
> 42:
> 43: private static Font[] allFonts;
Suggestion:
private final Font[] allFonts =
GraphicsEnvironment.getLocalGraphicsEnvironment()
.getAllFonts();
I suggest initialising the list of fonts right here. Otherwise, it looks
inconsistent. And `allFonts` should be an instance field for the class.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 44:
> 42:
> 43: private static Font[] allFonts;
> 44: private int lineHeight = 18;
Suggestion:
private static final int lineHeight = 18;
It's a constant. You may want to use the constant naming style `LINE_HEIGHT`. I
also suggest moving it above `allFonts`
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 53:
> 51: "\n" +
> 52: "This bug is system dependent and is not always
> reproducible.\n" +
> 53: "A passing test will have all text printed with correct font
> style.";
Clarify that the second column should be proper *italics*. I don't know how to
explain it better; you can look at the PDFs attached to
[JDK-4884389](https://bugs.openjdk.org/browse/JDK-4884389) to understand better
what the bug was.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 91:
> 89: }
> 90:
> 91: int fontsPerPage = (int) pf.getImageableHeight() / lineHeight;
Suggestion:
int fontsPerPage = (int) pf.getImageableHeight() / lineHeight - 1;
Allow for some margin at the end of the page, otherwise most fonts printed at
the bottom of a page are clipped.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 98:
> 96: for (int n = 0; n < fontsPerPage; n++) {
> 97: Font f = allFonts[fontNum].deriveFont(Font.PLAIN, 14);
> 98: Font fi = allFonts[fontNum].deriveFont(Font.ITALIC, 14);
I suggest declaring the font size as a constant at the top of the class.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 71:
> 69: myImageableXLabel.setText("Format Left Margin = " +
> drnd(myPageFormat.getImageableX()));
> 70: myImageableRightLabel.setText("Format Right Margin = " +
> drnd(myPageFormat.getWidth()
> 71: - (myPageFormat.getImageableX() +
> myPageFormat.getImageableWidth())));
Please update these long lines to the same style as in `PageSetupDialog.java`.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 113:
> 111: } else {
> 112: return ds;
> 113: }
Suggestion:
return String.format("%.2f", d);
I believe it conveys the meaning better.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 190:
> 188: PassFailJFrame.forceFail( "Test Failed");
> 189: pe.printStackTrace();
> 190: }
Provide a better description of the failure, at least mention
`PrinterException` was caught.
Print the stack trace before failing the test to ensure it's printed.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 218:
> 216: PassFailJFrame.forceFail( "Test Failed");
> 217: nfe.printStackTrace();
> 218: }
This should rather display a warning to the tester so that they're able to
correct it and continue with the test.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 54:
> 52:
> 53: private static final String INSTRUCTIONS =
> 54: "You must have a printer available to perform this test\n" +
I guess the first line can be removed from all the instructions: the test fails
if there's no printer.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17607#pullrequestreview-1877534530
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487577897
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487564554
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487564996
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487566382
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487572349
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487588360
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487601043
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487604997
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487606685
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487609576
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487610050
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487905573
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487901504
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487913061
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487916974
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487924430
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487921062
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487946454
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487949106
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487951759
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1487960033
