Multiline text layout panics when multi-byte UTF-8 is used #571
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jayvdb
reviewed
Apr 15, 2024
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // iterate over indices | ||
| while let Some(idx) = indices.next() { |
Check warning
Code scanning / clippy
this loop could be written as a for loop Warning
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The layout function for multi-line text crashes when multi-byte UTF-8 characters are used. This pull request fixes this issue.
The old algorithm was simply operating on bytes and directly indexing the string slices on byte boundaries, which panics for UTF-8 multi-byte characters (one of my use-cases).
The function has been rewritten to use an iterator over the string slice via
str::char_indices, which is on base of acharand not of a byte and therefore avoids the panic. A small test case was added in the code. The test depends on the ttf feature though, as it must use a font to determine the width of a text in pixels. So, the test is only compiled and run when also the ttf feature is enabled.NOTE: the CONTRIBUTING file mentions that plotters should be compilable with Rust 1.36. Unfortunately, I didn't manage to do that. After setting the toolchain, it first complained about the ab_glyph feature having the same name as a dependency. Fixing this, the compilation ran into a problem as the newer version of chrono (which are automatically downloaded on the build with 1.36) are for edition 2021, which is then of course unrecognized as 1.36 is for edition 2018. After trying around a bit, I could not get this to work with 1.36, so I stopped. I assume, that plotters itself is normally compiled with newer versions than 1.36 as the ab_glyph feature and chrono would not allow that. I hope this is ok.