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.

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

Rust panics when terminal is too small #1403

Closed
k2d222 opened this issue Dec 30, 2021 · 6 comments · Fixed by #1408
Closed

Rust panics when terminal is too small #1403

k2d222 opened this issue Dec 30, 2021 · 6 comments · Fixed by #1408
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug

Comments

@k2d222
Copy link
Contributor

k2d222 commented Dec 30, 2021

When the terminal gets too small to fit some widget, helix crashes.

Reproduction steps

  • Open helix in a resizable terminal.
  • Open the file picker (<space> f)
  • Resize the window to about 4 rows or 4 cols
  • => crash

Stumbled upon this bug when using hx in a drop-down terminal, which apparently resizes the window when hidden.

Environment

  • Platform: Linux (tested on gnome-terminal)
  • Helix version: current master branch
@k2d222 k2d222 added the C-bug Category: This is a bug label Dec 30, 2021
@kirawi
Copy link
Member

kirawi commented Dec 30, 2021

😅
\cc @archseer Maybe we should have a test to check for crashes with any of the UI elements on small terminal sizes?

let rows = inner.height;
let offset = self.cursor / (rows as usize) * (rows as usize);

Should be as simple as std::cmp::max(1, (row as usize) * (rows as usize))

@archseer
Copy link
Member

I think we need to just ignore out of bound render offsets.

(I personally can't even shrink my window that small so I can't reproduce. Can you at least include a backtrace?)
pos

@kirawi
Copy link
Member

kirawi commented Dec 30, 2021

Found another panic.

thread 'main' panicked at 'index out of bounds: the len is 115 but the index is 161', C:\Users\there\Desktop\helix\
helix-tui\src\buffer.rs:298:17
stack backtrace:
   0: std::panicking::begin_panic_handler at /rustc/8f117a77d0880ed59afcc1a19c72ec5c1e44b97c\/library\std\src\panicking.rs:498

Edit: Nevermind, looks like a propagated panic from the same source issue,

@archseer
Copy link
Member

Should be as simple as std::cmp::max(1, (row as usize) * (rows as usize))

Yeah that looks right.

@kirawi
Copy link
Member

kirawi commented Dec 30, 2021

(The original panic was a divide by 0 panic on line 508)

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Dec 30, 2021
@k2d222
Copy link
Contributor Author

k2d222 commented Dec 30, 2021

The div by 0 is not the only issue: The get and get_mut methods for Buffer panic when out of bounds.

I think it would be safer and more idiomatic to return Option and implement Index and IndexMut to panic. It would mirror the way Vec works. So whenever you're sure there is no overflow, use subscript operator [] (which has a well-known panic semantic) otherwise perform bound checks with the Option.

pub fn get(&self, x: u16, y: u16) -> &Cell {

Another solution would be to always perform the bounds checks and to return a dummy Cell when out of bounds. I dislike this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants