feat(core): add indexed color (0-255) and OSC palette control #559
Conversation
- Extend RGBA class with colorType, index properties and fromIndex() method
- Add parseColor() support for 'ansi:N' format, numeric indices, and {index: N} objects
- Add indexToApproximateRgb() for converting indexed colors to RGB approximations
- Extend Zig Cell structure with fg/bg color type and index tracking
- Add indexed color ANSI escape sequence output (ESC[38;5;Nm / ESC[48;5;Nm)
- Add default color support (ESC[39m / ESC[49m)
- Add terminal palette set/reset methods for OSC 4/10/11/12 color control
There was a problem hiding this comment.
Pull request overview
This pull request adds support for terminal indexed colors (0-255) and OSC-based palette control to enable dynamic terminal color modification. However, the implementation has a critical architectural issue: the color type metadata (indexed vs RGB vs default) is not transmitted across the FFI boundary between TypeScript and Zig, rendering the indexed color feature non-functional in practice.
Changes:
- Extended RGBA class with color type tracking (indexed, RGB, default) and added parsing support for numeric indices and "ansi:N" format
- Added OSC palette control methods for dynamically setting and resetting terminal colors
- Extended Zig buffer Cell struct with color type fields and updated renderer to output appropriate ANSI sequences based on color type
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/lib/RGBA.ts | Added color type tracking, indexed color support, and extended ColorInput type |
| packages/core/src/lib/terminal-palette.ts | Added OSC sequence methods for setting/resetting palette colors |
| packages/core/src/renderables/EditBufferRenderable.ts | Updated type annotations to use expanded ColorInput type |
| packages/core/src/zig/ansi.zig | Added ColorType enum, Color struct, and indexed color output functions |
| packages/core/src/zig/buffer.zig | Extended Cell and OptimizedBuffer with color type fields and added setCellWithColorType method |
| packages/core/src/zig/renderer.zig | Updated rendering logic to output indexed/default color ANSI codes based on cell color type |
Comments suppressed due to low confidence (1)
packages/core/src/zig/buffer.zig:925
- When creating Cell structs in drawChar and setCellWithAlphaBlending, the color type fields are not specified, causing them to default to .rgb with index 0. This means indexed and default colors passed from TypeScript will be converted to RGB during rendering. These methods need to either receive color type parameters or extract them from an extended FFI interface.
pub fn drawChar(
self: *OptimizedBuffer,
char: u32,
x: u32,
y: u32,
fg: RGBA,
bg: RGBA,
attributes: u32,
) !void {
if (!self.isPointInScissor(@intCast(x), @intCast(y))) return;
if (isRGBAWithAlpha(bg) or isRGBAWithAlpha(fg)) {
try self.setCellWithAlphaBlending(x, y, char, fg, bg, attributes);
} else {
self.set(x, y, Cell{
.char = char,
.fg = fg,
.bg = bg,
.attributes = attributes,
});
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… add tests - Add FFI functions to pass color type + index metadata to Zig - Update buffer.ts to use new FFI when colors have type info - Add parseColor context param for correct default fg/bg handling - Add hex validation in normalizeHex() with clear error messages - Remove unused Zig Color struct - Add tests for OSC palette methods and indexed color parsing
Keep colorType/index through styled text, box borders/fills, and renderer background so ANSI palette theming (e.g. pywal) works consistently.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Normalize colorType/index before passing to Zig and add RGBA coverage for default/indexed inputs.
@opentui/core
@opentui/react
@opentui/solid
@opentui/core-darwin-arm64
@opentui/core-darwin-x64
@opentui/core-linux-arm64
@opentui/core-linux-x64
@opentui/core-win32-arm64
@opentui/core-win32-x64
commit: |
| @@ -1,8 +1,16 @@ | |||
| export const COLOR_TYPE_RGB = 0 | |||
There was a problem hiding this comment.
I would make this an enum.
There was a problem hiding this comment.
Agreed an enum would be cleaner. I didn’t change it in this PR because those values cross the TS↔Zig FFI boundary (0/1/2) and we’re already touching a lot of call sites/ABI surfaces here; keeping plain numeric constants reduced churn and made it easier to verify we stayed aligned with Zig. If you’d like, I can do a small follow-up commit that introduces a ColorType enum (backed by the same 0/1/2 values) and updates the internal TS usages without changing the FFI encoding.
packages/core/src/lib/RGBA.ts
Outdated
| } | ||
|
|
||
| export function parseColor(color: ColorInput): RGBA { | ||
| export function parseColor(color: ColorInput, context: "fg" | "bg" = "fg"): RGBA { |
There was a problem hiding this comment.
parseColor is used in so many places... I currently don't think it should be context aware.
There was a problem hiding this comment.
Fixed: parseColor is stateless again. Added parseFgColor() / parseBgColor() helpers so only callers that need bg semantics opt in, while the default API remains unchanged.
| setPaletteColor(index: number, hex: string): void | ||
| setForeground(hex: string): void | ||
| setBackground(hex: string): void | ||
| setCursorColor(hex: string): void |
There was a problem hiding this comment.
There is a method on renderer already to set the cursor color I think. That would be duplicated.
There was a problem hiding this comment.
Good call — agreed. The renderer already exposes cursor color control (CliRenderer.setCursorColor(...)), so I removed setCursorColor/resetCursorColor from TerminalPalette (and updated the related tests) to avoid duplicating the API surface.
|
Thanks for taking a stab at this. Having I'd like to try and keep the API surface impact as small as possible, like keeping |
* upstream/main: Fix node using itself as anchor resulting in incorrect ordering (anomalyco#552) chore(core): add Code and ScrollBox construct helpers (anomalyco#565) fix(react): share app context across build entrypoints (anomalyco#571) feat(core): add SIGPIPE, SIGBUS, SIGFPE to default exit signals (anomalyco#547) fix(react/solid): add missing markdown element (anomalyco#569)
Replace parseColor fg/bg context arg with parseFgColor/parseBgColor helpers and remove TerminalPalette cursor color methods to avoid duplicating renderer API.
…ndexed-colors * origin/feat/indexed-colors:
I updated things so parseColor is stateless again. Callers that need “default background” semantics now opt into On the representation: I agree that an RGBA + meta packed float (or similar) would be a nice improvement to reduce object metadata overhead and allow the color type/index to travel naturally with the buffer. That’s a larger refactor—it would touch FFI structs, buffer layout, and a fair bit of Zig/TS packing code—so I kept this PR focused on correctness and on wiring the metadata through all render paths. I can follow up with a separate PR (maybe??) that explores the packed-f32 approach while keeping the external API stable. |
|
It's still alpha, so I wouldn't mind breaking some APIs at this stage to get it "right". But also, what high level APIs would break? |
Totally fair — if we’re willing to break some APIs now, doing the packed RGBA + meta is probably the cleanest long-term shape.
What would likely change is more “internal/FFI plumbing” than user-facing APIs:
|
|
I think this PR can merge as-is for correctness, and then if you’re happy with the direction, I can open a small follow-up PR that experiments with the packed RGBA+meta representation (bit-packing type/index into an extra float). The aim would be to keep the public |
|
Looking at this deeper, "a safer phased plan” -so to say- is if we do it later as a follow-up PR with those stages (maybe!) - and is becoming a large PR as it is...: Step 1 (no buffer layout change yet)
Step2 (packed meta, but keep separate arrays)
Step 3 (maybe some optimization)
|
|
That could make sense to do it step by step. @simonklee what do you think? @bresilla there is some segfault in the tests currently though. |
Allow SyntaxStyle.fromStyles to accept ColorInput while normalizing to RGBA for FFI safety, and guard FFI color metadata against non-RGBA inputs.
Sorry — I was initially only running core tests. Should be fixed now... Anyway, Solid test use Edit: still not passing, gonna check it later today. Edit2: I think the error is not related to the code i added... I think. Sorry not very good at js/ts/bun things. But according to my understanding the error is about bun version bla bla bla... :P |
simonklee
left a comment
simonklee
left a comment
There was a problem hiding this comment.
In general its hard to know if all edge-cases have been covered because there are many of these draw paths. I walked through a couple and found a few that still bypass the new colorType/index
handling. I don't know that tests are good enough for us to catch all of these either.
| } | ||
|
|
||
| // Grayscale (232-255) | ||
| const gray = (index - 232) * (1.0 / 23) |
There was a problem hiding this comment.
xterm grayscale uses values 8, 18, ... 238 (steps of 10)
const gray = (8 + (index - 232) * 10) / 255
There was a problem hiding this comment.
There was a problem hiding this comment.
From the wiki article `level = gray * 10 + 8`
| const rowSliceAttrs = self.buffer.attributes[rowStartIndex .. rowStartIndex + rowWidth]; | ||
| const rowSliceBgType = self.buffer.bg_color_type[rowStartIndex .. rowStartIndex + rowWidth]; | ||
| const rowSliceBgIndex = self.buffer.bg_index[rowStartIndex .. rowStartIndex + rowWidth]; | ||
|
|
There was a problem hiding this comment.
Aren't we missing the fg meta in fast path here?
const rowSliceFgType = self.buffer.fg_color_type[rowStartIndex .. rowStartIndex + rowWidth];
const rowSliceFgIndex = self.buffer.fg_index[rowStartIndex .. rowStartIndex + rowWidth];
There was a problem hiding this comment.
Yeah, sorry, mistake. Will add that :)
I will try to |
|
We should probably get some of the draw paths test covered more thoroughly and make sure those tests pass on main as well, so we can assure at least somewhat same behaviour. |
|
@bresilla thank you for pushing this forward, I really want to have this. I think we should go through with integrating the color info into the RGBA+A bytes to pass around. I know it's a big undertaking, that's why it wasn't done yet. I get the idea of taking it step by step, but I think the way it's currently done it introduces complexity we have to clean up in the next step, while the first step can be clane and I don't like these spreading through the codebase when the RGBA+A type can encode and transport them as a single unit: pub const StyleDefinition = struct {
fg: ?RGBA,
bg: ?RGBA,
+ fg_color_type: ansi.ColorType = .rgb,
+ bg_color_type: ansi.ColorType = .rgb,
+ fg_index: u8 = 0,
+ bg_index: u8 = 0,
attributes: u32,
};It should not be necessary to have additional methods like |
Hey, sorry for late response. How would you suggest me doing it? Close this and start from scratch? I think that would be the cleanest way... |
|
I'll pick this up asap, if I get to it before you do I'll take your branch here as a base. I would just work from what we have here now. |
4 participants
Summary
Adds full support for terminal indexed colors (0-255) and OSC-based terminal palette control, enabling the use of the standard 256-color palette and dynamic color modification via ANSI/OSC escape sequences.
Makes this possible (with tools like pywal et al):
Changes
TypeScript (RGBA.ts)
COLOR_TYPE_RGB,COLOR_TYPE_INDEXED,COLOR_TYPE_DEFAULTconstantsRGBAclass withcolorType,indexpropertiesRGBA.fromIndex(index),RGBA.defaultForeground(),RGBA.defaultBackground()static methodsisIndexed(),isDefault(),isRgb()instance methodsindexToApproximateRgb()function with standard 256-color palette mappingColorInputtype to acceptstring | RGBA | IndexedColor | numberparseColor()to handle"ansi:N"format, numeric indices, and{index: N}objectsTypeScript (terminal-palette.ts) - OSC Palette Control
setPaletteColor(index, hex)- dynamically set any of the 256 palette colors (OSC 4)setForeground(hex)- set terminal default foreground color (OSC 10)setBackground(hex)- set terminal default background color (OSC 11)setCursorColor(hex)- set cursor color (OSC 12)resetPaletteColor(index)- reset palette color to terminal default (OSC 104)resetForeground()- reset foreground to default (OSC 110)resetBackground()- reset background to default (OSC 111)resetCursorColor()- reset cursor color to default (OSC 112)Zig (ansi.zig)
ColorTypeenum:rgb,indexed,defaultColorstruct combining RGBA with type and indexindexToApproximateRgba(),fgIndexedColorOutput(),bgIndexedColorOutput()fgDefaultOutput(),bgDefaultOutput()for default color resetZig (buffer.zig)
Cellstruct withfg_color_type,bg_color_type,fg_index,bg_indexOptimizedBufferZig (renderer.zig)
\x1b[38;5;Nm/\x1b[48;5;Nmfor indexed colors\x1b[39m/\x1b[49mfor default colorsUsage Examples
Indexed Colors
OSC Palette Control
Testing