-
Notifications
You must be signed in to change notification settings - Fork 235
Merge upstream changes from Warp's core-foundation-rs fork #517
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
Add get_matrix to core text font
Expose the typographic bounds as a function within the CoreText Run
Fix Ventura Crash
cc @madsmtm--tagging you in case you'd be interested in reviewing. I think the changes here (notably the change to fix a crash on Ventura) would be pretty valuable to be merged upstream. Thanks! |
Can you clean up the commit history and avoid the formatting changes? |
Can you also give some example code or add a test case for reproducing the Ventura failure? |
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.
Don't know much about CoreText, but probably looks fine to me
☔ The latest upstream changes (presumably b009c87) made this pull request unmergeable. Please resolve the merge conflicts. |
In the name of finally getting (at least some of) these changes merged upstream, I've sent out #674 and #675 for the bits adding new functions. In terms of the Ventura fix, I can't think of an easy way to write a test for it. FWIW, this has been live in Warp (used by ~100k macOS users) for a year now with no indication of it causing any issues. I can send out a separate PR for that as well, or we can keep it in our own fork; either works for us. |
I definitely think we will want this fix upstream. A separate PR might make sense at this point seeing as it has been done for the other changes.
I think the best test for this would be a usage of the API that triggers the bug without the fix if you have a standalone reproduction (perhaps using a particular system font). But unless anyone else strongly objects, I think it would be better to land this without a test than not at all if you are unable to provide that. A link to the original bug in warp that lead to this fix would also potentially be helpful. I'm sorry that progress on reviewing/merging this has been so slow. The Servo project is still getting back up to speed after a long hiatus and is still quite behind on maintenance of libraries. Hopefully this will improve over the next few months. |
Yeah we (@alokedesai or I) can put together a separate PR for the No need to apologize for latency here - there was an explicit request to clean up the commit history and provide some example/test code for the |
Hey all, finally got around to sending out one last PR for the Ventura crash fix portion of this PR: #735 As mentioned in the PR description, we attempted ~6 months ago to switch to the published version of this crate (in case Apple had fixed the underlying issue in a newer release) but immediately had to roll back that release of Warp due to it still being an issue in the wild. |
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'm fine with these changes but it would be nice to clean up the commit history as previously requested.
With the PR you just approved, we'll be able to close this out - I've already merged separate PRs for the other bits. (Feel free to close this one out now and we can focus on getting #735 merged.) |
Cool, let's do that! |
Hey servo folks,
Sending a PR to merge Warp's changes to core-foundation-rs upstream. Note, these changes are purely additive, and shouldn't have an affect on any existing core-foundation-rs APIs:
Changes
get_matrix
toCTFont
, matching CTFontGetMatrix.get_typographic_bounds
toCTRun
, matching CTRunGetTypographicBoundsextract_number_for_key
that occurs in Ventura. The underlying issue is an issue within the Core Framework library on Ventura--but this patch allows this library to not panic on Ventura if someone tries to access any of the font traits.