Skip to content

fix: adjust cursor start position for large buffers #900

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

Merged
merged 2 commits into from
Jun 25, 2025
Merged

Conversation

qilme
Copy link
Contributor

@qilme qilme commented Apr 1, 2025

@fdncred
Copy link
Contributor

fdncred commented Apr 1, 2025

Thanks for trying to make nushell and reedline better. Please help me understand how this fixes the issue. If my screen height is 50 how is queuing 50 \n's going to help? Is there a way we can easily try this in the reedline demo to see if it works?

@qilme
Copy link
Contributor Author

qilme commented Apr 1, 2025

Since reedline uses crossterm and crossterm uses ansi escape sequences to implement the function, I can use nushell to emulate the behavior.

  1. scope commands or other command judged as large buffer.If it is a lagre buffer, then prompt_start_row is 0 and nushell send "ESC [0;0H" to terminal.

  2. ansi -e "H" is the same as "ESC [0;0H". It clear my viewpoint in the Windows Terminal, vscode, alacritty and termux.

  3. Scrool up and content as high as my screen height disappears.

  4. scope commands

  5. for x in 1..50 { if $x > 30 { break }; print ""}

  6. ansi -e "H"

  7. Scrool up and content exists

@fdncred
Copy link
Contributor

fdncred commented Apr 1, 2025

But don't you have a bunch of empty lines after the buffer?

@qilme
Copy link
Contributor Author

qilme commented Apr 1, 2025

The demo does have, but the nushell I compiled after patching not.

@qilme
Copy link
Contributor Author

qilme commented Apr 1, 2025

Origin:
origin.gif
My demo:
demo.gif
My patch:
patch.gif

@qilme qilme force-pushed the main branch 2 times, most recently from 7e611f1 to af8a02e Compare April 17, 2025 02:44
@Bahex
Copy link
Member

Bahex commented Jun 25, 2025

I was experiencing this issue but I wasn't sure if it was caused by my terminal emulator or nushell/reedline. Thank you for working on this!

@Bahex Bahex merged commit e4221b9 into nushell:main Jun 25, 2025
6 checks passed
@Bahex
Copy link
Member

Bahex commented Jun 25, 2025

Thanks!

Comment on lines +246 to +248
for _ in 0..screen_height - lines.required_lines(screen_width, None) {
self.stdout.queue(Print(&coerce_crlf("\n")))?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@qilme @Bahex Sorry to step in, I've noticed a panic caused by this change.

Basically we need saturating_sub to avoid negative numbers.
Besides, I'm not sure whether we should take lines.hint into account here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've pushed what I consider to be a proper fix to #898 (since there might be a value useful for both issues), Please review that when you get time.

Copy link
Member

Choose a reason for hiding this comment

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

@blindFS sorry for the slow reply, your PR was already merged by the time i got the time to take a look so i just forgot about it.

Besides, I'm not sure whether we should take lines.hint into account here.

getting a multiline hint can similarly push text up, not taking that into account can cause lines to disappear as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

getting a multiline hint can similarly push text up, not taking that into account can cause lines to disappear as well.

Say we have a large_buffer repaint caused by a long multiline hint, if we add the height of self.hint to lines.required_lines, we'll end up not doing any scrolling. Is that expected behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants