prompt: default prompt colors to the terminal foreground - #1046
Conversation
|
We've covered this in the past. If we set these to none then there are no colors for the prompt at all for reedline users unless you're using something else to color your prompt. |
|
i don't think you're right (but i'm not fully sure either!). AFAIK emitting text without a color (which is the case in this patch) will just default to the terminal's foreground color, right? so the actual color of the prompt will depend on the user's terminal emulator what i mean is that there will be colors, it just won't be controlled by reedline |
|
This is what I'm talking about. I really don't want to lose the default coloring in reedline. Before this PR
After this PR
|
|
i understand where you're coming from but i see it the other way around: users of reedline (including the examples) should override the this is just my personal opinion, of course :) do you think me making the examples colorful again (by overriding the colors) but keeping the default color-less would be a good middle ground? |
maybe. i also wonder if we'd need to do this too in nushell when running |
pr nushell#992 reset stale sgr state before each repaint, which fixed prompt color bleed from previous command output. it did not fix the starship case reported in starship#6560 and nushell#16384 because reedline still applied its own default prompt colors immediately before printing the prompt. that meant a prompt implementation that returned ansi text without overriding `get_prompt_color()` still inherited reedline's defaults. for starship, `style = "none"` emits no color sequence at all, so the first visible character inherited reedline's green prompt color instead of the terminal's foreground color. this patch changes the default prompt, multiline, indicator, and right-prompt colors to the terminal defaults. prompts that want explicit colors can still override the color methods, but unstyled prompts now stay unstyled and no longer need an extra reset sequence to avoid the green first character.
|
okay, I've made the |
|
I think it's good. Can you look into what changes are needed in nushell to support this? |
I implemented this for nushell and have conflicting thoughts about it now. The more I wrote the more I thought that this fix was happening in the wrong place. Regardless of whether or not reedline has a default prompt color, nushell will still have one, so this fix by itself ends up being pointless. What I ended up doing was checking whether or not a custom prompt string was set or not (which is easy since nushell represents these as After thinking about it for a bit I think this should be fixed on starship's side. What do you think? I assume you agree, but I'll wait to read your thoughts before closing this and submitting the starship PR. |
|
I think you should be able to use nushell, oh-my-posh, starship, others to change your prompt and reedline should care that you do that. |
|
So you think it's ok to change the prompt color based on whether you have a custom prompt or not? If that's alright then I'm happy to implement that. |
kronberger-droid
added
A-Display
|
Hey there, what's the state on this? My understanding: This PR moves the opinion to the right layer. In nushell we then just implement the green colors in What do you think about that? |
|
@kronberger-droid i'll defer to what you think is appropriate. i just would like to see the same defaults in nushell. |
|
I personally see a few ways forward:
|
|
@iniw Option 2 is the most sensible to me: Reedline provides clean defaults, nushell impl uses nushell default coloring and the contract for custom prompt is essentially: "You want to customize the prompt, it's all up to you how it looks now." @iniw I can take care of the nushell side if you want. Do you think this PR already reflects that sentiment? |
|
Nice to finally have direction on this :)
Sounds great to me! I didn't think my solution on the nushell side was good, hence why I never PR'd it.
Yup, I think this PR makes sense. |
|
Great,while I search for a good solution in nushell I will review this and then land in them in sequence. |
|
Check out my nushell PR nushell/nushell#18686. We can either land this one for reedline also, or just keep it. I am fine with both, probably landing it makes it cleaner. |
|
What I don't understand is why we're not using nu_ansi_term for Color. I don't think we should be using any coloring, even crossterm, with nushell or reedline. |
|
Good point, might be that i just lazily copy how this PR or how the reedline defautl impl did it. |
|
Oh sorry, now I see it. The trait is defined really weird. |
|
I'd block it now. I don't want to introduce non-nu-ansi-term coloring. Maybe that's a mistake? Maybe crossterm colors already exist all over the place? I haven't looked. We specifically forked ansi-term for this purpose that's why I object to it. Hopefully crossterm color hasn't gone too far. |
|
We did definitely not introduce it. |
|
Thanks. Glad you're fixing this. Sorry to be a stickler on this. Wish I would've noticed it when it original creeped in. |
|
All good no problem. Its not that big of a deal to fix now. |
|
One thing that i noticed is that a non VT console like Windows legacy console would have used a |
Covers the SGR bytes emitted for prompt, indicator and right prompt on both buffer paths: the trait defaults, Color::Default emitting an explicit SGR 39 (nushell#1046), the use_ansi_coloring guards, and that crossterm's Green/Cyan were the bright palette entries.
|
ok, i'm fine with that. we can land this when you're ready. |
Covers the SGR bytes emitted for prompt, indicator and right prompt on both buffer paths: the trait defaults, Color::Default emitting an explicit SGR 39 (nushell#1046), the use_ansi_coloring guards, and that crossterm's Green/Cyan were the bright palette entries.
…1128) Prompt's color methods and the reedline::Color re-export now use nu_ansi_term::Color; the painter writes SGR via Color::prefix() instead of crossterm's SetForegroundColor. Defaults become LightGreen/LightCyan. crossterm's Green/Cyan are palette 10/14 (it spells the dark ones DarkGreen/DarkCyan) while nu-ansi-term's are 2/6, so a name-for-name translation would have darkened the prompt. Tests pin the emitted SGR bytes on both buffer paths, that Color::Default emits an explicit SGR 39 (#1046), and that the use_ansi_coloring guards hold. BREAKING CHANGE: Prompt::get_prompt_color, get_indicator_color and get_prompt_right_color now return nu_ansi_term::Color. Implementors porting crossterm color names need the Light* variants.
3 participants
#992 reset stale sgr state before each repaint, which fixed prompt color bleed from previous command output. it did not fix the starship case reported in starship/starship#6560 and nushell/nushell#16384 because reedline still applied its own default prompt colors immediately before printing the prompt.
that meant a prompt implementation that returned ansi text without overriding
get_prompt_color()still inherited reedline's defaults. for starship,style = "none"emits no color sequence at all, so the first visible character inherited reedline's green prompt color instead of the terminal's foreground color.this patch changes the default prompt, multiline, indicator, and right-prompt colors to the terminal defaults. prompts that want explicit colors can still override the color methods, but unstyled prompts now stay unstyled and no longer need an extra reset sequence to avoid the green first character.