-
Notifications
You must be signed in to change notification settings - Fork 225
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
Merge upstream changes from Warp's core-foundation-rs fork #517
base: main
Are you sure you want to change the base?
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 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.