Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd a new safe harfbuzz crate. #105
Conversation
| text.as_ptr() as *const std::os::raw::c_char, | ||
| text.len() as std::os::raw::c_int, | ||
| 0, | ||
| text.len() as std::os::raw::c_int, |
This comment has been minimized.
This comment has been minimized.
khaledhosny
Feb 20, 2018
The item_offset and item_length are crucial for proper use of HarfBuzz (e.g. shaping a sub-run of the text) and should be exposed in the API.
This comment has been minimized.
This comment has been minimized.
waywardmonkeys
Feb 20, 2018
Author
Collaborator
I was thinking of exposing that via a separate function so that someone that just wants to layout a single piece of text can via something like add_substr instead of add_str. Does that make sense? IIRC, the servo code is not currently shaping substrings in a way that needs this.
This comment has been minimized.
This comment has been minimized.
waywardmonkeys
Feb 20, 2018
Author
Collaborator
Also, thanks for the feedback! It is very helpful! I hope that the Rust binding can be easier to use than the C API with solid docs.
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| /// Throw away text stored in the buffer, but maintain the | ||
| /// currently configured options like direction and script. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
waywardmonkeys
Feb 20, 2018
Author
Collaborator
Looking at the harfbuzz code, I see that the difference is that reset resets the unicode functions (which I haven't exposed yet), the buffer flags (which I don't yet understand well), and the replacement character, which isn't needed from Rust in the current API as a &str will be valid UTF-8 and shouldn't need the replacement character.
This probably also means that I should submit a patch upstream to improve the harfbuzz docs on this point.
|
This looks generally good to me. Since Servo is not (yet?) using this crate, would you like to be the primary maintainer of it? We can add you to the "reviewers" list for this repository in our homu config and as an owner on crates.io, and you could feel free to approve any pull requests that only touch the new crate (including your own PRs, if you choose). That might help you iterate faster if you plan to keep evolving this code. It would also be great to have you as an additional reviewer for harfbuzz-sys PRs. |
|
@mbrubeck That sounds good to me! I figured that once this crate was further along (and published), I'd submit a PR to update servo to use it rather than the unsafe code directly using |
waywardmonkeys
force-pushed the
waywardmonkeys:harfbuzz-crate
branch
from
d48c9a8
to
bb2b63a
|
@mbrubeck Do you need anything from me to move forward on this? |
|
This looks like a fine start to me. @bors-servo r+ |
|
|
Add a new safe harfbuzz crate. There is an orphaned crate, https://crates.io/crates/harfbuzz. This was previously published by the Servo project, but now the Servo project really only maintains `harfbuzz-sys` which doesn't present a safe API. This is proposing adding back a `harfbuzz` crate that could be used by Servo and other projects, but would be safe and have somewhat better ergonomics. This also would address my question from issue #90 about using `enum` and bit flags. Thoughts? <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/105) <!-- Reviewable:end -->
|
|
|
|
Make waywardmonkeys a reviewer for harfbuzz. Per servo/rust-harfbuzz#105 (comment). <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/809) <!-- Reviewable:end -->
waywardmonkeys commentedFeb 20, 2018
•
edited by larsbergstrom
There is an orphaned crate, https://crates.io/crates/harfbuzz. This was previously published by the Servo project, but now the Servo project really only maintains
harfbuzz-syswhich doesn't present a safe API.This is proposing adding back a
harfbuzzcrate that could be used by Servo and other projects, but would be safe and have somewhat better ergonomics.This also would address my question from issue #90 about using
enumand bit flags.Thoughts?
This change is