feat: support dynamic theme changes via Terminal.setTheme() and options.theme#14
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for dynamic theme updates at runtime. It adds a setColors method to the WASM interface and updates the Terminal class to manage an accumulated theme state, allowing for partial theme overrides. Review feedback identified several significant issues: the TypeScript setColors implementation incorrectly defaults missing fields to black, which the WASM layer applies directly; a logic error in the theme setter causes state to become stale if modified before the terminal is opened; and there is an inconsistency in color sentinel handling within the Zig implementation. Additionally, the current accumulation logic leads to a mismatch between the public options.theme and the internal terminal state.
| view.setUint32(offset, config.fgColor ?? 0, true); | ||
| offset += 4; | ||
|
|
||
| // bg_color (u32) | ||
| view.setUint32(offset, config.bgColor ?? 0, true); | ||
| offset += 4; | ||
|
|
||
| // cursor_color (u32) | ||
| view.setUint32(offset, config.cursorColor ?? 0, true); | ||
| offset += 4; | ||
|
|
||
| // palette[16] (u32 * 16) | ||
| for (let i = 0; i < 16; i++) { | ||
| view.setUint32(offset, config.palette?.[i] ?? 0, true); | ||
| offset += 4; | ||
| } |
There was a problem hiding this comment.
The setColors method is destructive when provided with a partial GhosttyTerminalConfig. Any omitted fields (like fgColor or palette entries) will default to 0 via the nullish coalescing operator. Since the Zig implementation in the WASM layer (added in this PR) applies these values directly without sentinel checks, this will unexpectedly reset those colors to black (0x000000). This is particularly problematic for the palette loop, which will zero out all 16 colors if the palette property is missing from the config.
| if (this.renderer && this.wasmTerm) { | ||
| // Merge partial theme with current accumulated theme. | ||
| // Null/undefined/empty resets to defaults. | ||
| const incoming = newValue && typeof newValue === 'object' ? newValue : {}; | ||
| const hasProperties = Object.keys(incoming).length > 0; | ||
| this.currentTheme = hasProperties | ||
| ? { ...this.currentTheme, ...incoming } | ||
| : { ...DEFAULT_THEME }; | ||
|
|
||
| // Update renderer (selection, cursor, palette colors) | ||
| this.renderer.setTheme(this.currentTheme); | ||
|
|
||
| // Update WASM terminal colors (for cell color re-resolution) | ||
| this.wasmTerm.setColors(this.buildThemeColorsConfig(this.currentTheme)); | ||
| } |
There was a problem hiding this comment.
The currentTheme state update is gated by the existence of this.renderer and this.wasmTerm. This creates a stale state bug: if options.theme is modified before open() is called (when components are null), currentTheme is never updated. When the terminal is later opened, it will use the correct initial colors, but the first partial theme update performed after opening will merge with the stale currentTheme from the constructor, reverting any pre-open changes. Additionally, the accumulation logic should be decoupled from component updates to ensure the private state remains consistent even if a component fails to initialize.
| if (this.renderer && this.wasmTerm) { | |
| // Merge partial theme with current accumulated theme. | |
| // Null/undefined/empty resets to defaults. | |
| const incoming = newValue && typeof newValue === 'object' ? newValue : {}; | |
| const hasProperties = Object.keys(incoming).length > 0; | |
| this.currentTheme = hasProperties | |
| ? { ...this.currentTheme, ...incoming } | |
| : { ...DEFAULT_THEME }; | |
| // Update renderer (selection, cursor, palette colors) | |
| this.renderer.setTheme(this.currentTheme); | |
| // Update WASM terminal colors (for cell color re-resolution) | |
| this.wasmTerm.setColors(this.buildThemeColorsConfig(this.currentTheme)); | |
| } | |
| // Merge partial theme with current accumulated theme. | |
| // Null/undefined/empty resets to defaults. | |
| const incoming = newValue && typeof newValue === 'object' ? newValue : {}; | |
| const hasProperties = Object.keys(incoming).length > 0; | |
| this.currentTheme = hasProperties | |
| ? { ...this.currentTheme, ...incoming } | |
| : { ...DEFAULT_THEME }; | |
| if (this.renderer) { | |
| this.renderer.setTheme(this.currentTheme); | |
| } | |
| if (this.wasmTerm) { | |
| this.wasmTerm.setColors(this.buildThemeColorsConfig(this.currentTheme)); | |
| } |
| +pub fn setColors(ptr: ?*anyopaque, config_ptr: ?*const GhosttyTerminalConfig) callconv(.c) void { | ||
| + const wrapper: *TerminalWrapper = @ptrCast(@alignCast(ptr orelse return)); | ||
| + const cfg = config_ptr orelse return; | ||
| + | ||
| + // Update foreground | ||
| + wrapper.terminal.colors.foreground = color.DynamicRGB.init(.{ | ||
| + .r = @truncate((cfg.fg_color >> 16) & 0xFF), | ||
| + .g = @truncate((cfg.fg_color >> 8) & 0xFF), | ||
| + .b = @truncate(cfg.fg_color & 0xFF), | ||
| + }); | ||
| + | ||
| + // Update background | ||
| + wrapper.terminal.colors.background = color.DynamicRGB.init(.{ | ||
| + .r = @truncate((cfg.bg_color >> 16) & 0xFF), | ||
| + .g = @truncate((cfg.bg_color >> 8) & 0xFF), | ||
| + .b = @truncate(cfg.bg_color & 0xFF), | ||
| + }); | ||
| + | ||
| + // Update cursor | ||
| + wrapper.terminal.colors.cursor = color.DynamicRGB.init(.{ | ||
| + .r = @truncate((cfg.cursor_color >> 16) & 0xFF), | ||
| + .g = @truncate((cfg.cursor_color >> 8) & 0xFF), | ||
| + .b = @truncate(cfg.cursor_color & 0xFF), | ||
| + }); | ||
| + | ||
| + // Update palette (all 16 colors, no sentinel) | ||
| + for (cfg.palette, 0..) |palette_color, i| { | ||
| + wrapper.terminal.colors.palette.set(@intCast(i), .{ | ||
| + .r = @truncate((palette_color >> 16) & 0xFF), | ||
| + .g = @truncate((palette_color >> 8) & 0xFF), | ||
| + .b = @truncate(palette_color & 0xFF), | ||
| + }); | ||
| + } | ||
| + | ||
| + // Force full redraw on next render | ||
| + wrapper.force_full_redraw = true; | ||
| +} |
There was a problem hiding this comment.
The Zig implementation of setColors is inconsistent with newWithConfig regarding color sentinels. While newWithConfig (line 844 of the patch) correctly checks if (color != 0) to allow defaults, setColors applies all values directly. This means a value of 0 (which is the default in the TS wrapper for missing fields) will be treated as valid black, making partial updates impossible via the low-level API. If the intention is to support pure black (0x000000), newWithConfig should be updated to remove the sentinel checks; otherwise, setColors should add them for consistency.
| this.currentTheme = hasProperties | ||
| ? { ...this.currentTheme, ...incoming } | ||
| : { ...DEFAULT_THEME }; |
There was a problem hiding this comment.
The accumulation logic here creates an inconsistency between the public options.theme and the actual terminal state. When a user sets term.options.theme = { background: 'red' }, the Proxy target is replaced with that object. If they later set term.options.theme = { foreground: 'blue' }, the Proxy target now only contains the foreground. However, currentTheme (and the display) will show both. Reading term.options.theme will return an object that does not reflect the active theme. To maintain xterm.js compatibility and API clarity, the theme should either be replaced entirely (no merge) or the Proxy should be updated to reflect the merged state.
d295d7f to
964e52e
Compare
…heme Today, the theme passed to the Terminal constructor is captured at open() time and never changes. Apps that need to switch themes at runtime (light/ dark toggle, accessibility preference change, multi-window state) had to dispose the Terminal and recreate it — which destroys scrollback, selection, and focus. This commit adds a runtime theme change path: - Public API: `Terminal.setTheme(theme)` updates the theme atomically and triggers a single render. Equivalent to assigning `options.theme = ...` via the existing options Proxy (also supported). - WASM bridge: new exports `terminal_set_theme` (full theme update) and the renderer is invalidated so the next frame redraws every cell with the new palette / background. - The renderer's color cache (introduced in older PRs) is cleared on theme change so old `rgb(...)` strings don't outlive their palette. Adds 12 new tests covering: full-theme update mid-session, ANSI palette update, default-color fallback when theme omits ansi colors, no-op on identical theme, render scheduling, options-proxy compatibility. Excludes the binary `ghostty-vt.wasm` from the upstream diff (CI / local `bun run build:wasm` rebuilds it from the updated patch). Co-authored-by: Brian Egan <brian.egan@verygood.ventures> Inspired-by: coder#144
964e52e to
a400311
Compare
|


Summary
Ports upstream PR #144: runtime theme changes.
Previously, theme was captured at
open()and never changed. Apps that needed light/dark toggles or per-window theming had to dispose and recreate the Terminal — destroying scrollback, selection, and focus.Public API
Terminal.setTheme(theme)— atomic theme update + single renderterminal.options.theme = newTheme— equivalent, via the existing options ProxyRenderer / WASM updates
terminal_set_theme(full theme) — Zig-side patch updated accordinglyrgb(...)strings don't outlive their paletteAdaptation vs upstream raw diff
ghostty-vt.wasmin the upstream PR diff is excluded — CI /bun run build:wasmrebuilds locally from the updatedpatches/ghostty-wasm-api.patch. Verified locally with Zig 0.15.2 (416 KB).theme: options.theme ?? {}preserved so callers without a theme keep the old defaults.Attribution
Thanks to @brianegan for the original implementation.
Test plan
bun run fmt && bun run lint && bun run typecheckbun run build:wasm— submodule + updated patch + Zig 0.15.2 → 416 KBbun test— 343 tests pass (12 new theme tests added by this PR), 0 failbun run build:libbun run dev:term.setTheme({ background: '#ffffff', foreground: '#000000' })mid-sessionRisk
Medium. Touches WASM ABI + renderer color path. 12 new tests cover the main code paths. Behavioural change is opt-in via
setTheme()— existing callers keep old behaviour.