nushell / reedline

A feature-rich line editor - powering Nushell
https://docs.rs/reedline/
MIT License
550 stars 155 forks source link

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

Closed bew closed 7 months ago

bew commented 8 months ago

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..) https://github.com/nushell/reedline/blob/0698712701418a7212ebf75d5724d72eccc4b43f/src/painting/painter.rs#L107-L120

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

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

Demo

https://github.com/nushell/reedline/assets/9730330/ab52e593-cc1d-4762-8df8-9e5774538531

MISSING (DONE)


What do you think of this implementation?

bew commented 8 months ago

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 commented 7 months ago

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

fdncred commented 7 months ago

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 commented 7 months ago

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

fdncred commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.