On Tue, 5 Nov 2024 17:41:07 GMT, Daniel Gredler <d...@openjdk.org> wrote:
>> There are multiple issue with this test case
>> 1) Parser error due to yesno in @run main/manual=yesno
>> 2) User can only compare the UI rendering and compare with the print out.
>> User can't mark the test as pass or fail due to pass or fail buttons are
>> missing.
>> 3) When the test is executed using jtreg after user click on the print
>> button on the print dialog the whole test UIs ( frames) gets dispose and
>> user cannot compare the printout with the UI. But this works as expected in
>> test is running individually using java PrintTextTest
>
> Daniel Gredler has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Separate paint logic from test class
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 84:
> 82:
> 83: public static void main(String[] args) throws Exception {
> 84:
I think blank lines at the start of methods are redundant. I don't insist on
removing them though.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 88:
> 86: PageFormat portrait = pjob.defaultPage();
> 87: portrait.setOrientation(PageFormat.PORTRAIT);
> 88: int preferredSize = (int) portrait.getImageableWidth();
The `preferredSize` could've remained static… to save some typing. Yet passing
it explicitly to objects is even better.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 101:
> 99: String name = "Page " + page++;
> 100: PrintText pt = new PrintText(name, font, null, false,
> preferredSize);
> 101: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
I think
[`JTabbedPane.addTab`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/javax/swing/JTabbedPane.html#addTab(java.lang.String,java.awt.Component))
should be used to add components rather than inherited `add`.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 128:
> 126: book.append(pt, landscape);
> 127:
> 128: font = new Font("Dialog", Font.PLAIN, 18);
Suggestion:
font = new Font(Font.DIALOG, Font.PLAIN, 18);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 136:
> 134: book.append(pt, landscape);
> 135:
> 136: font = new Font("Dialog", Font.PLAIN, 18);
Suggestion:
font = new Font(Font.DIALOG, Font.PLAIN, 18);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 145:
> 143: book.append(pt, landscape);
> 144:
> 145: font = new Font("Dialog", Font.PLAIN, 18);
Suggestion:
font = new Font(Font.DIALOG, Font.PLAIN, 18);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 158:
> 156: pt = new PrintText(name, font, null, false, preferredSize);
> 157: pane.add(pt, BorderLayout.CENTER);
> 158: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 162:
> 160: book.append(pt, landscape);
> 161:
> 162: font = new Font("Monospaced", Font.PLAIN, 12);
Suggestion:
font = new Font(Font.MONOSPACED, Font.PLAIN, 12);
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 166:
> 164: pt = new PrintText(name, font, null, false, preferredSize);
> 165: pane.add(pt, BorderLayout.CENTER);
> 166: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
One shouldn't add the same component twice. (Yes, I understand it's existing
code, but it's worth fixing.)
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 174:
> 172: pt = new PrintText(name, xfont, null, false, preferredSize);
> 173: pane.add(pt, BorderLayout.CENTER);
> 174: pane.add(name, pt);
Suggestion:
pane.addTab(name, pt);
And the following cases.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 214:
> 212: }
> 213: } catch (PrinterException e) {
> 214: throw new RuntimeException(e.getMessage());
Suggestion:
throw new RuntimeException(e.getMessage(), e);
The original exception could be very useful for debugging a failure.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 232:
> 230: }
> 231:
> 232: // The test needs a physical font that supports Latin.
The `getPhysicalFont` could be updated so that its `switch` statement uses
`Font.` constants, I'm pretty sure that was the intention.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 268:
> 266: protected String page;
> 267: protected boolean useFM;
> 268: protected int preferredSize;
All the fields could be `final`, the values aren't modified.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 279:
> 277: }
> 278:
> 279: public static AttributedCharacterIterator getIterator(String s) {
Suggestion:
private static AttributedCharacterIterator getIterator(String s) {
I think the `private` modifier helps understand that the method is local to the
class.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 283:
> 281: }
> 282:
> 283: static String orient(PageFormat pf) {
Suggestion:
private static String orient(PageFormat pf) {
I'd make it `private` too.
test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 291:
> 289: }
> 290:
> 291: public int print(Graphics g, PageFormat pf, int pageIndex) {
Could you add `@Override` annotations to the overridden methods, please?
-------------
PR Review: https://git.openjdk.org/jdk/pull/21716#pullrequestreview-2416362198
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829800180
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829799250
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829810540
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829803296
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829811125
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829803962
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829811638
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829804562
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829811999
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829812233
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829814875
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829825143
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829815515
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829816027
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829818063
PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829818749
