Support setting extended ansi colors (16-255)

[silamon]

The API is slightly different than proposed in #3601. It is aligned with the current ITheme variables and taking a string[] array of hex values.

Fixes #3601

[Tyriar]

Looks good, just one suggestion 👍

[Tyriar]

I didn't test this but it would lead to less work:

Suggested change

	if (theme.extendedAnsi) {
	for (let ansiColor = 0; ansiColor < theme.extendedAnsi.length && ansiColor <= 255 - 16; ansiColor++) {
	this.colors.ansi[ansiColor + 16] = this._parseColor(theme.extendedAnsi[ansiColor], DEFAULT_ANSI_COLORS[ansiColor + 16]);
	}
	}
	if (theme.extendedAnsi) {
	const colorCount = Math.max(256, theme.extendedAnsi.length + 16);
	for (let i = 16; i < colorCount; i++) {
	this.colors.ansi[i] = this._parseColor(theme.extendedAnsi[i], DEFAULT_ANSI_COLORS[i]);
	}
	}

[jerch]

Are you sure thats doing the right thing? 🙈
If the purpose is to skip first 16 entries and to cap at 256, maybe this will work:

    if (theme.extendedAnsi) {
      const colorCount = Math.min(240, theme.extendedAnsi.length);
      for (let i = 0; i < colorCount; i++) {
        this.colors.ansi[i + 16] = this._parseColor(theme.extendedAnsi[i], DEFAULT_ANSI_COLORS[i + 16]);
      }
    }

[silamon]

I'll set up another pr soon to fix that. Thanks @jerch .

[Tyriar]

Yeah there was an issue with my code above, I think it was fixed in c786c6a though

[jerch]

Yeah partially (though I dont like i-16 on indexes, but thats just my defensive coding style to avoid potential underruns). Whats with max vs. min?

Another minor code smell:
It might be a good idea not to limit on a magic number of 256. Could this be pulled from this.colors.ansi.length or some setup value of the default containers? Again defensive style - just in case the number of colors might change in the future. Always fun to debug magic numbers later on, that seemed so obvious in the first place.

[jerch]

@silamon Oh well, this seems to be a rather good idea, otherwise thats not possible at all from outside?

Edit: Maybe setTheme should always start from a DEFAULT_ANSI_COLORS clone? Or are there any downsides to such a clean startover beside losing "stacking" setTheme usage? (which seems weird to me in the first place, not sure if ppl use it that way)
Edit2: Nevermind, stacking is not possible for setTheme, as it always pulls from DEFAULT_ANSI_COLORS for color 0-15, if the obj does not contain the corresponding entry. So yeah, maybe do it likewise for extended colors (reset first + apply new ones)?

[Tyriar]

How about setTheme({extendedAnsi: []}) to unset?

[jerch]

Wouldn't a simple this.colors.ansi = DEFAULT_ANSI_COLORS.slice(); infront of all ansi color code do?

[Tyriar]

Wouldn't a simple this.colors.ansi = DEFAULT_ANSI_COLORS.slice(); infront of all ansi color code do?

setTheme does indeed reset everything after looking at the code, so this would be the right approach. That makes sense as taking VS Code as an example, a theme may not specify some colors, in which case we would want the defaults not the previously active theme colors.

[silamon]

I'll create a PR to address this.

[Tyriar]

Looks great!