fix: avoid integer overflow when calculating text alignment padding#188
fix: avoid integer overflow when calculating text alignment padding#188robn wants to merge 1 commit into
Conversation
If the available width is less than the text width, the calculated padding goes negative, causing a panic. Instead, check and clamp the subtraction so it never goes below zero. Signed-off-by: Rob Norris <robn@despairlabs.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #188 +/- ##
=======================================
Coverage 87.71% 87.71%
=======================================
Files 35 35
Lines 5526 5526
Branches 5526 5526
=======================================
Hits 4847 4847
Misses 570 570
Partials 109 109
🚀 New features to boost your workflow:
|
|
Thanks for the PR! I'm a little stumped as to how this panic is possible. Your fix is reasonable, but I'm worried this is a sign of some other subtle issue in the library. Is there any chance you could provide a full reproduction of the issue? If your code is in a public repo I'd love to take a look and try to figure out exactly what's happening. |
|
Alright, try this: Panic should be in There's three commits on there beyond current My gut feeling (without much to back it up) is that its a side-effect of the View wrapping the Sparkline component (immediately to the left of the Meter that is being overrun). That View is Also note that if you change the (I mean, kinda "duh", and I'll leave you to it. I'm just curious and spitballing a bit!) Lemme know if I can help with more! |
|
Incidentally, Sparkline is the place where a right "anchor" would have been nice. It had an earlier version that was pretty much just a |
|
Thank you so much for providing that code. Using it as a starting point, I managed to boil this down to a very simple minimal reproduction: element! {
View(
flex_direction: FlexDirection::Column,
width: 9,
) {
Text(
content: "123123123123",
align: TextAlign::Center,
wrap: TextWrap::NoWrap
)
}
}
.print();Your fix here would have prevented the panic, but the alignment would have still been incorrect. In cases like this, the text should be shifted over to the left (effectively negative alignment padding). I reworked the alignment logic in Thanks again for the help! |
|
Ahh, very nice! I'm glad it was such a simple repro. Looking forward to trying it soon! Cheers! |
2 participants
When using
TextAlign::RightorTextAlign::Center, if the available width is less than the text width, the padding calculation inText::alignment_padding()goes negative, causing a panic.This just clamps the subtract amount to the available width, so the result can't go below zero.
Testing and reproduction
I don't actually know enough to know how to write a reduced test case. In my own program, it happens deep inside a set of flex views. Something like:
As best I can tell, in the initial layout the
Viewwrapping theTextis given the minimum width of 9, which is fine becauseprops.contentis a string shorter than that. At some point, there is an update andprops.contentis longer than that. The layout however is already set, so the overflow.With this patch applied, the padding becomes 0, and the text is drawn, overflowing its bounds, but not crashing.
Panic & backtrace
Alternative solutions
Its possible that this is papering over problem in my program. Should I have done something to force a re-layout? Though I doubt a crash is the right thing there regardless!
There's also a world where a negative padding would be nice; consider:
I don't know if that would go against the CSS idea of text alignment. It definitely would be useful for something I'm doing elsewhere though, effectively "anchoring" a very long text line to the right instead of the left. As it is, I had to write a component to get the layout width and take a substring of the content string.