On Mon, 5 May 2025 23:45:36 GMT, Harshitha Onkar <hon...@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. > > src/java.desktop/macosx/classes/sun/font/CCharToGlyphMapper.java line 138: > >> 136: return code == 0x0009 || code == 0x000a || code == 0x000d; >> 137: } >> 138: > > @prrace @gredler > Following is an existing issue NOT related to the recent harfbuzz upgrade PR > nor this issue fix but it is similar in nature so a separate JBS issue can be > created to handle it. I wanted to discuss it before integrating the HB > upgrade tomorrow. > > There seems to be similar issue on windows and linux where the hexcode - > 0x2028 (line separator) and 0x2029 (paragraph separator) are shown with a > non-zero advance - `/java/awt/font/TextLayout/TestControls.java` > > **On Windows:** > > <img width="287" alt="image" > src="https://github.com/user-attachments/assets/9eec9009-e3a5-49be-b505-619d041bfa3b"; > /> > > **On Linux:** > > <img width="272" alt="image" > src="https://github.com/user-attachments/assets/73a56d6d-53ec-4d6f-ad49-6fc13bdceea0"; > /> > > > If adding these codes (0x2028, 0x2029) to platform specific src code (I > believe it is NativeGlyphMapper (linux) and WPathGraphics (windows)) fixes > the above issue then should we consider moving isIgnorableWhitespace() to > common class - `CharToGlyphMapper` instead of macOS specific src? Yeah, I think right now `\t` `\r` `\n` handling is done separately for each platform, and I think also separately for screen display vs. printing. As an example, this fix addresses the screen display side of things for macOS, but printing on macOS needs to be fixed separately (as can be seen when you run `test/jdk/java/awt/print/PrinterJob/PrintTextTest.java`). I had a quick look at the corresponding macOS printing issue a few months ago and the fix wasn't obvious, especially given how that part of the code is organized (not really 1-to-1 with the other platforms). So I think `\t` `\r` `\n` handling is repeated quite a bit, and if we wanted to handle `0x2028` and `0x2029` exactly like we do `\t` `\r` `\n` then we'd need to change multiple places right now. > should we consider moving isIgnorableWhitespace() to common class - > CharToGlyphMapper instead of macOS specific src That sounds like it could be a good idea, but I don't have a sense of how likely it is to cause breakage elsewhere -- it'd be a matter of "do it and check for test regressions". Maybe you have a better gut feel for the likelihood of unintended side-effects? Note these two characters (`0x2028` and `0x2029`) are not default-ignorable, though. It sounds like we just don't want to render their glyphs, if they have any (similar to `\t` `\r` `\n`, which are also not default-ignorable). Over on PR #24412 Nikita has suggested an interesting change to `CharToGlyphMapper` and subclasses which might make this change easier, but I'd like some feedback from a reviewer or two before moving forward. The PR is complete from my POV, but we need some design direction (good as is, or needs tweaks, or need to use a different approach entirely, etc). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2076538725
