Skip to content
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.

Sign up for GitHub

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

Fix panic on 0 size terminal on windows #946

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Fix panic on 0 size terminal on windows #946

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6f21cc
fix windows panic on -1 size
maciek50322 Nov 21, 2024
8c487a4
Merge branch 'master' into bugfix/windows-overflow
joshka Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/event/source/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ impl EventSource for WindowsEventSource {
InputRecord::WindowBufferSizeEvent(record) => {
// windows starts counting at 0, unix at 1, add one to replicate unix behaviour.
Some(Event::Resize(
record.size.x as u16 + 1,
record.size.y as u16 + 1,
(record.size.x as i32 + 1) as u16,
(record.size.y as i32 + 1) as u16,
Comment on lines -69 to +70
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

))
}
InputRecord::FocusEvent(record) => {
Expand Down
Loading