On Tue, 19 Sep 2023 14:45:47 GMT, Michal Sobierski <d...@openjdk.org> wrote:
> For NullClipARGB we are throwing SkippedException only if instance of caught
> exception is PrinterException. All other exceptions are considered unrelated
> to printer configuration.
>
> For CountPrintServices we want to throw SkippedException in the case when
> lpstat is not installed, where an IOException is caught in that case.
>
> For ExceptionTest we want to throw SkippedException for all PrinterExceptions
> which were not caused by IndexOutOfBoundsException.
>
> For rest of the tests if PrintService was not created instead
> RuntimeException we are throwing now SkippedException.
>
> Tested on environment without printer installed.
> Test are passing with skipped exception as expected.
>
> Passed: java/awt/print/PrinterJob/ExceptionTest.java
> Passed: java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java
> Passed: java/awt/print/PrinterJob/PrtException.java
> Passed: javax/print/attribute/AttributeTest.java
> Passed: javax/print/attribute/GetCopiesSupported.java
> Passed: javax/print/attribute/SidesPageRangesTest.java
> Passed: javax/print/attribute/SupportedPrintableAreas.java
> Passed: javax/print/PrintServiceLookup/CountPrintServices.java
> Passed: javax/print/CheckDupFlavor.java
>
> Additionally unused imports have been removed.
The copyright years need to be adjusted a bit.
Copyright (c) 2007, Oracle and/or its affiliates. All rights reserved.
...should become:
Copyright (c) 2007, 2023, Oracle and/or its affiliates. All rights reserved.
Apart from stylistic comments, this looks good.
Looks good to me. Once OCA clears, someone familiar with this area would need
to take a look too.
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 43:
> 41:
> 42: public static void main( String[] args ) {
> 43: PrintService[] svc =
> PrintServiceLookup.lookupPrintServices(DocFlavor.SERVICE_FORMATTED.PRINTABLE,
> null);
Do we need to ask for `DocFlavor.SERVICE_FORMATTED.PRINTABLE` here? Can we just
ask for `null`?
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 43:
> 41: try {
> 42: PrinterJob pj = PrinterJob.getPrinterJob();
> 43: pj.setPrintable(new NullClipARGB());
I see this is a cleanup, but we don't need to do it here, to keep patch simple
and backportable.
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 47:
> 45: } catch (Exception ex) {
> 46: if (ex instanceof PrinterException) {
> 47: throw new SkippedException("Printer is required for this
> test. TEST ABORTED");
I think for this test, we need to check
`PrintServiceLookup.lookupPrintServices(DocFlavor.SERVICE_FORMATTED.PRINTABLE,
null);`, like other tests do it. Let's not assume that `PrinterException` is
thrown here only when printer is not configured. Add the block right at the
beginning of `main`?
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 53:
> 51: pj.print();
> 52: } catch (Exception ex) {
> 53: throw new RuntimeException(ex);
One more redundant whitespace change.
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 58:
> 56:
> 57: public int print(Graphics g, PageFormat pf, int pageIndex)
> 58: throws PrinterException{
Redundant whitespace change.
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 58:
> 56:
> 57: public int print(Graphics g, PageFormat pf, int pageIndex)
> 58: throws PrinterException{
One more redundant whitespace change.
test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java line 71:
> 69: g2.drawString("This text should be visible through the image", 0,
> 20);
> 70: BufferedImage bi = new BufferedImage(100, 100,
> 71: BufferedImage.TYPE_INT_ARGB );
Redundant whitespace change.
test/jdk/javax/print/CheckDupFlavor.java line 33:
> 31:
> 32: import javax.print.*;
> 33: import javax.print.attribute.*;
Here and later, keep the imports in place, for the same reason: targeted
backportable patch.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15821#pullrequestreview-1633824490
PR Review: https://git.openjdk.org/jdk/pull/15821#pullrequestreview-1635371892
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15821#pullrequestreview-1635892563
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331464422
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331465944
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1330467074
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331726090
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331462305
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331726158
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331462355
PR Review Comment: https://git.openjdk.org/jdk/pull/15821#discussion_r1331466547
