On Tue, 6 May 2025 23:31:34 GMT, Daniel Gredler <dgred...@openjdk.org> wrote:
>> 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). @gredler This issue _[JDK-8356803](https://bugs.openjdk.org/browse/JDK-8356803) - Test TextLayout/TestControls fails on windows & linux line and paragraph separator show a non-zero advance_, is re-assigned to you since it is a regression of [JDK-8208377](https://bugs.openjdk.org/browse/JDK-8208377) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23665#discussion_r2085495237
