Skip to content

Add smooth scrolling to menus #891

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

maxomatic458
Copy link
Contributor

currently when scrolling up, the selected item will always stick to the bottom (see #733)

This PR fixes that.

Currently implemented for

  • ide menu
  • columnar menu

closes #733

@maxomatic458
Copy link
Contributor Author

Here is a video of the new behavior, see issue above to see the current behavior

demo.mp4

@maxomatic458 maxomatic458 marked this pull request as ready for review March 19, 2025 21:35
@maxomatic458 maxomatic458 marked this pull request as draft March 20, 2025 13:12
@maxomatic458 maxomatic458 marked this pull request as ready for review March 20, 2025 17:45
@ysthakur
Copy link
Member

ysthakur commented Apr 2, 2025

I'm really sorry, I'm having a hard time understanding what exactly this PR does from looking at the video, and when I run it myself (VS Code terminal, Windows Terminal + Zellij, Windows Terminal without Zellij), I don't see any change in behavior from the main branch. Is this supposed to keep the selected item at the top when scrolling?

Edit: Okay, I was being dumb earlier. I thought "scrolling" meant scrolling in the terminal, not hitting the up arrow to move within the menu. Sorry about that, makes sense now.

@maxomatic458
Copy link
Contributor Author

@ysthakur see the video linked in the original issue, basically currently when you scroll up, the selected item will be stuck at the bottom.
This PR fixes that.

@maxomatic458
Copy link
Contributor Author

ok i believe the PR should be ready now.
i reverted the change in prompt_lines_with_wrap as the -1 seems to be intentional and i dont want to break stuff anywhere else.

I tested it with multiline prompts and with prompt wrapping (due to terminal size)

The CI is failing because of the new rust lints added with 1.86, it would probably be better to fix them in another PR (although i can also fix them here if you want to)

@ysthakur
Copy link
Member

ysthakur commented Apr 4, 2025

@sholderbach Could you take a look at this? I don't know Reedline well enough to say if the change to say if the remaining_lines change is fine.


The CI is failing because of the new rust lints added with 1.86, it would probably be better to fix them in another PR (although i can also fix them here if you want to)

Yeah that's fine, no need to do it in this PR.


With the change to Painter::remaining_lines, this is what the description menu looks like when I open up the help menu and scroll all the way to the bottom of the description for cd:
image

This is how it looks currently without your change:
image

So the new behavior actually looks more reasonable to me, since the prompt is visible. We do need to throw in an ANSI reset or something to fix that color, though.


The only other place using remaining_lines() is Painter::repaint_buffer():

} else if required_lines >= remaining_lines {
let extra = required_lines.saturating_sub(remaining_lines);

Here, it hasn't drawn the prompt yet, so it doesn't want you to subtract the prompt height from the remaining lines.

@maxomatic458
Copy link
Contributor Author

maxomatic458 commented Apr 4, 2025

So the new behavior actually looks more reasonable to me, since the prompt is visible. We do need to throw in an ANSI reset or something to fix that color, though.

Ok so the prompt should now always be visible whenever a menu is open (so any content that is not fitting will get cut of at the end).

So the new behavior actually looks more reasonable to me, since the prompt is visible. We do need to throw in an ANSI reset or something to fix that color, though.

For me the colors are still the same (maybe check if that still happens with the latest commit)

Here, it hasn't drawn the prompt yet, so it doesn't want you to subtract the prompt height from the remaining lines.

Fixed that by re-adding the prompt_height

Edit: i believe it is ok to always subtract it from the remaining_lines() because then it does what is said in the doc comment (which didnt happen before)

@ysthakur
Copy link
Member

ysthakur commented Apr 7, 2025

Hmm, I'm still getting those colors on the latest commit, along with the prompt not always being visible:
image

It does seem unlikely to me that someone would be activating a description menu with such a small terminal, but I don't know how people use Reedline (apart from Nushell itself).

Apart from that, I see no problems with this PR. It works well for the columnar and ide menus. I'd just like Stefan to take a look to confirm that this is fine.

@maxomatic458
Copy link
Contributor Author

PR currently breaks menu rendering with multiline prompts.

1: Current behavior -> menu has priority over prompt (prompt will be partially rendered if required)
2: This PR -> prompt has priority over menu

realistically people will probably at most use 2-3 line prompts, so option 2 is probably better for most cases
however with Large prompts (where a prompt is larger than the terminal) menus will not correctly render (or not at all)

but i believe i will try change it so we stick with the current behavior.

@maxomatic458
Copy link
Contributor Author

With the latest commit, everything should be the same as before (apart from the scrolling)

Hmm, I'm still getting those colors on the latest commit, along with the prompt not always being visible:

not sure about the colors, but for me on both latest master, and latest commit on this PR the description menu (f1)
looks like this (small terminal + pressed tab 2 times):

grafik

so the prompt dissapearing seems to be "normal" behavior and is not caused by that (at least not by the latest commit)

I did a good amount of testing and it works with
large multiline prompts (either just multiple lines or due to wrapping).

So this PR should be ready now!

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.

smoother menu scrolling
2 participants