Refactor/more themes

[nojhan]

• refactor the powerline with a more consistent color scheme

• rename powerline_full as powerline_plus

• add two themes

• adds support for features from LP 2.2

[nojhan]

Screenshots are still missing an update.

[Rycieos]

Good start. I loaded each theme, and besides powerpuff, they all work well at first glance. I'll trust you on the color changes, though they need docs mentions that they changed. Unless we want to treat this as an entirely new theme, not one we are splitting off.

[nojhan]

I'll trust you on the color changes, though they need docs mentions that they changed. Unless we want to treat this as an entirely new theme, not one we are splitting off.

I don't have an opinion on that, so I'll let you decide.

[nojhan]

Or maybe we could keep the former color scheme as a legacy.conf preset?
Following my researches on colors, it may also be interesting to have a high contrast theme for powerline. Given that the background coloring is less prone to dark/light background incompatibilities, it seems feasible with the 256 colors space.

[Rycieos]

I don't have an opinion on that, so I'll let you decide.

I guess we don't have a good reason to not be explicit in documentation. I think what makes sense is that I release this theme as it is right now as v1.0.0, which supports Liquid Prompt 2.1 probably. Then you can target this PR to v1.1, and mark changes in the docs as such.

Or maybe we could keep the former color scheme as a legacy.conf preset?

Yes, I like that idea a lot.

Following my researches on colors, it may also be interesting to have a high contrast theme for powerline. Given that the background coloring is less prone to dark/light background incompatibilities, it seems feasible with the 256 colors space.

Sure, why not, presets are pretty easy to add and maintain.

In general this looks good, just a few issues.

[Rycieos]

My only issue with these screenshots is that the cursor character looks different between them. Probably because it is blinking? A small nitpick to be sure.

[nojhan]

Yes. This comes from the automated screenshot scripts, so that the cursor is always displayed. Should I patch them in Gimp or is it OK as is?

[Rycieos]

My need for perfection wants it patched out, but you can do whatever.

[nojhan]

I think what makes sense is that I release this theme as it is right now as v1.0.0, which supports Liquid Prompt 2.1 probably. Then you can target this PR to v1.1, and mark changes in the docs as such.

OK!

[Rycieos]

Looking pretty good. Everything works now. The docs are failing because they are trying to link to things in the Liquid Prompt docs that are not released to stable yet.

Only issue is that the powerpuff theme is very slow on Bash. This isn't reflected:

$ time __lp_set_prompt
real   	0m0.185s
user   	0m0.156s
sys    	0m0.030s

But from hitting enter to a new prompt appearing is almost exactly a second. If it's not something you are seeing, then I'll have to dig in.

[nojhan]

I am starting to see that themes involving _lp_fill with a heavy left-hand side are noticeably slower than their left-aligned counterparts. My guess is that striping escaped characters on big sections is costly, but I haven't had time to check.

[Rycieos]

I should look into if we can improve the string multiplication/concatenation loop...

I found this very helpful SO answer which includes a script to test possible implementations.

There are two solutions that are faster than the loop we currently use, but they have issues that prevent us from using them (mainly that they use printf tricks, which means we would need a subshell to capture the output).

So that's out. But while our solution scales linearly with repetition count, I don't think it is causing our issues here. 100 count took 0.0004 seconds, and 200 count took 0.0008 seconds.

I ran some tests to see if there was anything obviously slow, but nothing jumped out. I used this script to see that data:

(
  N=`date +%s%N`
  PS4='+[$(((`date +%s%N`-$N)/1000000))ms][${BASH_SOURCE}:${LINENO}]: ${FUNCNAME[0]:+${FUNCNAME[0]}(): }'
  set -x
  __lp_set_prompt
) 2>&1 | while read l; do
  [[ ! ${l%%[*} =~ ^\+ ]] && echo $l && continue
  i=`echo $l | sed 's#[^0-9]*\([0-9]\+\).*#\1#'`
  echo $l | sed "s#${i}ms#${i}ms+$(($i-${p-0}))ms#"
  p=$i
done

Each line shows at least 2ms because the date call takes that long. I couldn't find any line that took longer than 4ms.

I think your hunch is right, but also I think it is a function of how many lines are executed. The default theme executed 526 lines, where your powerpuff theme executed 1833. I think that is why it is much slower.

[nojhan]

After a couple of tests, it seems much faster to avoid having a right-aligned segment.
And thus not calling __lp_strip_escapes (that _lp_fill uses to compute the visible width of left and right segments).

The SO answer you mentioned notes:

avoid Bash's global substring replacement with large strings
(e.g., ${var// /=}), as it is prohibitively slow

And __lp_strip_escapes is heavily using global substring replacements.

If that's true, a fix would be to have a function that computes a string's "visible" length (not taking into account escaped characters), without using substring replacements.

[Rycieos]

a fix would be to have a function that computes a string's "visible" length (not taking into account escaped characters), without using substring replacements.

Indeed. And that would solve a lot of problems. See liquidprompt/liquidprompt#547 and liquidprompt/liquidprompt#599 for issues that would be helped for a solution here as well. Not to mention right prompt support has been a feature request in LP for a long time (liquidprompt/liquidprompt#154)

This is very difficult though. I have done a lot of research into this before (though I can't find my comments on it right now). I think the only true and correct solution is to have the terminal emulator do the work. It would look like this:

1. Set the "recall point" of the terminal (ESC 7: Save cursor position)
2. Get current coordinates (CSI 6 n: Report Cursor Position)
3. Print the string you want to measure.
4. Get current coordinates again.
5. Do math between 2 and 4.
6. Jump to the recall point (ESC 8: Restore cursor position)

This has many potential issues:

1. The string stays printed on the terminal. Best case, the program will overwrite that bit and the user will never notice. But the user might notice a flicker. Worst case, we don't print enough characters later and not everything is erased.
2. Terminal emulator support might be an issue. Though these seem to be normal xterm commands, and so far no one has reported any issues with the terminal title feature, which relies on similar commands.
3. Reading the terminal emulator report might be buggy, or not reliable. In my experience so far it seems ok, but it also prints the response to the terminal no matter what I do: printf "\033[6n" ; read -r -n100 -dR var ; printf '%q\n' "$var". It could be possible to get into a situation where the terminal hangs.
4. Using ESC 7: Save cursor position seems to overwrite any previous save to the register (it is not a stack), which could break something if a user was depending on that register.
5. It will get tripped up by newline characters. But that might be what is wanted.

That said, this my implementation. Works on both Bash and Zsh. I would love a version that doesn't have the above issues.

string_length() {
  local string="$1"
  local pos
  printf "\0337"
  printf "\033[6n"
  IFS="" read -r -dR pos

  printf "\0338"

  pos="${pos#$'\E['}"
  local old_row="${pos%;*}"
  local old_column="${pos#*;}"

  printf '%s' "$string"

  printf "\033[6n"
  IFS="" read -r -dR pos

  printf "\0338"

  pos="${pos#$'\E['}"
  local new_row="${pos%;*}"
  local new_column="${pos#*;}"

  length=$(( (new_row - old_row) * COLUMNS + (new_column - old_column) ))
}