Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

alokedesai
Copy link

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

  1. Adds get_matrix to CTFont, matching CTFontGetMatrix.
  2. Adds get_typographic_bounds to CTRun, matching CTRunGetTypographicBounds
  3. Adds a patch for a panic in extract_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.

@alokedesai
Copy link
Author

alokedesai commented Jun 27, 2022

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!

@jrmuizel
Copy link
Collaborator

Can you clean up the commit history and avoid the formatting changes?

@jrmuizel
Copy link
Collaborator

Can you also give some example code or add a test case for reproducing the Ventura failure?

Copy link
Contributor

@madsmtm madsmtm left a 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

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably b009c87) made this pull request unmergeable. Please resolve the merge conflicts.

@vorporeal
Copy link
Contributor

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.

@nicoburns
Copy link

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.

In terms of the Ventura fix, I can't think of an easy way to write a test for it.

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.

@vorporeal
Copy link
Contributor

vorporeal commented May 1, 2024

Yeah we (@alokedesai or I) can put together a separate PR for the extract_number_for_key fix.

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 extract_number_for_key issue, and nobody from our side followed up. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants