fix: unify colour resolution behind a terminal profile - #84
Open
aymanbagabas wants to merge 1 commit into
Open
Conversation
A screenshot and a color assertion disagreed about what a cell was painted. `render/svg.rs` carried a private sixteen-color table and `assert/color.rs` carried a different one, so `expect --fg "#800000"` passed on a cell the screenshot drew `#e88388`. Both tables are deleted here and both callers resolve through one profile, which is what makes them agree by construction rather than by coincidence. The palette had to become configurable to fix it anyway: the two tables could only be collapsed by choosing which one was right, and that choice belongs to the user rather than to whichever module was read first. The shipped default is the VGA/xterm palette that `TERM=xterm-256color` already promises, which is what the assertion side used. A profile is read from `shell-use.toml` and sets scrollback and colors. Only the sixteen ANSI slots and the three defaults are configurable; indices 16-255 are the xterm color cube and gray ramp, which are fixed by the spec, so a config that could move them would let two sessions disagree about what `--fg 196` means. Profiles are named, and `--profile` selects one. The file is looked up nearest first, project before user, so a repository can pin the terminal its tests expect. Resolution happens in the CLI rather than the daemon: the daemon is long-lived and shared, so it has no single working directory to resolve a project-local config against, and a resolved profile travels on `Request::Open` the same way timeouts already do. Absent settings take the default, and the field is `#[serde(default)]`, so a client that predates this keeps the behavior it had. Scrollback moves from a hardcoded 5,000 to a configurable 10,000, matching alacritty's own default. Two things are deliberately errors rather than silent fallbacks: an unknown profile name, which reports the ones that exist, and a config file that does not parse, which would otherwise run the session with settings nobody asked for. A *missing* file stays fine, since running without one is normal. Screenshots will look different: the default background is now black rather than the previous dark blue-gray, and the palette is saturated rather than muted. Both are recoverable in a profile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
This was referenced Aug 4, 2026
aymanbagabas
marked this pull request as ready for review
August 4, 2026 21:07
Member
Author
|
@cpendery could you take a look? This is the base of a three PR stack:
Read them in that order. GitHub could not request you as a reviewer directly (I do not have push access here), hence the mention. |
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.
First of a three PR stack: #84 → #85 → #86.
A screenshot and a color assertion disagreed about what color a cell was, because each resolved palette indices through its own hardcoded table.
render/svg.rshad a privateThemewhere red was#e88388;assert/color.rshad anANSI16table where red was#800000. Nothing kept them in sync and nothing noticed they had drifted.The bug
Same program, same red text, on
main:Asserting the color the image actually paints failed, and asserting a color that appears nowhere passed. Either the picture was lying or the assertion was, and a test suite could not tell you which.
#e88388#800000Both tables are deleted. One
Colorstype resolves every index once, so the renderer and the assertion cannot disagree by construction.--fg '#800000'now passes and matches the picture.Profiles
The palette is no longer a private constant, so a session can be given one.
--configpicks the file and--profilethe entry; discovery cascades./shell-use.tomlthen~/.shell-use/shell-use.toml.$ shell-use run --config shell-use.toml --profile solarized -- bashA profile is exactly 19 values: the 16 ANSI colors plus foreground, background, and cursor. It is read only for the life of the session, so a program cannot rewrite the palette a test was pinned against. Anything outside 0-15 comes from a static xterm table, and
scrollbackdefaults to 10000.Notes for review
--fg <index>compares the cell's palette index, so it was never affected by this bug and still is not. Only#rrggbbresolves through the palette."fg": 1), not RGB, so existing baselines survive a palette change. There are tests pinning that.#c0c0c0on background#000000, which is what the assertion side already used, so no assertion changes meaning.