-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8350203: [macos] Newlines and tabs are not ignored when drawing text to a Graphics2D object #23665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back dgredler! A progress list of the required criteria for merging this PR into |
|
@gredler This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 79 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@honkar-jdk, @prrace, @aivanov-jdk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
|
If anyone has a few free cycles, reviews for this PR would be much appreciated. Thanks! |
|
I am seeing an existing test fail on macOS with this change java NLGlyphTest This code |
|
OK, it looks like we've got a constant conflict between Java's I'm not sure where to take it from here. Java has been using Suggestions welcome!
|
That's unfortunate. |
|
Looking across the code base, there are a number of places which look explicitly for 0xFFFF (INVISIBLE_GLYPH_ID) and somewhat fewer that look for >= 0xFFFE (INVISIBLE_GLYPHS) That's an interesting inconsistency in itself. The idea of 0xFFFE might be worth a shot but it maybe that we need to make a bunch of changes to code that looks just for INVISIBLE_GLYPH_ID, including in some tests, but it also might be that since this is macOS specific that it could be less risky - although since the result could still be used by shared code it isn't quite as constrained as you might hope. |
|
I'm checking with the HarfBuzz guys to see if there's a possible fix on their end: harfbuzz/harfbuzz#5118 Unfortunately, it sounds like glyph ID |
|
Good news: it looks like it was possible to resolve the |
|
@gredler Yes, JDK harfbuzz was just upgraded to v10.4.0 - #23910. @prrace Do we consider patching the fix from upstream (harfbuzz/harfbuzz#5119) for this upgrade or can it wait till the next time harfbuzz is upgraded on jdk ? |
|
@honkar-jdk Thanks for the info -- note that it's actually a combination of two HB PRs (harfbuzz/harfbuzz#5119 and harfbuzz/harfbuzz#5126), and it sounds like they plan to cut a release with the fix later this week. |
|
Without even yet having looked at the fix in upstream I'll say something Upgrading harfbuzz is not frequent (we don't need busy work) but if there's a clear and valuable reason, it can be done. |
|
So let's see if we can integrate a new HB release in maybe 6 weeks from now, and then take up this PR again. |
|
@honkar-jdk @prrace Makes sense -- thanks for having a look, and I'll check back in on this one later in April. |
|
@honkar-jdk pls remember we need an updated harfbuzz for this PR to proceed. |
Noted. Thank you for the reminder. |
|
Hi @behdad, thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user behdad" for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gredler Tested your fix by applying your changes on top of harfbuzz upgrade - #24973 . Initial testing of your fix with IgnoredWhitespaceTest.java test and NLGlyphTest.java looks good. I haven't done a full fledged testing yet.
This PR fixes the same issue observed on another test - java/awt/font/TextLayout/TestControls.java - JDK-8353187. You can link the additional JBS ID to this PR with /issue add 8353187
I will be integrating harfbuzz upgrade PR soon after which you can sync your branch with mainline to test it on your end.
|
/issue add 8353187 |
|
@gredler |
|
@honkar-jdk Great, thanks! I'll keep an eye on the HarfBuzz upgrade PR and sync + test once it has been integrated. |
| private static boolean isIgnorableWhitespace(int code) { | ||
| return code == 0x0009 || code == 0x000a || code == 0x000d; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
On Linux:
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a new JBS to track the issue on windows and linux - JDK-8356803 - Test TextLayout/TestControls fails on windows & linux: line and paragraph separator show a non-zero advance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Are these characters new? I can't remember there were any characters with non-zero width on Linux and Windows. I ran this test (or its previous version) on each HarfBuzz update.
No, they were these characters are part of the existing test.
I cannot reproduce this problem with 21.0.6. Nor is it reproducible with 21.0.7, the latest GA version of Java 21.
Yet I the same picture as you attached on Windows on a recent build of mainline.
If that's the case, it seems to be regression rather than an existing issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also submitted JDK-8356812: Create an automated version of TextLayout/TestControls.
Verifying that the widths of all control characters is zero can be automated; the manual version can exist along-side to ensure all the characters are rendered as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aivanov-jdk Thanks for testing on older versions of JDK. I'm checking if this issue was caused by harfbuzz upgrade or recent code changes to JDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above issue is replicable after jdk25+b10 and was introduced by changeset related to JDK-8208377 - Soft hyphens render if not using TextLayout , #22670
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gredler This issue 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
|
I don't think are where I'd expect to find anything that could impact the TestControls test. But I agree with the general points
The other PR still needs a proper look on my part I suggest we get this PR pushed but first I'll submit a test job .. unless Harshitha already did that ? |
I haven't done a complete CI run along with this PR changes. |
I agree it was not as straightforward to see where to add the fix for windows and linux platforms. |
|
I haven't had time to test this latest merge on Windows, but my local testing of the current state on Linux and macOS looks good, so I'm cautiously optimistic. |
honkar-jdk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI Testing looks good. Didn't observe any regressions in the couple of manual tests that I ran with this fix. LGTM
aivanov-jdk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, except for formatting suggestions.
test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java
Outdated
Show resolved
Hide resolved
test/jdk/java/awt/Graphics2D/DrawString/IgnoredWhitespaceTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
|
@prrace @honkar-jdk I've incorporated a few minor formatting changes requested by Alexey. Please let me know if you have any concerns with these latest tweaks, otherwise I will go ahead and mark as ready for integration tomorrow. Thanks! |
honkar-jdk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/integrate |
|
/sponsor |
|
Going to push as commit 85db463.
Your commit was automatically rebased without conflicts. |
|
@aivanov-jdk @gredler Pushed as commit 85db463. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
On other platforms like Windows and Linux, the
\n,\rand\tcharacters are ignored when drawing text to aGraphics2Dobject. On macOS this is not currently the case.See, for example,
CMap.getControlCodeGlyph(int, boolean)orRasterPrinterJob.removeControlChars(String).This bug was found while running
test/jdk/java/awt/print/PrinterJob/PrintTextTest.javaon macOS.The new test class passes on Linux, Windows and macOS.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23665/head:pull/23665$ git checkout pull/23665Update a local copy of the PR:
$ git checkout pull/23665$ git pull https://git.openjdk.org/jdk.git pull/23665/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23665View PR using the GUI difftool:
$ git pr show -t 23665Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23665.diff
Using Webrev
Link to Webrev Comment