-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 rust-freetype with freetype-rs #3369
Comments
Note: NCSU students might be working on this, so please contact me before commencing any work to avoid overlap. |
They submitted #4215, which switches to Piston's freetype-sys but not to the high level bindings. This issue is up for grabs again and would be a great start for an eager contributor from the Rust community! |
I'd like to work on this. What's the best way to test that this works, once I've made the change? |
@schuster Likely just test some sites that use various fonts and check that they look the same before and after. |
Any news on this? |
@boghison You're welcome to start working on it if you'd like. |
@boghison I'm working on it my free time now and then, but I haven't had nearly as much time as I'd like. Feel free to start on it, although I'm still going to work on it as well (as a Rust learning exercise for myself as much as anything else). It should be pretty simple if you already know Rust; I'm just slowed down by learning about lifetimes, lifetime parameters, and other parts of the ownership model as I go through it all. |
Ok, I am gonna give this a try :) |
@jdm What I am worried about is |
@boghison I think it's fine to leave raw API when necessary - you can replace the FT_Library with the Library type (https://github.com/PistonDevelopers/freetype-rs/blob/master/src/library.rs#L34), but it looks like our use of FT_Memory and the UserPtr should stay, since we do special things with them. |
@jdm Wouldn't it be better to reformat the code entirely, because my prediction is that this is just gonna end up a mess because of the mixing of unsafe code (using freetype_rs::ffi types) and the safe code (provided by the library). Is it still OK? |
Well, your other option is to add the bits that are missing to freetype-rs - the code at http://mxr.mozilla.org/servo/source/components/gfx/platform/freetype/font_context.rs#115 is different than the code at https://github.com/PistonDevelopers/freetype-rs/blob/master/src/library.rs#L27, so we would need an API that allows us to provide our own allocation functions and user pointer when creating a Library. That would be a useful addition :) |
@jdm So basically send code upstream? That would be the optimal solution IMHO. Getting rid of unsafe code in servo is the end result, I will start working on freetype-rs then :) |
Thanks! |
@jdm Question about Rust: can any function be marked as extern? |
Makes sense, but it probably shouldn't include the pt_size and set_char_size call. |
@jdm Actually I didn't notice, but there's a new_memory_face function in library.rs, which is almost the same thing. I am trying to understand how servo handles fonts cross-platform and I see that FontHandle is where all the magic happens. It augments the bindings to the platform-dependent libs into a struct that has the same calls on all platforms, is that right? I need to know something: all possible calls are encapsulated in |
Good question! Yes, it appears that FontHandleMethods is the trait that encapsulates all cross-platform font API usage. |
Awesome! I'll let you know if I have any more questions |
@jdm What is stretchiness? |
@jdm I have a problem: freetype-sys v0.1.2 |
You probably need to remove the appropriate entries from http://mxr.mozilla.org/servo/source/components/gfx/Cargo.toml#79 . |
@jdm I've updated those fields when I started. Maybe the problem lies in the fact that servo has dependencies which depend respectively on either freetype-sys (servo) or rust-freetype, but I can't deal with that. |
I don't have any good ideas right now. If you push your changes to a branch (as well as your changes to freetype-rs), perhaps we can play with it. |
Oh wait, Cargo.lock helped me figure out that https://github.com/servo/rust-azure/blob/master/Cargo.toml#L31is also using it. |
As are skia and libfontconfig. |
@jdm That is correct, because Cargo.lock is what I was basing my assumption on. Any thoughts? |
Yeah, all of skia, libfontconfig and rust-azure should switch to using the same freetype-sys that freetype-rs uses: https://github.com/PistonDevelopers/freetype-rs/blob/master/Cargo.toml#L21 |
You can deal with them by cloning the repositories, making changes, and following http://doc.crates.io/guide.html#overriding-dependencies |
@jdm In file included from libazure/mozilla/TypedEnumInternal.h:15:0, define MOZ_WARN_UNUSED_RESULT attribute ((warn_unused_result))^ UPDATE: I fixed this by manually adding |
Switch to PistonDevelopers freetype-sys Required for servo/servo#3369. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/libfontconfig/12) <!-- Reviewable:end -->
Switch to PistonDevelopers freetype-sys Required for servo/servo#3369. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/skia/57) <!-- Reviewable:end -->
Switch to PistonDevelopers freetype-sys Required for servo/servo#3369. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/skia/57) <!-- Reviewable:end -->
Switch to PistonDevelopers freetype-sys Required for servo/servo#3369. Depends on servo/skia#57. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-azure/183) <!-- Reviewable:end -->
@jdm What if we ship all the necessary files directly with rust-azure? |
My code changes are here. Until the freetype-sys issue is solved, it's in limbo. |
What do we need to do here? WR depends on our freetype crate and I want to remove that. |
We need to switch all of the packages that depend on freetype-sys away from it. |
@jdm I'm not sure I parse what you mean. Do you mean we should switch all the packages that depend on servo/rust-freetype away from it? |
Yes, that's what I meant. |
freetype-rs looks so much more pleasant to use. We could get rid of lots of unsafe blocks in the font-related code in gfx, and probably fix some of those lurking crashes that appear every few months.
The text was updated successfully, but these errors were encountered: