[para-api] Create ParagraphBidiInfo structs as API for processing a single paragraph at a time #115
Conversation
| /// (This should be kept in sync with BidiInfo::reorder_line.) | ||
| #[cfg_attr(feature = "flame_it", flamer::flame)] | ||
| pub fn reorder_line(&self, line: Range<usize>) -> Cow<'text, [u16]> { | ||
| if !level::has_rtl(&self.levels[line.clone()]) { |
There was a problem hiding this comment.
issue: I do not want to duplicate this code
| /// (This should be kept in sync with BidiInfo::reorder_line.) | ||
| #[cfg_attr(feature = "flame_it", flamer::flame)] | ||
| pub fn reorder_line(&self, line: Range<usize>) -> Cow<'text, str> { | ||
| if !level::has_rtl(&self.levels[line.clone()]) { |
There was a problem hiding this comment.
issue: I do not want to duplicate this code
The new ParagraphInfo types should not have methods that are more than a single method call, maybe two.
src/lib.rs
Outdated
| @@ -868,6 +840,24 @@ impl<'text> ParagraphBidiInfo<'text> { | |||
| } | |||
| } | |||
|
|
|||
| // Implementation of reorder_line for both BidiInfo and ParagraphBidiInfo. | |||
There was a problem hiding this comment.
nit: more docs for what the parameters do, on both functions
src/lib.rs
Outdated
| }); | ||
| pure_ltr.as_mut().unwrap().push(is_pure_ltr); | ||
| } | ||
| assert_eq!( |
There was a problem hiding this comment.
OK, will do (although this was essentially just moving the already-existing assertion inside the if split_paragraphs condition).
src/lib.rs
Outdated
| /// This is used for both BidiInfo and ParagraphBidiInfo, with split_paragraphs=true | ||
| /// indicating that it should split paragraphs and return the two Option<> results | ||
| /// (for BidiInfo), and split_paragraphs=false indicating it should not split paras, | ||
| /// and returns the single paragraph_level and is_pure_ltr values instead. |
There was a problem hiding this comment.
issue: I don't want the callers to have to unwrap (we want to avoid unwraps as much as possible in unicode_bidi)
can split_paragraphs be implemented as an Option<&mut Vec> outparam instead?
(might need to use an option of a tuple to support pure_ltr as well)
There was a problem hiding this comment.
I'll have a look and see what I can come up with...
src/lib.rs
Outdated
|
|
||
| if split_paragraphs { | ||
| if para_start < text.len() { | ||
| paragraphs.as_mut().unwrap().push(ParagraphInfo { |
jfkthame
force-pushed
the
paragraph-api
branch
from
9d067e9
to
5d785ce
Compare
| /// (This should be kept in sync with BidiInfo::reordered_levels.) | ||
| #[cfg_attr(feature = "flame_it", flamer::flame)] | ||
| pub fn reordered_levels(&self, line: Range<usize>) -> Vec<Level> { | ||
| assert!(line.start <= self.levels.len()); |
There was a problem hiding this comment.
I guess I was a bit unclear what to do with cases like this (compare the existing asserts at https://docs.rs/unicode-bidi/0.3.13/src/unicode_bidi/lib.rs.html#546-547). But I'm happy to make them debug_assert! if you prefer.
There was a problem hiding this comment.
oh, sorry, you're totally right, those are external asserts. My bad!
There was a problem hiding this comment.
yeah just remove the last commit and we're ready to land
There was a problem hiding this comment.
I'll add some docs later that explain that this API panics
jfkthame
force-pushed
the
paragraph-api
branch
from
6e37c03
to
41057ca
Compare
|
Thanks! |
|
And thank you! :) |
This gives us ParagraphBidiInfo and utf16::ParagraphBidiInfo structs, which work like BidiInfo but are explicitly single-paragraph APIs.
(If there's a paragraph separator present in the text, they'll simply treat it like a segment separator. An alternative would be to return some kind of error if this happens.... thoughts?)
I extended test_process_text() to also run its single-paragraph testcases through the new APIs, and also added a couple of tests of what happens when text with a paragraph separator is (erroneously) passed here, to confirm that we handle it as expected.