core-foundation-rs icon indicating copy to clipboard operation
core-foundation-rs copied to clipboard

Merge upstream changes from Warp's core-foundation-rs fork

Open alokedesai opened this issue 3 years ago • 7 comments

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 avatar Jun 27 '22 14:06 alokedesai

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!

alokedesai avatar Jun 27 '22 14:06 alokedesai

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

jrmuizel avatar Jun 27 '22 17:06 jrmuizel

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

jrmuizel avatar Jun 27 '22 17:06 jrmuizel

:umbrella: The latest upstream changes (presumably b009c87f2da0038bb474800cd88597843e5dbdae) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Jul 25 '23 02:07 bors-servo

In the name of finally getting (at least some of) these changes merged upstream, I've sent out https://github.com/servo/core-foundation-rs/pull/674 and https://github.com/servo/core-foundation-rs/pull/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.

vorporeal avatar Apr 29 '24 19:04 vorporeal

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.

nicoburns avatar May 01 '24 02:05 nicoburns

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. :sweat_smile:

vorporeal avatar May 01 '24 20:05 vorporeal