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

Replace core-foundation and core-graphics #35

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Jul 20, 2022

Message sending now requires types to implement Encode, which these crates don't do (yet, though maybe they will, see servo/core-foundation-rs#628). Since these crates are used quite infrequently, and we'll probably want to use icrate later on anyhow, this PR replaces them.

@ryanmcgrath
Copy link
Owner

This one makes sense to me, though I'm wondering if it doesn't make sense to batch all 3 of these PRs into one big PR and just merge that... I could be wrong but I think there's some things repeating across them (e.g *const Class to static changes). No idea if that'll make merging hell or not - though I'll defer to you given I don't want to ask for too much free work here. ;P

@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 22, 2022

Don't worry about things being difficult to merge or not, I'll figure that out once #30 is done (which is probably also why things are confusing rn; because that PR is "further ahead" than these).

@ryanmcgrath
Copy link
Owner

ryanmcgrath commented Aug 22, 2022 via email

@madsmtm madsmtm force-pushed the objc2-foundation branch 3 times, most recently from c93655f to 8285897 Compare August 29, 2022 03:17
@madsmtm madsmtm changed the title Use objc2-foundation to replace core-foundation and core-graphics Use icrate to replace core-foundation and core-graphics Sep 11, 2023
@madsmtm madsmtm changed the title Use icrate to replace core-foundation and core-graphics Replace core-foundation and core-graphics Sep 11, 2023
@madsmtm madsmtm marked this pull request as ready for review September 11, 2023 23:47
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 11, 2023

I think this one was what I wanted to do after #30.

Note the change in API for Image::draw, which is unfortunate. It's not a strictly a needed change, but I wanted to avoid the direct dependency on core-graphics since it locks the public API to the specific version cacao depends on, and is used so little.

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 12, 2023

A few more notes:

  • NSRect and CGRect (and the others) are exactly equivalent, it's just that the version of objc2 that we're using here doesn't have the aliases defined for it.
  • NSSize currently restricts its input to be valid floating points; this turned out to be a bad idea, and I've removed that in a future version of objc2

@ryanmcgrath
Copy link
Owner

Note the change in API for Image::draw, which is unfortunate. It's not a strictly a needed change, but I wanted to avoid the direct dependency on core-graphics since it locks the public API to the specific version cacao depends on, and is used so little.

Hmm, you're specifically referring to not needing CGContextRef? I see what you mean, though it is definitely unfortunate... but then again, if there were to be other spots that use CGContextRef, it might make sense to bite the bullet and at the very least feature-flag it.

How often does core-graphics change anyway? It might not be a big deal to have it as a dependency.

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.

2 participants