On Wed, 7 May 2025 22:23:02 GMT, Daniel Gredler <dgred...@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
