feat: per-instance color palette + OSC 4/104 + computed dim - #31
Open
pocketprobe wants to merge 1 commit into
Open
feat: per-instance color palette + OSC 4/104 + computed dim#31pocketprobe wants to merge 1 commit into
pocketprobe wants to merge 1 commit into
Conversation
pocketprobe
force-pushed
the
feat/work/per-instance-palette-osc4
branch
from
August 30, 2026 01:36
354e867 to
0299253
Compare
Resolve SpotBugs MS_MUTABLE_ARRAY by design (not suppression) and build a real VT feature on top of it. Rebased onto work after DivByDiamond#30 merged; adapts DivByDiamond#30's follow-up test (942e1f9) to the per-instance accessors. Per-instance palette: the palette statics (COLORS/BRIGHT_COLORS/ COLORS_256) were public static final int[] — a process-global mutable shared across every Terminal. Privatized; each Terminal holds its own palette256 copy, initialized from the immutable default in RIS. A palette change never bleeds across terminals. Revert-and-fail proven: drop the clone and osc4DoesNotMutateStaticDefault / osc4PerTerminalIsolation fail. OSC 4 / 104: OSCManager was a pure stub. Real dispatch now — bounded char[] buffer, ST/BEL terminator, routes by numeric code to a per-code handler. The OSCHandler/OSC4/OSC104/OSCParse split mirrors CSISequenceHandler/CSIManager: a new OSC is a class + one registration. OSC 4 sets/queries a palette entry; OSC 104 resets one or all. Leaves OSC 0/2/8/10/11 as addable follow-ups. Palette sync across the network seam (Kimi K3 review F1): the palette never crossed the server->client diff — a server-side OSC 4 redefinition was invisible to players (the render path reads the client's palette256, which stayed default). Terminal now tracks paletteRevision (bumped on every palette write) + lastSentPaletteRevision; Snapshot carries int[] palette (nullable — null = unchanged, zero steady-state cost, ~1 KiB on change) when the revision moved, and captureFull forces it so a RIS reset snapshot carries the default palette (a client holding a prior change can't keep a stale one across reset). apply clones it onto the client. Cross-seam tests (osc4PaletteChangeSyncsToClientAcrossTheDiff etc.) exercise the server->client path the single-instance suite couldn't. OSC 4 reply xterm parity (F2): replies 16-bit per channel (rgb:%04x, duplicating the 8-bit value as xterm's UnMaskIt2 does, misc.c:2768) and reuses the query's own terminator (BEL query -> BEL reply, ST query -> ST reply), matching xterm unparseputc1(xw, final) (misc.c:2655). OSC 4 batched pairs (F3): a single OSC 4 may carry c;spec;c;spec pairs (xterm ChangeAnsiColorRequest, misc.c:2981); the loop stops on the first malformed entry. OSC 104 already looped correctly. OSC code parse (F4): the routing code is parsed without the 0-255 palette clamp (parseCode), so future OSC 777/1337 dispatch instead of silently dying at the palette edge; parseClampIndex stays for palette entries. Render path: indexed colors read terminal.palette256 and track OSC 4. DEFAULT_FOREGROUND/BACKGROUND + cursor read fixed-default accessors (xterm reserves those for OSC 10/11/12). Computed dim (SGR 2): replaces the fixed DIM_COLORS table with a computeFaint tail modifier (xterm's mechanism, util.c:5386) — scale the resolved color after mode/bold/blink resolution, so dim composes with bold and applies to bright/256/truecolor (the table only covered SIXTEEN_COLOR and silently ignored dim elsewhere). The 1/2 factor reproduces the old table exactly (0xAA0000 -> 0x550000) and is correct for our palette: xterm's 2/3 would collapse faint-bright onto normal (2/3*0xFF=0xAA), losing the fourth shade. Dim is foreground-only in xterm (getXtermBackground doesn't consume ATR_FAINT); the prior code dimmed the SIXTEEN_COLOR background, a divergence dropped. DIM_COLORS deleted — the last MS_MUTABLE_ARRAY, loop fully closed. Known divergence (separate follow-up): the ST-string managers (OSC/DCS/APC) swallow an ESC not followed by '\' rather than aborting (xterm illegal_parse aborts and re-enters the escape state). Harmless for well-formed OSC 4/104; matters for OSC 2 (title)/DCS sixel. Pre- existing across all three; a separate small PR will fix them together. Note on width changes: palette init lives in RIS only, not setWidth/resetRendition — a width change today is destructive (wipes screen/margins/cursor and would wipe the palette). DECSCPP is the non-destructive column primitive that separates "change columns" from "DECCOLM destructive reset"; the palette's setWidth-avoidance raises the urgency of that refactor promised in prior work. Verified: clean build (Error Prone on), 281 tests / 0 fail, Checkstyle 0, PMD 0, SpotBugs clean (all four palette MS_MUTABLE_ARRAY gone by design; Snapshot.palette EI_EXPOSE_REP/2 baselined, matching rowData's precedent; OSCHandler EI_EXPOSE_REP2 baselined, matching CSISequenceHandler; stale DIM_COLORS baseline entry pruned). Cross-seam tests + revert-and-fail proven. In-game confirmed (256color.sh full chart, ansi.sh --color-table four-shade + both reverse-video). Refs: §36 m7 Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review. A Kimi K3 adversarial review caught the network-seam miss and three xterm-parity gaps; all addressed.
pocketprobe
force-pushed
the
feat/work/per-instance-palette-osc4
branch
from
August 30, 2026 02:08
0299253 to
6319f45
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves
MS_MUTABLE_ARRAYby design (not suppression) and builds a real VT feature on top of it. Rebased ontoworkafter #30 (the df→d7 palette defaults) merged; adapts #30's follow-up test (942e1f9b) to the per-instance accessors.A Kimi K3 adversarial review caught a network-seam miss and three xterm-parity gaps in the first pass; all four are addressed here.
Per-instance palette
The palette statics (
COLORS/BRIGHT_COLORS/COLORS_256) werepublic static final int[]— a process-global mutable shared across everyTerminal. Privatized; eachTerminalnow holds its ownpalette256copy, initialized from the immutable default inRIS. A palette change never bleeds across terminals (a real VT has a per-terminal palette). Revert-and-fail proven: drop the clone andosc4DoesNotMutateStaticDefault/osc4PerTerminalIsolationfail (the static mutates and bleeds).Palette sync across the network seam
The first pass shipped OSC 4 server-side only: the palette never crossed the server→client diff, so a guest's
OSC 4;1;rgb:00/ff/00mutated the server's palette (query replies round-tripped, so the guest believed it worked) but the player's screen rendered the client's untouched default forever — the render path reads the clientTerminal'spalette256. The single-instance unit tests passed only because parse and render share oneTerminalin-test; the in-game scripts don't issue OSC 4, so they confirmed the render refactor and dim, not the title feature.Fix:
TerminaltrackspaletteRevision(bumped on every palette write —RIS/OSC4/OSC104) andlastSentPaletteRevision.Snapshotcarriesint[] palette(nullable —null= unchanged, zero steady-state cost; ~1 KiB only when the palette changed), populated when the revision moved.captureFull(the RIS reset path) forces the palette onto the snapshot, so a client holding a prior OSC 4 change can't keep a stale palette across a reset.applyclones it onto the client (terminal.palette256 = s.palette().clone()), keeping the client's array independent.capture/tick, dirty consumed once, all viewers get the same snapshot), solastSentPaletteRevisiononTerminalmatches the model — no per-client tracking.palette256istransient, so a joining client gets the default via constructor→RIS (runtime mutations aren't persisted; they sync via the diff).Cross-seam tests in
TerminalDiffTestexercise the server→client path the single-instance suite couldn't:osc4PaletteChangeSyncsToClientAcrossTheDiff,unchangedPaletteIsNotSentOnIncrementalDiff,osc104ResetSyncsToClientAcrossTheDiff,risResetSnapshotCarriesTheDefaultPalette.OSC 4 / 104
OSCManagerwas a pure stub that discarded all OSC payload. Real dispatch now: boundedchar[]buffer, ST/BEL terminator, routes by numeric code to a per-code handler. TheOSCHandler/OSC4/OSC104/OSCParsesplit mirrorsCSISequenceHandler/CSIManager— a new OSC is a class + one registration, not a switch arm. OSC 4 sets/queries a palette entry; OSC 104 resets one or all. Replies are OSC-framed (ESC ] … <terminator>, not CSI), viaterminal.io.putResponse(the DSR machinery). Leaves OSC 0/2/8/10/11 as addable follow-ups.xterm-parity fixes from the review:
rgb:%04x), duplicating the 8-bit value as xterm'sUnMaskIt2does (misc.c:2768), and reuses the query's own terminator (BEL query → BEL reply, ST query → ST reply), matchingunparseputc1(xw, final)(misc.c:2655).c;spec;c;specpairs (xtermChangeAnsiColorRequest,misc.c:2981); the loop stops on the first malformed entry. (OSC 104 already looped correctly.)parseCode), so future OSC 777/1337 dispatch instead of silently dying at the palette edge;parseClampIndexstays for palette entries.Render path
Indexed colors (
SIXTEEN_COLOR/SIXTEEN_COLOR_BRIGHT/TWO_FIFTY_SIX_COLOR) readterminal.palette256and so track OSC 4.DEFAULT_FOREGROUND/BACKGROUNDand cursor color read fixed-default accessors — xterm reserves those for OSC 10/11/12, not OSC 4 — so they don't track a redefinition.Computed dim (SGR 2)
Replaces the fixed
DIM_COLORStable with acomputeFainttail modifier (xterm's mechanism,util.c:5386): scale the resolved color after mode/bold/blink resolution, so dim composes with bold and now applies to bright/256/truecolor (the table only coveredSIXTEEN_COLORand silently ignored dim elsewhere). The 1/2 factor reproduces the old table exactly forSIXTEEN_COLOR(0xAA0000→0x550000), so existing dim text is unchanged — and it's the correct factor for our palette: xterm's 2/3 would collapse faint-bright into normal (2/3·0xFF=0xAA), losing the fourth shade (faint/normal/faint-bright/bright must stay 4 distinct tiers). Dim is foreground-only in xterm (getXtermBackgrounddoesn't consumeATR_FAINT); the prior code dimmed theSIXTEEN_COLORbackground, a divergence dropped here.DIM_COLORSdeleted — the lastMS_MUTABLE_ARRAY, loop fully closed.Note: adapts #30's follow-up test
#30's follow-up (
942e1f9b) movedTerminalColorsTestunder.../vm/terminal/color/and added a full 256-entry canonical validator referencingCOLORS/BRIGHT_COLORS/COLORS_256directly. This PR privatizes those arrays, so the validator is retargeted at clone accessors (getDefaultColors16/getDefaultBrightColors16/getDefaultPalette256) — every palette table now has a defensive-copy getter. Test logic unchanged; only field reads → accessor calls. AddedcomputeFaintScalesByHalf.Known divergence (separate follow-up)
The ST-string managers (
OSC/DCS/APC) swallow an ESC that isn't followed by\rather than aborting the string, where xterm'sillegal_parseaborts and re-enters the escape state. Harmless for well-formed OSC 4/104 (terminators can't appear in payload) but matters for OSC 2 (title) / DCS sixel, which take arbitrary text. Pre-existing across all three managers; a separate small PR will fix them together (xtermillegal_parseparity) rather than drive-by this one.Note on width changes
Palette init lives in
RISonly, notsetWidth/resetRendition— because a width change today is destructive (wipes screen/margins/cursor and would wipe the palette). Not all width changes should be destructive:DECSCPP(Select Columns Per Page) is the non-destructive column primitive that separates "change columns" from "DECCOLM destructive reset." The palette'ssetWidth-avoidance is a direct symptom of that coupling; it raises the urgency of the non-destructive-width refactor promised in prior work.Verification
Clean build (Error Prone on), 281 tests / 0 fail, Checkstyle 0, PMD 0, SpotBugs clean (all four palette
MS_MUTABLE_ARRAYgone by design;Snapshot.paletteEI_EXPOSE_REP/2baselined, matchingrowData's precedent;OSCHandlerEI_EXPOSE_REP2baselined, matchingCSISequenceHandler's identical pattern; staleDIM_COLORSbaseline entry pruned). Cross-seam tests + revert-and-fail proven. In-game confirmed with256color.sh(full 256 chart) andansi.sh --color-table(four distinct intensity tiers, both reverse-video modes).Refs: §36 m7
Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review.
A Kimi K3 adversarial review caught the network-seam miss and three xterm-parity gaps; all addressed.