Skip to content

fix: adjust cursor start position for large buffers#900

Merged
Bahex merged 2 commits into
nushell:mainfrom
qilme:main
Jun 25, 2025
Merged

fix: adjust cursor start position for large buffers#900
Bahex merged 2 commits into
nushell:mainfrom
qilme:main

Conversation

@qilme

@qilme qilme commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

@fdncred

fdncred commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

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

qilme commented Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

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

fdncred commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

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

@qilme

qilme commented Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

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

@qilme

qilme commented Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

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

Bahex commented Jun 25, 2025

Copy link
Copy Markdown
Member

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!

Comment thread src/painting/painter.rs Outdated
@Bahex Bahex merged commit e4221b9 into nushell:main Jun 25, 2025
6 checks passed
@Bahex

Bahex commented Jun 25, 2025

Copy link
Copy Markdown
Member

Thanks!

Comment thread src/painting/painter.rs
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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