On Mon, 4 Mar 2024 06:48:18 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 added
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 42:
> 40: */
> 41: public class PageDlgPrnButton implements Printable
> 42: {
Suggestion:
public class PageDlgPrnButton implements Printable {
Keep the opening brace on the same line.
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 44:
> 42: {
> 43: private static final String INSTRUCTIONS =
> 44: "You must have at least 2 printers available to perform this
> test.\n" +
Suggestion:
The first line can be removed, the test verifies this condition before starting.
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 52:
> 50: if (PrinterJob.lookupPrintServices().length < 2) {
> 51: throw new RuntimeException("Printer not configured or
> available.");
> 52: }
This is inaccurate now. We need to cases here:
- No printer configured — fail the test because the test has `@key printer`.
- There are less than 2 printers — throw `SkippedException` because the test
cannot continue and there's no way to automatically check for it.
Suggestion:
final int serviceCount = PrinterJob.lookupPrintServices().length;
if (serviceCount == 0) {
throw new RuntimeException("Printer not configured or available.");
}
if (serviceCount < 2) {
throw new SkippedException("The test requires at least 2
printers.");
}
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 73:
> 71: if (originalPageFormat == pageFormat) {
> 72: throw new RuntimeException("OriginalPageFormat equals
> pageFormat");
> 73: }
This does not make sense to me… All code paths seem to return a new object all
the times. It is impossible to for these two objects to be identical.
I can't find JDK-4956397 in JBS.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 57:
> 55: "Printing must be done in Japanese 2nd Edition.\n" +
> 56: "Preferably Canon LaserShot A309GII.\n" +
> 57: "\n" +
Suggestion:
I believe we can remove the first part of the instructions. The original bug
[JDK-4298489](https://bugs.openjdk.org/browse/JDK-4298489) says printing by the
first method was ugly and the second one was good. Both methods should produce
the identical results.
According to the bug description, it was reported against Windows 95 OSR 2
although the instructions in the test refer to Windows 98 Second Edition and
its Japanese version for whatever reason.
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 59:
> 57: "\n" +
> 58: "Passing test : Output of text image for PrintTest1 and
> PrintTest2 should be\n" +
> 59: "same as that on the screen.";
Expand the instructions:
Suggestion:
"Select PrintTest1 in the File menu.\n" +
"Print Dialog will appear.\n" +
"Click OK to start the first print job.\n" +
"\n" +
"Select PrintTest2 in the File menu.\n" +
"Page Setup Dialog will appear.\n" +
"Click OK.\n" +
"Print Dialog will appear.\n" +
"Click OK to start the second print job.\n" +
"\n" +
"The text in the printouts for PrintTest1 and PrintTest2 should
be\n" +
"same as that on the screen.\n" +
"Press Pass if they are, otherwise press Fail.";
test/jdk/java/awt/print/PrinterJob/PrintImage.java line 93:
> 91: me.add(print2Menu);
> 92: mb.add(me);
> 93: this.setMenuBar(mb);
Suggestion:
setMenuBar(mb);
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 54:
> 52: "The messages should contain only 'OK' and 'expected'
> messages\n" +
> 53: "If the page fails to print, but there were no exceptions\n" +
> 54: "then the problem is likely elsewhere (ie your printer)";
Suggestion:
"This test should print a page which contains the same\n" +
"text messages as in the test window on the screen.\n" +
"\n" +
"The messages should contain only 'OK' and 'expected' messages.\n" +
"Press Pass if it's the case; otherwise press Fail.\n" +
"\n" +
"If the page fails to print, but there were no exceptions\n" +
"then the problem is likely elsewhere (i.e. your printer)";
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 76:
> 74: add("Center", c);
> 75:
> 76: JButton b = new JButton("Print");
Suggestion:
Button b = new Button("Print");
Better use `Button` with other AWT components.
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 122:
> 120: }
> 121:
> 122: public void paint(Graphics2D g2d) {
Suggestion:
private void paint(Graphics2D g2d) {
To avoid any confusion with public `paint` and `print`, even though it accepts
`Graphics2D` as parameter.
test/jdk/java/awt/print/PrinterJob/PrintNullString.java line 176:
> 174: }
> 175:
> 176: public Dimension getPreferredSize() {
Suggestion:
@Override
public Dimension getPreferredSize() {
test/jdk/java/awt/print/PrinterJob/PrintParenString.java line 48:
> 46: public class PrintParenString implements Printable {
> 47: private static final String STR = "String containing unclosed
> parenthesis (.";
> 48: private static final String INSTRUCTIONS =
Suggestion:
private static final String STR = "String containing unclosed parenthesis
(.";
private static final String INSTRUCTIONS =
I'd add a blank line between these two.
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 55:
> 53: "should be immediately under the text\n" +
> 54: "You should also monitor the command line to see if any
> exceptions\n" +
> 55: "were thrown\n" +
Suggestion:
The tester cannot see the console or any messages printed onto the console by
the test.
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 79:
> 77: add("Center", c);
> 78:
> 79: JButton b = new JButton("Print");
Suggestion:
Button b = new Button("Print");
It is better not to mix AWT and Swing components; you use `Frame` as the host
instead of `JFrame` in this case.
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 100:
> 98: }
> 99:
> 100: static class TextCanvas extends Panel implements Printable {
Suggestion:
private static class TextCanvas extends Panel implements Printable {
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 119:
> 117: }
> 118:
> 119: public void paint(Graphics2D g2d) {
Suggestion:
private void paint(Graphics2D g2d) {
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 143:
> 141: }
> 142:
> 143: public Dimension getPreferredSize() {
Suggestion:
@Override
public Dimension getPreferredSize() {
-------------
PR Review: https://git.openjdk.org/jdk/pull/17609#pullrequestreview-1917119360
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512898725
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512947071
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512908332
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512930564
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512971059
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512983320
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512983789
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1512996573
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513000538
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513003163
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513022787
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513008822
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513015864
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513018179
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513018642
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513019078
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1513019442
