Skip to content

(DON'T MERGE) feat: starship prompt#804

Closed
Southclaws wants to merge 6 commits intonushell:masterfrom
Southclaws:starship-prompt
Closed

(DON'T MERGE) feat: starship prompt#804
Southclaws wants to merge 6 commits intonushell:masterfrom
Southclaws:starship-prompt

Conversation

@Southclaws
Copy link
Copy Markdown
Contributor

Kind of touches on #356 by integrating the Starship prompt directly into the shell.

Not finished yet and has surfaced a potential bug in rustyline anyway. It depends on starship/starship#509 being merged so the Starship prompt can be used as a library.

I could have tackled #356 completely and implemented a full custom prompt feature but I felt this was a simpler approach given that Starship is both written in Rust so shelling out isn't necessary and it already has a bunch of useful features built in.

However, I would understand if it would be preferable to just scrap integrating Starship directly and instead implement a custom prompt system which would facilitate simply shelling out to Starship.

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Oct 8, 2019

Kind of touches on #356 by integrating the Starship prompt directly into the shell.

Not finished yet and has surfaced a potential bug in rustyline anyway. It depends on starship/starship#509 being merged so the Starship prompt can be used as a library.

I could have tackled #356 completely and implemented a full custom prompt feature but I felt this was a simpler approach given that Starship is both written in Rust so shelling out isn't necessary and it already has a bunch of useful features built in.

However, I would understand if it would be preferable to just scrap integrating Starship directly and instead implement a custom prompt system which would facilitate simply shelling out to Starship.
@Southclaws
Copy link
Copy Markdown
Contributor Author

Southclaws commented Oct 8, 2019

Found some possible issues, maybe regressions caused by this PR but not sure.

The issue is the prompt is being cut off by two characters then being replaced with > (a right arrow and a space)

image

Which, if you change the readline_with_initial to just use a string literal, like "prompt" then the issue is clearly not caused by Starship:

readline = rl.readline_with_initial("prompt", (&cmd, ""));

image

Which then lead to noticing another bug, which may be caused by this one, where the caret seems to be one character to the left of the actual typing area - not sure what the actual terminology is but in the following image, I have already typed loc yet the caret is inside the c character, whereas it should be to the right of the c character:

image

These might be related to how the rustyline Editor is configured.

@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Oct 8, 2019

You may also need to change the prompt that is highlighted by rustyline:

https://github.com/nushell/nushell/blob/master/src/shell/helper.rs#L59-L62

@Southclaws
Copy link
Copy Markdown
Contributor Author

Ah, perfect! That clears up the confusion thanks.

This resolves a small integration issue that would make custom prompts problematic (if they are implemented). The approach was to use the highlighter implementation in Helper to insert colour codes to the prompt however it heavily relies on the prompt being in a specific format, ending with a `> ` sequence. However, this should really be the job of the prompt itself not the presentation layer.

For now, I've simply stripped off the additional `> ` characters and passed in just the prompt itself without slicing off the last two characters. I moved the `\x1b[m` control sequence to the prompt creation in `cli.rs` as this feels like the more logical home for controlling what the prompt looks like. I can think of better ways to do this in future but this should be a fine solution for now.

In future it would probably make sense to completely separate prompts (be it, internal or external) from this code so it can be configured as an isolated piece of code.
@Southclaws
Copy link
Copy Markdown
Contributor Author

From the most recent commit:

This resolves a small integration issue that would make custom prompts problematic (if they are implemented). The approach was to use the highlighter implementation in Helper to insert colour codes to the prompt however it heavily relies on the prompt being in a specific format, ending with a > sequence. However, this should really be the job of the prompt itself not the presentation layer.

For now, I've simply stripped off the additional > characters and passed in just the prompt itself without slicing off the last two characters. I moved the \x1b[m control sequence to the prompt creation in cli.rs as this feels like the more logical home for controlling what the prompt looks like. I can think of better ways to do this in future but this should be a fine solution for now.

In future it would probably make sense to completely separate prompts (be it, internal or external) from this code so it can be configured as an isolated piece of code.

@Southclaws
Copy link
Copy Markdown
Contributor Author

🎉

image

@Southclaws Southclaws marked this pull request as ready for review October 8, 2019 20:51
@Southclaws
Copy link
Copy Markdown
Contributor Author

Southclaws commented Oct 9, 2019

I'll fix the remaining lint issues later, it might be simpler to just move the prompt code with feature conditions to a new file so the starship-prompt cfg feature checks aren't scattered around.

@Southclaws
Copy link
Copy Markdown
Contributor Author

Don't merge, it's still a little broken!

It's not passing the cwd to starship correctly for some reason...

@sophiajt sophiajt changed the title feat: starship prompt (DON'T MERGE) feat: starship prompt Oct 9, 2019
@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Oct 9, 2019

Changed title until you're ready :) (mostly so one one of us doesn't accidentally click merge just yet)

@jedahan
Copy link
Copy Markdown

jedahan commented Oct 9, 2019

I think github supports draft pull requests for this exact use case.

@Southclaws
Copy link
Copy Markdown
Contributor Author

Yeah it was a draft then the tests passed and I figured it was ready and now I can't switch it back to draft.

@sophiajt
Copy link
Copy Markdown
Contributor

@Southclaws no worries :) Just let us know when it's ready

@sophiajt
Copy link
Copy Markdown
Contributor

@Southclaws - oh, I had another question. I was playing with starship in Windows, and I noticed the default prompt has some unicode that the Windows terminal doesn't handle so well. Do you know if we can configure what the default prompt is when we land support into nushell? We can let users configure it from there but I figured we could make the default crossplatform-friendly.

@Southclaws
Copy link
Copy Markdown
Contributor Author

Southclaws commented Oct 11, 2019

Okay so I found the answer to the issue I was running into with paths: the problem was, Starship is moving to using logical paths by default but right now there's a configuration option to force it. For some reason, not setting this results in the path not updating correctly when you move around directories.

[directory]
use_logical_path = false

Is the Starship configuration solution. This should also be possible pragmatically but it might be best to wait until Starship does this by default.


In regards to cross platform, I actually forgot about this when evaluating Starship (which is not like me at all!) and it seems it doesn't explicitly claim Windows support on the readme though it seems there are quite a few mentions of Windows in the issues and Discord so there are certainly users with it.

It's worth noting that Starship itself requires a Powerline font, I know the new terminal is fine with these but not sure about the old PowerShell/Command Prompt emulators.

But it seems like it's not going to be a huge blocker, the prompt itself isn't doing anything complicated and Linux-specific by the looks of it, at the end of the day it just writes some text to stdout!

When I've got my Windows laptop again I will play around in the various terminal emulators and see what works and what doesn't.

@Southclaws
Copy link
Copy Markdown
Contributor Author

Okay so the Windows answer is good news! It's being worked on and is fairly close, this is the PR to track: starship/starship#470

@sophiajt
Copy link
Copy Markdown
Contributor

@Southclaws - just noticed that starship/starship#470 has landed (which is great!). Are there steps we need to do on our side to get ready to add support for it not that it's landed?

@Southclaws
Copy link
Copy Markdown
Contributor Author

Yeah possibly, I will look into it this weekend. I was following that PR and it seems like it shouldn't affect this much.

@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Nov 1, 2019

@Southclaws are there things we can help with? We're putting out 0.5.0 this next week, and it'd be fun to see if we could have starship support available.

@Southclaws
Copy link
Copy Markdown
Contributor Author

I've been super busy these last few weeks but that's slowing down now so I'll get some time to clean this up and finish it off soon!

@sophiajt sophiajt mentioned this pull request Nov 16, 2019
@Southclaws Southclaws deleted the starship-prompt branch November 17, 2019 13:26
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.

3 participants