Conversation
nicoburns
left a comment
nicoburns
left a comment
There was a problem hiding this comment.
I wonder whether it would make more sense for this to use a separate struct rather than BreakLines. And it generally seems like it ought to be possible to simplify the code for this mode. But this seems basically fine.
| } | ||
|
|
||
| #[inline] | ||
| fn resolve_indent(&mut self) -> f32 { |
There was a problem hiding this comment.
Does this need to be &mut?
There was a problem hiding this comment.
I'm surprised the linter didn't pick this one up
| env.check_layout_snapshot(&layout); | ||
| } | ||
|
|
||
| /// This test verifies that breaking in the middle of a ligature produces valid glyphs. |
There was a problem hiding this comment.
This test verifies that breaking in the middle of a ligature produces valid glyphs.
It sounds like it actually verifies that breaking in the middle of a ligature doesn't produce valid glyphs? In the other line breaker we just break early in leiu of reshaping. I guess you have reasons for not wanting to do that?
There was a problem hiding this comment.
Yup! My bad on this phrasing.
In the other line breaker we just break early in leiu of reshaping.
👀 ! Maybe not for long! There is work being planned to make this more correct - to reshape after breaking if it is required (for Arabic / ligatures / and so on)!
I guess you have reasons for not wanting to do that?
We actually don't like this behaviour - we want to reshape after breaking, but that isn't currently supported
| use crate::util::TestEnv; | ||
| use parley::{Alignment, AlignmentOptions, InlineBox, PositionedLayoutItem, StyleProperty}; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
I'd love to see a test that includes some emoji. They seems like a case that might easily break if not handled correctly.
| if self.done { | ||
| return None; | ||
| } | ||
| self.prev_state = Some(self.state.clone()); |
There was a problem hiding this comment.
Is this necessary? I would expect this line breaker to never need to revert?
| let item = &self.layout.data.items[self.state.item_idx]; | ||
|
|
||
| match item.kind { | ||
| LayoutItemKind::InlineBox => { |
There was a problem hiding this comment.
Does supporting inline boxes in this mode make sense? (are you using them?) I guess if you carefully control the size of the boxes it would work decently well..
There was a problem hiding this comment.
I'm not using them. Happy to change this behaviour to something else?
There was a problem hiding this comment.
I'd be tempted to ignore them entirely in this mode. But I guess it's also harmless.
|
|
||
| // Compute the x position. | ||
| // Newlines don't contribute to line width (matching break_next behavior). | ||
| let next_x = if is_newline { |
There was a problem hiding this comment.
Would it make sense to strip out newlines before passing the string into Parley? That's what Blitz is doing. I guess there's a general conversation to be had about whitespace collapsing.
There was a problem hiding this comment.
Yes, I'm not sure TBH! We might need to talk about it
Please see Nico's great review here #575 (review) 🙏
3 participants
We have been using this
break_line_with_nextbreaking algorithm in production for a couple months now. The use case is that sometimes the consumer knows how many characters they want to break each line at (rather than specifying a width by some pixel dimension). (It's also significantly faster than the current implementation for obvious reasons - no need to calculate and bookkeep intermediary line length state).Happy to change the name to something like
break_line_with_max_charsor similar.See https://xi.zulipchat.com/#narrow/channel/205635-parley/topic/Floated.20boxes/near/564353468