On Mon, 4 Mar 2024 06:04:07 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:
>
> Suggesions incorporated
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 239:
> 237: PassFailJFrame.builder()
> 238: .instructions(INSTRUCTIONS)
> 239: .testUI(PageSetupDialog::new)
Suggestion:
PassFailJFrame.builder()
.instructions(INSTRUCTIONS)
.testTimeOut(10)
.testUI(PageSetupDialog::new)
Given the number of cases to go through, I suggest increasing the timeout to 10
minutes at least.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 50:
> 48: "progress automatically cancels the print job.\n" +
> 49: "You should see a message on System.out that the job\n" +
> 50: "was properly cancelled.";
Suggestion:
"Test that print job cancellation works.\n\n" +
"This test starts after clicking OK / Print button.
"While the print job is in progress, the test automatically cancels
it.\n" +
"The test will complete automatically.";
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 69:
> 67: pjc.pj.cancel();
> 68: }
> 69: else {
Suggestion:
} else {
Use Java style.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 70:
> 68: }
> 69: else {
> 70: PassFailJFrame.forceFail("User cancelled");
Be more specific?
Suggestion:
PassFailJFrame.forceFail("User cancelled printing");
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 93:
> 91: prex.printStackTrace();
> 92: PassFailJFrame.forceFail("This is wrong .. we shouldn't be
> here, " +
> 93: "Looks like a test failure");
Suggestion:
PassFailJFrame.forceFail("Unexpected PrinterException caught: " +
prex.getMessage());
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 45:
> 43: private static final int FONT_SIZE = 14;
> 44: private final Font[] allFonts =
> 45:
> GraphicsEnvironment.getLocalGraphicsEnvironment().getAllFonts();
I suggest adding blank lines to visually separate fields / constants with
different purpose.
Suggestion:
private static final int LINE_HEIGHT = 18;
private static final int FONT_SIZE = 14;
private final Font[] allFonts =
GraphicsEnvironment.getLocalGraphicsEnvironment().getAllFonts();
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 45:
> 43: boolean done = false;
> 44: int thisPage = 0;
> 45:
This blank was actually good, it separated the instance fields from the
instructions constant.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 61:
> 59: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
> 60: .instructions(INSTRUCTIONS)
> 61: .rows((int) INSTRUCTIONS.lines().count() + 1)
Suggestion:
.instructions(INSTRUCTIONS)
.testTimeOut(10)
.rows((int) INSTRUCTIONS.lines().count() + 1)
The list of fonts could be long, and the tester may need to go to fetch the
printouts from a printer.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 69:
> 67: if (pj.printDialog()) {
> 68: pj.print();
> 69: }
Fail the test if the user clicks Cancel in the print dialog?
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 214:
> 212: JOptionPane.showMessageDialog(null,
> 213: "NumberFormatException occured", "Error",
> 214: JOptionPane.ERROR_MESSAGE);
Suggestion:
JOptionPane.showMessageDialog(ValidatePage.this,
"NumberFormatException occurred", "Error",
JOptionPane.ERROR_MESSAGE);
You may provide more information to the user, at the very least the message
from the exception itself. Add a comment that the tester should correct the
error and try again.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 233:
> 231: "needed to accomodate the imageable area.\n \n \n" +
> 232: "To test 6229507, put the minimum margins (all 0s) in
> Page Setup dialog.\n" +
> 233: "Compare Imageable width, height, and margins of
> portrait against landscape.");
Suggestion:
"When validating a page, the process is 1st to find the closest
matching\n" +
"paper size, next to make sure the requested imageable area
fits within\n" +
"the printer's imageable area for that paper size. Finally the
top and\n" +
"left margins will be shrunk if they are too great for the
adjusted\n" +
"imageable area to fit at that position. They will shrink by
the minimum\n" +
"needed to accomodate the imageable area.\n\n\n" +
"To test 6229507, put the minimum margins (all 0s) in Page
Setup dialog.\n" +
"Compare Imageable width, height, and margins of portrait
against landscape.");
The spaces at the end of lines are redundant.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 301:
> 299: PassFailJFrame.builder()
> 300: .instructions(INSTRUCTIONS)
> 301: .testUI(ValidatePage::new)
Suggestion:
.instructions(INSTRUCTIONS)
.testTimeOut(15)
.testUI(ValidatePage::new)
The test requires much interaction, so the timeout should be increased
accordingly.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 92:
> 90: PrinterJob pj = PrinterJob.getPrinterJob();
> 91:
> 92: if (pj != null && pj.printDialog()) {
Suggestion:
if (pj.printDialog()) {
The contract of
[`PrinterJob.getPrinterJob`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/print/PrinterJob.html#getPrinterJob())
doesn't allow returning `null`, so the null-check can be removed, which will
resolve a warning from IDE.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 105:
> 103: private static class RasterCanvas extends Canvas implements
> Printable {
> 104:
> 105: public int print(Graphics g, PageFormat pgFmt, int pgIndex) {
Suggestion:
@Override
public int print(Graphics g, PageFormat pgFmt, int pgIndex) {
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 118:
> 116: }
> 117:
> 118: public void paint(Graphics g) {
Suggestion:
@Override
public void paint(Graphics g) {
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 125:
> 123: doPaint(g);
> 124: }
> 125:
Suggestion:
The `paintComponent` can be removed; `Canvas` doesn't have it, only Swing
components extending `JComponent` have `paintComponent` method.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 126:
> 124: }
> 125:
> 126: public void doPaint(Graphics g) {
Suggestion:
private void doPaint(Graphics g) {
Should be `private`.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 150:
> 148: }
> 149:
> 150: public Dimension getPreferredSize() {
Suggestion:
@Override
public Dimension getPreferredSize() {
Let's add `@Override` annotations for clarity when a method is overridden or an
interface method is implemented.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17607#pullrequestreview-1916685779
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512702925
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512710224
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512705603
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512712351
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512714468
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512718756
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512719757
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512742727
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512739665
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512749442
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512750500
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512754795
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512761318
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512768938
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512768757
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512766713
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512765313
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1512767988
