-
Notifications
You must be signed in to change notification settings - Fork 947
Fix RPROMPT_ON_NEWLINE when PROMPT_ON_NEWLINE is set to false #843
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @notrev !
Thanks for your contribution. Honestly I am a bit ambivalent on this change. It seems like a good thing to give the user more control over the vertical placement of the prompts, but with the risk that it could break in some circumstances..
@@ -1500,26 +1500,25 @@ powerlevel9k_prepare_prompts() { | |||
# Reset start time | |||
_P9K_TIMER_START=0x7FFFFFFF | |||
|
|||
if [[ "$POWERLEVEL9K_RPROMPT_ON_NEWLINE" != true ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Reason why that codeblock was inside of POWERLEVEL9K_PROMPT_ON_NEWLINE
is that we wanted to align left and right prompt accordingly. So, with your change it is possible to have no newline on the left prompt at all, but move the right prompt one line up..
I don't say that this is necessarily bad, as the user still has full control over this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an impediment for merging, I can add a check for newline
segments too.
On other hand, if you have POWERLEVEL9K_PROMPT_ADD_NEWLINE=true
and the right prompt is printed one line before the left prompt, you can think of the segments in the right prompt as related to the previous command, like the status
segment. I think this would provide another nice option for the right prompt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I wanted!
I'd like to go ahead and close out this PR, whether the decision is merging it or foregoing it. The use-case discussed by @notrev and seconded by @Zeta611 does indeed sound interesting, and i could see some people wanting to use the The only change, here, appears to be moving the conditional block outside of the other conditional. I'm okay with this as long as we expect existing configurations to continue to work with no changes. That is indeed the case, as I understand it - is that accurate, @dritter? |
Just found this one again - I don't want this to accidentally bitrot. Would be good to either just close it intentionally or merge it. @dritter - Based on your understanding of the code, if we merge this PR as-is, existing configurations would continue to work, right? If so, I'm okay merging, although this PR does also need an update to the README documenting this feature. Also, I just changed the target to |
Any ETA on when this gets merged? |
By having the
RPROMPT_ON_NEWLINE
if-block only inside thePROMPT_ON_NEWLINE == true
if-block,RPROMPT_ON_NEWLINE
is ignored in the case where(RPROMPT_ON_NEWLINE == false) && (PROMPT_ON_NEWLINE == false)
and there is anewline
segment in the left prompt. This causes the right prompt to always be drawn in the second line.This patch fixes the issue #840