RFR: 8350203: [macos] Newlines and tabs are not ignored when drawing text to a Graphics2D object [v2]

[Alexey Ivanov]

On Wed, 7 May 2025 22:23:02 GMT, Daniel Gredler <dgredler at openjdk.org> wrote:

>> On other platforms like Windows and Linux, the `\n`, `\r` and `\t` characters are ignored when drawing text to a `Graphics2D` object. On macOS this is not currently the case.
>> 
>> See, for example, `CMap.getControlCodeGlyph(int, boolean)` or `RasterPrinterJob.removeControlChars(String)`.
>> 
>> This bug was found while running `test/jdk/java/awt/print/PrinterJob/PrintTextTest.java` on macOS.
>> 
>> The new test class passes on Linux, Windows and macOS.
>
> Daniel Gredler has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' into ignored-whitespace
>  - Make Graphics2D.drawString ignore tabs and newlines on macOS
>  - Add actual bug ID
>  - Add ignored whitespace test

Looks good to me, except for formatting suggestions.

test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java line 24:

> 22:  */
> 23: 
> 24: /**

Suggestion:

/*

It shouldn't use a javadoc comment syntax.

test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java line 46:

> 44:     public static void main(String[] args) throws Exception {
> 45: 
> 46:         BufferedImage image = new BufferedImage(600, 600, BufferedImage.TYPE_BYTE_BINARY);

Suggestion:

    public static void main(String[] args) throws Exception {
        BufferedImage image = new BufferedImage(600, 600, BufferedImage.TYPE_BYTE_BINARY);

Just two methods start with a (redundant) blank line.

test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java line 80:

> 78:     private static void test(BufferedImage image, Graphics2D g2d, Font font, String reference, String text) {
> 79: 
> 80:         g2d.setFont(font);

Suggestion:

    private static void test(BufferedImage image, Graphics2D g2d, Font font, String reference, String text) {
        g2d.setFont(font);

test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java line 124:

> 122:     private static void assertEqual(Rectangle r1, Rectangle r2, String text) {
> 123:         if (!r1.equals(r2)) {
> 124:             String escaped = text.replace("\r", "\\r").replace("\n", "\\n").replace("\t", "\\t");

I would format this way, so that each `replace` stands out in the chain of calls.
Suggestion:

            String escaped = text.replace("\r", "\\r")
                                 .replace("\n", "\\n")
                                 .replace("\t", "\\t");

test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java line 159:

> 157:         int height = image.getHeight();
> 158:         int[] rowPixels = new int[width];
> 159:         for (int y = 0; y < height; y++) {

Suggestion:

        int height = image.getHeight();

        int[] rowPixels = new int[width];
        for (int y = 0; y < height; y++) {

I suggest having a blank line here to introduce a new logical ‘block’ of code which iterates over the pixels in the image.

test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java line 179:

> 177:             }
> 178:         }
> 179:         if (minX != Integer.MAX_VALUE) {

Suggestion:

        }

        if (minX != Integer.MAX_VALUE) {

A blank line here breaks a long method into sections.

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

Marked as reviewed by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23665#pullrequestreview-2828951322
PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2082035028
PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2082034595
PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2082033475
PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2082045776
PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2082038800
PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2082036239