RFR: 8307246 : Printing: banded raster path doesn't account for device offset values [v13]

[Alexey Ivanov]

On Tue, 13 Feb 2024 12:06:25 GMT, vtstydev <duke at openjdk.org> wrote:

>> More correct way to take in consideration nonzero PHYSICALOFFSETX, PHYSICALOFFSETY of device for banded-raster printing loop. Only on Windows platform under certain conditions real device prints shifted image on paper.
>
> vtstydev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Done requested fixes 5

The copyright must contain two years at most, you modify the second one to be the current year. With the three years, it will break the build because of incorrect copyright header.

src/java.desktop/share/classes/sun/print/RasterPrinterJob.java line 2:

> 1: /*
> 2:  * Copyright (c) 1998, 2023, 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 1998, 2024, Oracle and/or its affiliates. All rights reserved.

test/jdk/java/awt/print/PrinterJob/ImagePrinting/AlphaPrintingOffsets.java line 60:

> 58:             "Tested bug occurs only on-paper printing " +
> 59:                     "so you mustn't use PDF printer\n\n" +
> 60:                     "1. Java print dialog should appear.\n" +

Suggestion:

                    "Tested bug occurs only on-paper printing " +
                    "so you mustn't use PDF printer\n\n" +
                    "1. Java print dialog should appear.\n" +

Alternatively, reduce the indentation of *all* the lines with the instructions.

test/jdk/java/awt/print/PrinterJob/ImagePrinting/AlphaPrintingOffsets.java line 86:

> 84:             }
> 85:             PassFailJFrame.builder().instructions(instructionsHeader + INSTRUCTIONS)
> 86:                     .rows(15).testUI(() -> createTestUI()).build().awaitAndCheck();

Suggestion:

            PassFailJFrame.builder()
                    .instructions(instructionsHeader + INSTRUCTIONS)
                    .rows(15)
                    .testUI(() -> createTestUI())
                    .build()
                    .awaitAndCheck();

What do you think? Is it easier to read?

test/jdk/java/awt/print/PrinterJob/ImagePrinting/AlphaPrintingOffsets.java line 92:

> 90:                     "Printer not configured or available.");
> 91:             throw new RuntimeException("Test failed : " +
> 92:                     "Printer not configured or available.");

I'd rather keep those unmodified, the string is not so long, and wrapping it doesn't improve readability.

In my opinion, `println` is redundant, the runtime exception conveys it.

test/jdk/java/awt/print/PrinterJob/ImagePrinting/AlphaPrintingOffsets.java line 138:

> 136:             printerJob.setPageable(bookPageSavingTest);
> 137:         }
> 138:         else {

Suggestion:

        } else {

This is the Java style and you follow this style in other places.

-------------

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17030#pullrequestreview-1878115023
PR Review Comment: https://git.openjdk.org/jdk/pull/17030#discussion_r1487968754
PR Review Comment: https://git.openjdk.org/jdk/pull/17030#discussion_r1487975023
PR Review Comment: https://git.openjdk.org/jdk/pull/17030#discussion_r1487988493
PR Review Comment: https://git.openjdk.org/jdk/pull/17030#discussion_r1487980225
PR Review Comment: https://git.openjdk.org/jdk/pull/17030#discussion_r1487986622