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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (properly) the logic around prompt re-use & Host Command handling #770

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

bew
Copy link
Contributor

@bew bew commented Mar 12, 2024

First PR for nushell/reedline \o/

Revert/Rewrites #758

Fixes #755 (correctly this time 馃槵)

Where did the newline actually come from in that issue?

The phantom newline that OP reported was due to this block in painter initialization:
(馃憠 It would detect some text before the cursor and always make a new prompt on the next line..)

// Assumption: if the cursor is not on the zeroth column,
// there is content we want to leave intact, thus advance to the next row
let new_row = if column > 0 { row + 1 } else { row };
// If we are on the last line and would move beyond the last line due to
// the condition above, we need to make room for the prompt.
// Otherwise printing the prompt would scroll of the stored prompt
// origin, causing issues after repaints.
let new_row = if new_row == self.screen_height() {
self.print_crlf()?;
new_row.saturating_sub(1)
} else {
new_row
};
self.prompt_start_row = new_row;

Fixes #771 (my issue)
Now I have the same experience than on my zsh config 馃槂

Supports everything I mentioned in #771
Supports replacing the prompt (even from multi-line old prompt)

Demo

nushell.fixed.prompt.mov

MISSING (DONE)

  • Unit tests for the painter initialization logic
  • Refactor painter initialization logic (to remove that unwrap 馃憖)

What do you think of this implementation?

@bew
Copy link
Contributor Author

bew commented Mar 12, 2024

This example isn't perfect DX/UX, there's a bad case with a multi-line prompt + cursor on one of the first lines + executing command echo; foo-that-outputs..
Like with a 3 lines prompt, cursor on first line, that command would go down 1 line but would overwrite the 3rd line.

A solution would be to use the command commandline set-cursor --end; echo; foo-that-outputs.
But I'm thinking we could remove the echo and expose a new flag like commandline set-cursor --below-prompt to place the cursor below all the lines of the prompt (and it's nice & easy to read 馃檭)

@bew
Copy link
Contributor Author

bew commented Apr 14, 2024

Hello!
I finally went back to finish this PR, it should be ready for review now 馃檹

@fdncred
Copy link
Collaborator

fdncred commented Apr 14, 2024

I've played a little bit with this without nushell and it appears to be outputting less ansi escape codes, which is good.

Separately, on my own and unrelated to this PR, I'm trying to figure out how/why reedline prints so many CRLFs in nushell and also how/why the prompt is redrawn with every character typed.

@bew
Copy link
Contributor Author

bew commented Apr 22, 2024

Hello o/
Is there anything else I can do to get this PR merged? 馃

@fdncred
Copy link
Collaborator

fdncred commented Apr 23, 2024

there's a bad case with a multi-line prompt + cursor on one of the first lines + executing command echo; foo-that-outputs..

@bew does this 'bad case' still exist?

@bew
Copy link
Contributor Author

bew commented Apr 23, 2024

there's a bad case with a multi-line prompt + cursor on one of the first lines + executing command echo; foo-that-outputs..

@bew does this 'bad case' still exist?

The 'bad case' I mentioned is only a limitation with the example, due to using 'echo' in the keybind.
There is nothing I can do in this PR afaik.

To property solve this limitation with using echo to move cursor below the prompt I suggested the creation of a commandline set-cursor option that would do that, but I think this should be done in a separate PR.

@fdncred
Copy link
Collaborator

fdncred commented Apr 23, 2024

ok, thanks. let's try it because we're a week away from a release. if we don't try it now, it'll have to wait until after the release.

@fdncred fdncred merged commit 455b9a3 into nushell:main Apr 23, 2024
6 checks passed
@bew bew deleted the fix/prompt-suspend-and-reuse branch April 23, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants