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

Attempt to fix multiline prompt resize issue #841

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

blindFS
Copy link
Contributor

@blindFS blindFS commented Oct 6, 2024

Tried to fix #809 in this PR.

The problem lies in the handle_resize function

pub(crate) fn handle_resize(&mut self, width: u16, height: u16) {
self.terminal_size = (width, height);
// `cursor::position() is blocking and can timeout.
// The question is whether we can afford it. If not, perhaps we should use it in some scenarios but not others
// The problem is trying to calculate this internally doesn't seem to be reliable because terminals might
// have additional text in their buffer that messes with the offset on scroll.
// It seems like it _should_ be ok because it only happens on resize.
// Known bug: on iterm2 and kitty, clearing the screen via CMD-K doesn't reset
// the position. Might need to special-case this.
//
// I assume this is a bug with the position() call but haven't figured that
// out yet.
if let Ok(position) = cursor::position() {
self.prompt_start_row = position.1;
}
}
where the new position is set according to cursor, while the cursor's row is larger than the prompt start row in multiline cases.

@fdncred
Copy link
Collaborator

fdncred commented Oct 6, 2024

Thanks for taking this on. We've had prompt issues for a long time and a while back (incorrectly) disabled some of it. What I've learned since then is some terminals, when resized, leave it to the shell to redraw things. Ghostty is one of those shells. May be related or unrelated, just thought I'd share.

I'm trying to figure how where/how to test this. Do we have an example with a multi-line prompt?

@blindFS
Copy link
Contributor Author

blindFS commented Oct 6, 2024

Thanks for taking this on. We've had prompt issues for a long time and a while back (incorrectly) disabled some of it. What I've learned since then is some terminals, when resized, leave it to the shell to redraw things. Ghostty is one of those shells. May be related or unrelated, just thought I'd share.

I'm trying to figure how where/how to test this. Do we have an example with a multi-line prompt?

No test case found for the resize handling.
Hope these recordings are convincing.

nu 0.98

Screen.Recording.2024-10-06.at.22.14.54.mov

nu 0.98 + fixed reedline

Screen.Recording.2024-10-06.at.22.18.02.mov

@blindFS
Copy link
Contributor Author

blindFS commented Oct 6, 2024

BTW, I tested it on both wezterm and alacrity. This PR probably won't fix all multiline prompt drawing issues at once, but the code I quoted above is obviously buggy, prompt_start_row will just increase over each resize op. So I think we'd better start from fixing this function.

@fdncred
Copy link
Collaborator

fdncred commented Oct 6, 2024

When I tested earlier, it seems ok on the left prompt, but the right prompt was still wrapping but not unwrapping. I'm fine with leaving that to another PR.

@blindFS
Copy link
Contributor Author

blindFS commented Oct 7, 2024

When I tested earlier, it seems ok on the left prompt, but the right prompt was still wrapping but not unwrapping. I'm fine with leaving that to another PR.

You are right, the right prompt wrapping is not tackled at all.
In my setup, this single line fixes the wrapping issue. Do I need to add it to this PR?

diff --git a/src/engine.rs b/src/engine.rs
index c5ae7d2..ee0d930 100644
--- a/src/engine.rs
+++ b/src/engine.rs
@@ -1197,6 +1197,7 @@ impl Reedline {
             ReedlineEvent::OpenEditor => self.open_editor().map(|_| EventStatus::Handled),
             ReedlineEvent::Resize(width, height) => {
                 self.painter.handle_resize(width, height);
+                self.repaint(prompt)?;
                 Ok(EventStatus::Inapplicable)
             }
             ReedlineEvent::Repaint => {

To me personally, the left prompt duplication is much more annoying because tmux new pane will trigger it.

@fdncred
Copy link
Collaborator

fdncred commented Oct 9, 2024

Maybe in another PR because I'm guessing there will be edge cases.

@fdncred fdncred merged commit 5e556bf into nushell:main Oct 9, 2024
6 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Oct 9, 2024

Let's run with this to see how it goes this week.

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.

Nushell: Multi-line Prompt Bug When Resizing Window
2 participants