Add option to insert newline when prompt is long #617

PandaWill · 2020-09-09T22:55:21Z

Add new setting LP_MINIMUM_CMD_LENGTH, which controls when a newline is added. Setting defaults to 0 (disabled).

See feature request #599.

I had to use perl for removing the non-printable characters from the prompt. It doesn't feel like the nicest solution, but I couldn't remove them with bash pattern matching or sed. This was due to the requirement to match the 2 characters at the end of each block: \] .

Let me know if there's anything you'd like changed, I'm keen to do all I can to get this merged. I only discovered this project recently and it's almost perfect - this is the missing piece of the puzzle for me!

Rycieos

Thanks for the PR! As I'm sure you saw on the feature request, I agree this is a needed feature.

A few things to note:

Even with this check, there a quite a few scenarios where this will not work right or make it worse. This ignores how wide your terminal is, and could add a newline where it doesn't make sense at all. A better implementation would decide to wrap based on how many columns are left, not how many have been taken. This is obviously harder. Imagine a scenario where the prompt is 60 characters, but the terminal is 55 characters. Should you add a newline? Probably not.

I wasn't planning on adding this feature until v2.1, as I wanted other reworking of stuff to happen first. Not saying that I can't merge this before then, only that I want this to look pretty close to the final form, not some partial thing that adds more work for v2.0.

Anyway, let's start with these changes and see if we can't come up with a solution for my above hypothetical scenarios.

liquidprompt

Rycieos · 2020-09-10T13:59:03Z

liquidprompt

@@ -1849,6 +1850,14 @@ _lp_set_prompt()
        # is set.
        PS1+="${LP_VCS}"

+        if [[ "$LP_MINIMUM_CMD_LENGTH" -ne 0 ]]; then
+            if [[ $(_remaining_space) -lt "$LP_MINIMUM_CMD_LENGTH" ]]; then
+                LP_MARK_PREFIX=$'\n'


Technically, even though it isn't documented as such, $LP_MARK_PREFIX is a valid configuration variable:
https://github.com/nojhan/liquidprompt/blob/deff598f30f097279d6f6959ba49441923dec041/liquidprompt#L347

Overriding it here would break the config of anyone who configures this. I'm not sure what the right way to do it would be (maybe add another var??), but this isn't it.

liquidprompt

Rycieos · 2020-09-10T14:07:09Z

liquidprompt

@@ -1849,6 +1850,14 @@ _lp_set_prompt()
        # is set.
        PS1+="${LP_VCS}"

+        if [[ "$LP_MINIMUM_CMD_LENGTH" -ne 0 ]]; then
+            if [[ $(_remaining_space) -lt "$LP_MINIMUM_CMD_LENGTH" ]]; then


No need for a new function: _lp_as_text() does the stripping already:

Suggested change

if [[ $(_remaining_space) -lt "$LP_MINIMUM_CMD_LENGTH" ]]; then

local printable_ps1=$(_lp_as_text "$PS1")

if [[ ${#printable_ps1} -lt "$LP_MINIMUM_CMD_LENGTH" ]]; then

Add new setting LP_MINIMUM_SPACE_AFTER_PROMPT. A new line is added when the prompt would leave less space than this in the terminal. See feature request liquidprompt#599.

PandaWill · 2020-09-10T17:32:28Z

Thanks for the quick response.

This ignores how wide your terminal is, and could add a newline where it doesn't make sense at all. A better implementation would decide to wrap based on how many columns are left, not how many have been taken. This is obviously harder. Imagine a scenario where the prompt is 60 characters, but the terminal is 55 characters.

I actually used $COLUMNS to take into account the terminal width and handled the edge cases in the _remaining_space function. I calculate remaining space as:

term_width - (prompt_length % term_width)

So for a prompt length of 60 and terminal 55, remaining space would be 50 = 55 - (60 % 55).

However, I admit it wasn't clear from the variable names or documentation that this what I was doing, so I've hopefully made that clearer.

I've taken perl out and used _lp_as_text as recommended.

I also added an _lp_length function to make it even more explicit what I'm doing. I had to break this down by shell type as I couldn't figure out a bash and zsh compatible way of expanding the prompt escape codes (e.g. \u for user). We need to expand them to count accurately.

I'm a bash user, but I've tested it with zsh and it seems to be working as I expect.

…or_long_prompt

Rycieos

Thanks for updating this PR. Unfortunately there is no way to merge this without first a rebase onto the current master. There have been too many changes since then.

I'm happy to help if you run into issues with rebasing, just let me know. You will want to look at the new docs/ folder to add documentation, and if you still need to add a new function like _remaining_space(), you should add tests for it in tests/test_utils.sh.

Rycieos · 2021-01-25T15:17:35Z

liquidprompt

+_lp_length()
+{
+    typeset len evaluated_ps1
+    typeset printable_ps1=$(_lp_as_text "$1")


This (typeset and assignment on the same line) is unsafe in zsh < 5.1:

% func() { typeset var=$(echo foo bar) echo "$var" } % func foo

Either wrap in quotes or break into two lines:

Suggested change

typeset printable_ps1=$(_lp_as_text "$1")

typeset printable_ps1="$(_lp_as_text "$1")"

Check for other issues like this, I see a few other instances.

Rycieos · 2021-01-25T15:32:23Z

liquidprompt

+    typeset printable_ps1=$(_lp_as_text "$1")
+
+    if $_LP_SHELL_bash; then
+        evaluated_ps1=$(printf '%s' "${printable_ps1@P}")


This is a Bash-4.4+ feature:
https://github.com/bminor/bash/blob/76404c85d492c001f59f2644074333ffb7608532/CHANGES#L2400-L2401
Liquidprompt supports Bash-3.2+, so this is a no-go.

How to evaluate escape codes is a known problem in Liquidprompt. See:

Terminal title: Make title bar work out-of-the-box with Gnome Terminal #609 (comment)

Ksh support: Ksh compatibility #608 (comment)

On the surface it is simple, and should not be hard to fix, but I think there could be issues that spring up when we try to replace all escape codes with strings.

Anyway, that is on the top of my to-do list for v2.1. I don't see a way to make this PR work without that first, so you might need to wait...

Rycieos · 2021-01-25T15:34:55Z

liquidprompt

+    len=${#evaluated_ps1}
+
+    echo $len


Post-rework Liquidprompt avoids using stdout to return values. Use a variable, named after the function:

Suggested change

len=${#evaluated_ps1}

echo $len

lp_prompt_length=${#evaluated_ps1}

Oh and name the function __lp_prompt_length(). The name is too generic, and lies about what it does.

Rycieos · 2021-01-25T15:40:29Z

liquidprompt

+        # add return code
+        PS1+="${LP_RUNTIME}${LP_ERR}"
+
+        # determine if we need to add new line before prompt mark
+        if (( LP_MINIMUM_SPACE_AFTER_PROMPT )); then
+            if [[ $(_remaining_space) -lt "$LP_MINIMUM_SPACE_AFTER_PROMPT" ]]; then
+                PS1+=$'\n'
+            fi
+        fi
+
+        # add prompt mark
+        PS1+="${LP_MARK_PREFIX}${LP_COLOR_MARK}${LP_MARK}${LP_PS1_POSTFIX}"


This part is elegant.

The problem is that this section is possibly in the user's control. If the user has a template file ($LP_PS1_FILE), this block will never be run.

If we are going to advertise this as a feature, it needs to work no matter what template or theme the user is running. I'm not sure at the moment how to fix that, something to worry about later after the other issues are fixed maybe.

Rmano · 2022-02-13T11:45:13Z

If I can help (by testing, basically) this feature on zsh, just tell me. It will be a great feature....

PandaWill force-pushed the feature/newline_for_long_prompt branch from 0530580 to e74fc50 Compare September 9, 2020 23:02

Rycieos requested changes Sep 10, 2020

View reviewed changes

Add option to insert newline when prompt is long

50234ed

Add new setting LP_MINIMUM_SPACE_AFTER_PROMPT. A new line is added when the prompt would leave less space than this in the terminal. See feature request liquidprompt#599.

PandaWill force-pushed the feature/newline_for_long_prompt branch from 8e55fe6 to 50234ed Compare September 10, 2020 17:31

Merge remote-tracking branch 'upstream/stable' into feature/newline_f…

a5b1906

…or_long_prompt

Rycieos requested changes Jan 25, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to insert newline when prompt is long #617

Add option to insert newline when prompt is long #617

PandaWill commented Sep 9, 2020

Rycieos left a comment

Rycieos Sep 10, 2020

Rycieos Sep 10, 2020

PandaWill commented Sep 10, 2020

Rycieos left a comment

Rycieos Jan 25, 2021

Rycieos Jan 25, 2021

Rycieos Jan 25, 2021

Rycieos Jan 25, 2021

Rmano commented Feb 13, 2022

	if [[ $(_remaining_space) -lt "$LP_MINIMUM_CMD_LENGTH" ]]; then
	local printable_ps1=$(_lp_as_text "$PS1")
	if [[ ${#printable_ps1} -lt "$LP_MINIMUM_CMD_LENGTH" ]]; then

	typeset printable_ps1=$(_lp_as_text "$1")
	typeset printable_ps1="$(_lp_as_text "$1")"

Add option to insert newline when prompt is long #617

Are you sure you want to change the base?

Add option to insert newline when prompt is long #617

Conversation

PandaWill commented Sep 9, 2020

Rycieos left a comment

Choose a reason for hiding this comment

Rycieos Sep 10, 2020

Choose a reason for hiding this comment

Rycieos Sep 10, 2020

Choose a reason for hiding this comment

PandaWill commented Sep 10, 2020

Rycieos left a comment

Choose a reason for hiding this comment

Rycieos Jan 25, 2021

Choose a reason for hiding this comment

Rycieos Jan 25, 2021

Choose a reason for hiding this comment

Rycieos Jan 25, 2021

Choose a reason for hiding this comment

Rycieos Jan 25, 2021

Choose a reason for hiding this comment

Rmano commented Feb 13, 2022