Fix panic on 0 size terminal on windows

[maciek50322]

Found bug on windows working with ratatui, with everything default, no drawing, just waiting for event.

When BOTH of terminal width & height are changed to 0 (like on image)

there seems to be record.size.y == -1, because record.size.y as u16 + 1 panics with

thread 'main' panicked at C:\...\crossterm-0.28.1\src\event\source\windows.rs:70:33:
attempt to add with overflow

-1i16 as u16 == u16::MAX
u16::MAX + 1 // panics

used as i32 for -1 to become 0 (represents size of terminal better than 65535) and i16::MAX (32767) to become 32768

[joshka]

A few unit tests for this covering the edge cases (-1, 0, i16:Max), and perhaps moving the impl to a impl From<T> conversion might be reasonable for this. Looking at this code, it would be something that someone could accidentally "optimize" later and remove this fix if there's not code which helps make it easy to understand why this is correct.

Also, TIL that the maximum screen buffer size seems like it's 32768 and not 65536 as I'd assumed on Windows. I haven't specifically tested that (or found any docs to show the limit). I'm curious whether this your understanding too?

General approval that this is the correct impl because we know that the range of values is -1..36767 here, but it could be worth making a note of that as it doesn't seem to be documented anywhere in the windows / crossterm-winapi crate that this is the case. I'm assuming the window size can never be -2 here...

[maciek50322]

Also, TIL that the maximum screen buffer size seems like it's 32768

Didn't find documentation but suggestions that max buffer size is actually 32766 (= i16::MAX - 1)
https://stackoverflow.com/a/8775884
https://superuser.com/a/1331420
So just (record.size.y + 1) as u16 should work, buut mode con lines=0 gives same message as mode con lines=32767 and we know it can repport -1, so maybe it could do 32767 too.

I'm assuming the window size can never be -2 here...

I'm assuming that too since the comment says // windows starts counting at 0, unix at 1, I guess that on windows represents max index and the -1 is "no buffer". If it ever does -2, then code can be aligned again.

Also I found that actually when height is 0 and then width changes (doesn't have to be min) reports height as -1. So maybe it wasn't intended, but it's this rust code that panics.

A few unit tests for this covering the edge cases (-1, 0, i16:Max), and perhaps moving the impl to a impl From conversion might be reasonable for this.

I'm still learning rust and making these changes are troublesome for me, because I don't understand what excatly what you mean / prefer. If you have more time to explain:

1. Unit tests covering these cases would need go through whole try_read and I don't know how to mock console and polling? The impl From<T> should help here to test just this one part.
2. I think the impl From<WindowBufferSizeRecord> for Event can't be done because WindowBufferSizeRecord seems to be private (in different package).
3. That would mean to move whole match to impl From<InputRecord> for Event? But there's an empty match arm, so I think that should be TryFrom.
4. Still the whole match expression has references to self: WindowsEventSource, that I think can't be accessed in impl TryFrom<InputRecord> for Event?

Also feel free to make it yourself. I can infer answers from the changes.

[joshka]

Yep - you're right. Thanks for the explanation. The hassle to do this is much higher than I'd anticipated and exceeds the benefit. Let's skip that for now I think rather than making perfect be the enemy of good here.

The code looks correct enough as-is.