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

panic when scrolling #731

Closed
4 tasks done
jeevithakannan2 opened this issue Oct 2, 2024 · 15 comments · Fixed by #743
Closed
4 tasks done

panic when scrolling #731

jeevithakannan2 opened this issue Oct 2, 2024 · 15 comments · Fixed by #743
Labels
bug Something isn't working rust Pull requests that update Rust code

Comments

@jeevithakannan2
Copy link
Contributor

jeevithakannan2 commented Oct 2, 2024

Describe the bug

To Reproduce

Steps to reproduce the behavior:

  1. Run any command that outputs a very long message.
  2. Press pageup 3 to 4 times.
  3. See error.

Expected behavior

  • No panic and scroll up

Screenshots

image

Additional context

Workarounds

  • Limit the scroll offset up to the maximum number of rows of terminal
    let mut parser = vt100::Parser::new(size.height, size.width, 200);
  • Remove the scroll functionality for now.

Solution

  • Fork the vt100 and make our own crate just like the how other big projects are doing ( Big changes needed in linutil )
  • Switching to a different parser ( Not ideal ).

Checklist

  • I checked for duplicate issues.
  • I checked already existing discussions.
  • This issue is not included in the roadmap.
  • This issue is present on both stable and development branches.
@jeevithakannan2 jeevithakannan2 added the bug Something isn't working label Oct 2, 2024
@github-actions github-actions bot added the rust Pull requests that update Rust code label Oct 2, 2024
@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

@jeevithakannan2 its not a linutil issue i would just leave it how it is until it gets fixed in vt100

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

actually, it looks like vt100 hasnt been updated in a year, maybe we should fix this 😆 i wouldnt be saying this if it was being actively updated though

@jeevithakannan2
Copy link
Contributor Author

Yeah it is not active that's why I created a issue here to get feedback. I could fork the vt100 and apply the fixes. Making our own version of vt100

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

Yeah it is not active that's why I created a issue here to get feedback. I could fork the vt100 and apply the fixes. Making our own version of vt100

we should let Chris decide

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

we should add keybindings for arrow keys as well

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

once this gets fixed

@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Oct 2, 2024

Adding key bindings for arrowkeys is bad as we use up and down keys in scripts

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

Adding key bindings for arrowkeys is bad as we use up and down keys in scripts

j and k dont work when running scripts neither do arrow keys, + arrow keys is a good addition to avoid conflicts with j and k

@adamperkowski
Copy link
Collaborator

adamperkowski commented Oct 2, 2024

@ChrisTitusTech i summon you

@ChrisTitusTech
Copy link
Owner

Well shit. I guess for now we can remove scroll and if have the means to fix or fork we can do that in the future.

@adamperkowski
Copy link
Collaborator

if we were to remove scroll, i'd not remove code but turn scroll into an optional feature

@adamperkowski
Copy link
Collaborator

i'll research this issue in some time. maybe there's a workaround

@cartercanedy
Copy link
Contributor

mproc-vt100 looks like a recently updated fork, idk how much they've refactored the api

@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Oct 2, 2024

Will keep this issue open until the scroll feature is fixed. Until then we should remove it. Just 6 lines of code is not a big deal. The biggest issue here is if a command is running and a panic happens the child is not killed leading to potential issues.

@jeevithakannan2
Copy link
Contributor Author

mproc-vt100 looks like a recently updated fork, idk how much they've refactored the api

I looked at a bunch of forks the tui-term does not run with forks. Need to sort this out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants