-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
indicatif crashes with assertion failed: self.orphan_lines_count <= self.lines.len() if number of terminal columns is less than 135 #612
Comments
Sorry for the regression! Can you try if it works better with the changes from #608? |
@djc nope still crashes unfortunately
|
@bes Can you share your code, or provide instructions to reproduce? |
@smoelius here is a proof-of-concept https://github.com/bes/indicatif-assert-panic-poc |
I can reproduce the crash. Can you give me a sense for what you expect that program's output to look like (i.e., if everything worked correctly)? |
Oh, I can just run it on a larger width to get that. 🤦 |
The code in state.rs and multi.rs seem to have different ideas about what The code in state.rs seems to regard it a number of Line 155 in 8941d5e
Whereas the code in multi.rs seems to regard it as a visual line count (formerly Lines 315 to 323 in 8941d5e
When I wrote the fix for #582, I had the former in mind. But I see now (and understand why) @oli-obk had the latter in mind. I think the code could be made to work either way. One approach just has to be chosen over the other. Could one of the maintainers render a decision? Then I can try to prepare a fix. |
@smoelius thanks for digging in! I don't have a strong preference so open to arguments that I'm wrong, but I feel like a "line" being a horizontal stripe across the window (that is, counting multiple for wrapped lines) is probably the more intuitive concept? In any case, would be great to use comments in all the relevant places to disambiguate how these numbers should be interpreted. Could even go with newtype wrappers if there are a bunch of different places where we have to pass around context on these, but I feel like that's probably overkill here? |
I like the idea of newtype wrappers. Feels like a good idea to lean on Rust's type safety here, especially since flavors of this issue have been coming back for a while now. |
I agree that adding newtype wrappers sounds like the "right" solution. But it also sounds significant and invasive. Could I suggests merging something like #613 now, and adding newtype wrappers in a separate PR? Note that there is still the question of what @djc suggested the latter. But I ran into trouble with this, and so I went with the former. If |
Hmm, good point.
Yes, merged! |
Hello!
The fix for #582 introduced a crash in my code, and I can't figure out why.
My application crashes with this message:
If my terminal has enough "width" - 135 columns, it works without crashing
If my terminal has below 135 columns, it crashes!
The number of rows seems to be irrelevant, I've tried everything from 9 to 74 lines.
The text was updated successfully, but these errors were encountered: