Support setting extended ansi colors (16-255)#3905
Support setting extended ansi colors (16-255)#3905Tyriar merged 2 commits intoxtermjs:masterfrom silamon:extendedcolors
Conversation
Tyriar
left a comment
There was a problem hiding this comment.
Looks good, just one suggestion 👍
| 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]); | ||
| } | ||
| } |
There was a problem hiding this comment.
I didn't test this but it would lead to less work:
| 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]); | |
| } | |
| } |
There was a problem hiding this comment.
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]);
}
}There was a problem hiding this comment.
I'll set up another pr soon to fix that. Thanks @jerch .
There was a problem hiding this comment.
Yeah there was an issue with my code above, I think it was fixed in c786c6a though
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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)?
There was a problem hiding this comment.
How about setTheme({extendedAnsi: []}) to unset?
There was a problem hiding this comment.
Wouldn't a simple this.colors.ansi = DEFAULT_ANSI_COLORS.slice(); infront of all ansi color code do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll create a PR to address this.
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