Repository: JanDeDobbeleer/oh-my-posh
Pull request #999 (2021-09-25 08:52:40)

refactor: color logic performance
### Prerequisites

- [x] I have read and understood the `CONTRIBUTING` guide
- [x] The commit message follows the [conventional commits][cc] guidelines
- [x] Tests for the changes have been added (for bug fixes/features)
- [x] Docs have been added / updated (for bug fixes/features)

### Description

[Description of the change]

<!---

Tips:

If you're not comfortable with working with Git, we're working a guide (https://ohmyposh.dev/docs/contributing_git) to help you out.
Oh My Posh advises GitKraken (https://www.gitkraken.com/invite/nQmDPR9D) as your preferred cross platform Git GUI power tool.

-->

[cc]: https://www.conventionalcommits.org/en/v1.0.0/#summary


JanDeDobbeleer: @lnu I always disliked how the colorer worked (mainly trial and error development) so I took the time to make it cleaner and more performant by not repeating logic. Looks a lot more understandable now.

Patch:
diff --git a/src/ansi_color.go b/src/ansi_color.go
index e7771c69..772cbaa2 100644
--- a/src/ansi_color.go
+++ b/src/ansi_color.go
@@ -67,6 +67,9 @@ const (
 // This can include a valid hex color in the format `#FFFFFF`,
 // but also a name of one of the first 16 ANSI colors like `lightBlue`.
 func (a *AnsiColor) getAnsiFromColorString(colorString string, isBackground bool) string {
+	if colorString == Transparent || len(colorString) == 0 {
+		return colorString
+	}
 	colorFromName, err := getColorFromName(colorString, isBackground)
 	if err == nil {
 		return colorFromName
@@ -83,27 +86,26 @@ func (a *AnsiColor) writeColoredText(background, foreground, text string) {
 	if text == "" {
 		return
 	}
-	if foreground == Transparent && background != "" && a.terminalBackground != "" {
-		bgAnsiColor := a.getAnsiFromColorString(background, true)
+	// default to white fg if empty, empty backgrond is supported
+	if len(foreground) == 0 {
+		foreground = a.getAnsiFromColorString("white", false)
+	}
+	if foreground == Transparent && len(background) != 0 && len(a.terminalBackground) != 0 {
 		fgAnsiColor := a.getAnsiFromColorString(a.terminalBackground, false)
-		coloredText := fmt.Sprintf(a.ansi.colorFull, bgAnsiColor, fgAnsiColor, text)
+		coloredText := fmt.Sprintf(a.ansi.colorFull, background, fgAnsiColor, text)
 		a.builder.WriteString(coloredText)
 		return
 	}
-	if foreground == Transparent && background != "" {
-		ansiColor := a.getAnsiFromColorString(background, false)
-		coloredText := fmt.Sprintf(a.ansi.colorTransparent, ansiColor, text)
+	if foreground == Transparent && len(background) != 0 {
+		coloredText := fmt.Sprintf(a.ansi.colorTransparent, background, text)
 		a.builder.WriteString(coloredText)
 		return
-	} else if background == "" || background == Transparent {
-		ansiColor := a.getAnsiFromColorString(foreground, false)
-		coloredText := fmt.Sprintf(a.ansi.colorSingle, ansiColor, text)
+	} else if len(background) == 0 || background == Transparent {
+		coloredText := fmt.Sprintf(a.ansi.colorSingle, foreground, text)
 		a.builder.WriteString(coloredText)
 		return
 	}
-	bgAnsiColor := a.getAnsiFromColorString(background, true)
-	fgAnsiColor := a.getAnsiFromColorString(foreground, false)
-	coloredText := fmt.Sprintf(a.ansi.colorFull, bgAnsiColor, fgAnsiColor, text)
+	coloredText := fmt.Sprintf(a.ansi.colorFull, background, foreground, text)
 	a.builder.WriteString(coloredText)
 }
 
@@ -116,6 +118,16 @@ func (a *AnsiColor) write(background, foreground, text string) {
 	if len(text) == 0 {
 		return
 	}
+
+	getAnsiColors := func(background, foreground string) (string, string) {
+		inverted := foreground == Transparent && len(background) != 0
+		background = a.getAnsiFromColorString(background, !inverted)
+		foreground = a.getAnsiFromColorString(foreground, false)
+		return background, foreground
+	}
+
+	bgAnsi, fgAnsi := getAnsiColors(background, foreground)
+
 	text = a.ansi.escapeText(text)
 	text = a.ansi.formatText(text)
 	text = a.ansi.generateHyperlink(text)
@@ -123,29 +135,27 @@ func (a *AnsiColor) write(background, foreground, text string) {
 	// first we match for any potentially valid colors enclosed in <>
 	match := findAllNamedRegexMatch(`<(?P<foreground>[^,>]+)?,?(?P<background>[^>]+)?>(?P<content>[^<]*)<\/>`, text)
 	for i := range match {
-		extractedForegroundColor := match[i]["foreground"]
-		extractedBackgroundColor := match[i]["background"]
-		if col := a.getAnsiFromColorString(extractedForegroundColor, false); col == "" && extractedForegroundColor != Transparent && len(extractedBackgroundColor) == 0 {
-			continue // we skip invalid colors
+		fg := match[i]["foreground"]
+		bg := match[i]["background"]
+		if fg == Transparent && len(bg) == 0 {
+			bg = background
 		}
-		if col := a.getAnsiFromColorString(extractedBackgroundColor, false); col == "" && extractedBackgroundColor != Transparent && len(extractedForegroundColor) == 0 {
-			continue // we skip invalid colors
+		bg, fg = getAnsiColors(bg, fg)
+		// set colors if they are empty
+		if len(bg) == 0 {
+			bg = bgAnsi
 		}
-		// reuse function colors if only one was specified
-		if len(extractedBackgroundColor) == 0 {
-			extractedBackgroundColor = background
-		}
-		if len(extractedForegroundColor) == 0 {
-			extractedForegroundColor = foreground
+		if len(fg) == 0 {
+			fg = fgAnsi
 		}
 		escapedTextSegment := match[i]["text"]
 		innerText := match[i]["content"]
 		textBeforeColorOverride := strings.Split(text, escapedTextSegment)[0]
-		text = a.writeAndRemoveText(background, foreground, textBeforeColorOverride, textBeforeColorOverride, text)
-		text = a.writeAndRemoveText(extractedBackgroundColor, extractedForegroundColor, innerText, escapedTextSegment, text)
+		text = a.writeAndRemoveText(bgAnsi, fgAnsi, textBeforeColorOverride, textBeforeColorOverride, text)
+		text = a.writeAndRemoveText(bg, fg, innerText, escapedTextSegment, text)
 	}
 	// color the remaining part of text with background and foreground
-	a.writeColoredText(background, foreground, text)
+	a.writeColoredText(bgAnsi, fgAnsi, text)
 }
 
 func (a *AnsiColor) string() string {
diff --git a/src/block.go b/src/block.go
index 00208e52..1ad47621 100644
--- a/src/block.go
+++ b/src/block.go
@@ -82,17 +82,19 @@ func (b *Block) setStringValues() {
 }
 
 func (b *Block) foreground() string {
-	if b.previousActiveSegment != nil && b.activeSegment.foreground() == Inherit {
+	color := b.activeSegment.foreground()
+	if b.previousActiveSegment != nil && color == Inherit {
 		return b.previousActiveSegment.foreground()
 	}
-	return b.activeSegment.foreground()
+	return color
 }
 
 func (b *Block) background() string {
-	if b.previousActiveSegment != nil && b.activeSegment.background() == Inherit {
+	color := b.activeSegment.background()
+	if b.previousActiveSegment != nil && color == Inherit {
 		return b.previousActiveSegment.background()
 	}
-	return b.activeSegment.background()
+	return color
 }
 
 func (b *Block) renderSegments() string {
diff --git a/themes/bubbles.omp.json b/themes/bubbles.omp.json
index b6d580df..283098b1 100644
--- a/themes/bubbles.omp.json
+++ b/themes/bubbles.omp.json
@@ -1,5 +1,6 @@
 {
   "$schema": "https://raw.githubusercontent.com/JanDeDobbeleer/oh-my-posh/main/themes/schema.json",
+  "final_space": true,
   "blocks": [
     {
       "type": "prompt",
@@ -176,16 +177,6 @@
               "text": "\u276F",
               "postfix": ""
           }
-        },
-        {
-          "type":"text",
-          "style": "plain",
-          "foreground": "transparent",
-          "properties": {
-              "prefix": "",
-              "text": " ",
-              "postfix": ""
-          }
         }
       ]
     }


Test patch:
diff --git a/src/ansi_color_test.go b/src/ansi_color_test.go
index bd128319..e9c4f50f 100644
--- a/src/ansi_color_test.go
+++ b/src/ansi_color_test.go
@@ -133,7 +133,7 @@ func TestWriteColorInvalid(t *testing.T) {
 	}
 	text := "This is white, <invalid>this is orange</>, white again"
 	renderer.write("#193549", "invalid", text)
-	assert.Contains(t, renderer.string(), "<invalid>")
+	assert.NotContains(t, renderer.string(), "<invalid>")
 }
 
 func TestGetAnsiFromColorStringBg(t *testing.T) {