search

jmacdonald / amp

A complete text editor for your terminal.
https://amp.rs
Other
3.68k stars 105 forks source link

Fix a few crashes on terminal resizing. #202

Closed christoph-heiss closed 3 years ago

christoph-heiss commented 3 years ago

This resolves #93. Underlying issue were some overflowing subtractions, which would cause panics in debug mode and OOM-situation in release mode, due to trying to allocate a huge amount of memory.

I hope I found all possible spots where the assumption that the terminal width/height is at least 1 is made.

jmacdonald commented 3 years ago

Thanks for catching this, @christoph-heiss!

After reviewing this, I'm thinking it might make more sense for us to always guarantee a positive value from the view::Terminal type's width/height methods, using something like width.max(10). Can you give that a shot to see if it fixes the bug? If so, that'll also guard against any other consumers of that API that might do arithmetic on the terminal dimensions.

If it does work, I think we should extract those to MIN_WIDTH and MIN_HEIGHT constants and just accept that we always expect the terminal to be at least that large. There's no valid use case for a terminal window smaller than 10x10 (or arguably even 50x50), so as long as it doesn't break the terminal emulator by writing off screen, it feels like the way to go. :slightly_smiling_face:

christoph-heiss commented 3 years ago

That makes sense, will do that. I experimented with this at first and it seems that MIN_WIDTH/MIN_HEIGHT needs to be at least 2x2 to prevent any overflowing. But I guess we can also make that 10x10 since that is already unusable small.

christoph-heiss commented 3 years ago

So, I just implemented the width/height clamping as suggested, also defining constants for that. I still had to fix up a few instances where the subtraction is based on the content size, which theoretically can be unbound.

While quickly skimming test_terminal.rs I noticed then although WIDTH/HEIGHT constants are defined, there are not used anymore. I figured I could include this small change (9b72efa) here as well as it is semi-related and only a semantic change.

jmacdonald commented 3 years ago

Thanks for this, and the follow-up changes, @christoph-heiss! :beers: