Add a light-terminal palette, with auto-detection and an override - #3
Add a light-terminal palette, with auto-detection and an override#3saforem2 wants to merge 2 commits into
Conversation
The renderer sets only foreground colors and lets the terminal background show through (render.go). Every color therefore encodes an assumption about that background — and every one of them assumed it was dark: note ink sat around 0.85 luma, the highlight borders were literally named "warm white" / "bright", and the cork was honey over black. On a light terminal the notes wash out to near-unreadable. Split the colors out of theme.go into a Palette value in palette.go, with darkPalette() reproducing today's colors byte-for-byte and lightPalette() as its counterpart. ApplyTheme() installs one into the package-level vars the draw sites already read, so no call site changed. The light palette is not a luma flip. Text inverts (ink becomes the darkest, most saturated value of its hue) but texture — cork specks, shadow dither — is placed at the same perceptual *distance* from a pale background as its dark counterpart sits from black, hue preserved and chroma eased ~8%. Deriving the texture by "make it dark enough" instead was visibly wrong: the board read as noise, with the drop shadows punching holes in the page. theme_test.go now enforces both rules separately. Selection: --theme=light|dark|auto (default auto) RT_THEME=light for shell rc / tmux config T toggles in-app, and is remembered "theme" in notes.json persists the choice (schema v5 -> v6) Auto queries the terminal background via lipgloss before Bubble Tea takes the screen. When that query goes unanswered — tmux without passthrough, some ssh sessions, a non-TTY — it reports dark, which is exactly today's behavior, and the override is there for when it guesses wrong. Both palettes expose the same nine tint names and the same nine SelBorderChoices in the same order, so a saved tint or highlightColor index means the same thing in either theme. Also adds five light fills to the background picker (paper, github light, solarized light, latte, gruvbox light), which previously offered only dark ones.
There was a problem hiding this comment.
Pull request overview
This PR introduces light/dark terminal-aware color palettes for the app’s foreground-only renderer, including auto-detection of terminal background plus user overrides (flag/env/in-app toggle) and persistence in the workspace save file.
Changes:
- Moves color definitions into a new
Palettemodel (darkPaletteunchanged; addslightPalette) and applies it viaApplyTheme(light bool)without changing draw call sites. - Adds theme selection logic:
--theme,RT_THEME, saved workspace preference, and OSC 11 background detection; plus an in-appTtoggle. - Bumps workspace schema to v6 to persist an optional
"theme"key; expands background fill picker with light fills and updates docs/tests accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents light/dark theme behavior, precedence, persistence, and updated schema + picker options. |
| internal/app/theme.go | Converts palette constants to package-level vars populated by ApplyTheme; background fill list gains light entries. |
| internal/app/palette.go | Adds Palette, dark/light palette data, and ApplyTheme to swap package-level colors. |
| internal/app/theme_mode.go | Adds ThemeMode parsing and terminal-background-based resolution for auto mode. |
| internal/app/storage.go | Bumps schema to v6 and persists/loads the workspace theme preference. |
| internal/app/notes.go | Adds Workspace.Theme plus helpers to parse/set/toggle the theme and apply palettes live. |
| internal/app/model.go | Adds T keybinding to toggle theme, re-apply borders, and toast the current mode. |
| internal/app/menu.go | Marks light background fills with a faint · indicator in the picker. |
| internal/app/theme_test.go | Adds contract tests for palette shape, contrast rules, and ApplyTheme behavior. |
| internal/app/theme_mode_test.go | Tests parsing, precedence helpers, and probe behavior. |
| internal/app/theme_persist_test.go | Tests persistence, v5 load behavior, and toggle/apply integration. |
| internal/app/menu_test.go | Ensures picker layout fits common terminals and validates background fills. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| mode := themeModeFrom(themeFlag, os.Getenv("RT_THEME")) | ||
| if mode == ThemeAuto { | ||
| mode = ws.ThemeMode() | ||
| } | ||
| light := ResolveTheme(mode) | ||
| ApplyTheme(light) | ||
|
|
||
| // Remember an explicit choice so the next run does not have to probe. | ||
| if mode != ThemeAuto { | ||
| ws.SetThemeMode(mode) | ||
| } |
There was a problem hiding this comment.
Good catch — confirmed and fixed in df1c960.
The root cause was that themeModeFrom returned a bare ThemeMode, so "the user typed auto" and "the user said nothing" were the same value, and Run() read both as "no override". It now returns an explicit-ness bool alongside the mode:
- only an absent flag and env var defer to the saved theme
- an explicit
autore-detects and clears the saved value, so the next launch detects too rather than reverting - a malformed
--themefalls through toRT_THEMEinstead of reading as a request for auto, and its warning now says "ignoring it" rather than the misleading "using auto"
Covered by TestExplicitAutoOverridesASavedTheme and TestExplicitAutoClearsTheSavedTheme, plus five new cases in the themeModeFrom table. README updated to say that --theme=auto is the way back to detection after pressing T.
themeModeFrom collapsed "the user typed auto" and "the user said nothing" into the same ThemeAuto value, and Run() treated ThemeAuto as "no override" and substituted the saved workspace theme. So `--theme=auto` could not undo a choice made with `T` — the very thing it reads like it should do — and the warning for a malformed --theme claimed "using auto" while the saved theme actually won. Return an explicit-ness bool alongside the mode. Only an absent flag and env var defer to the saved value; an explicit auto now re-detects and clears the saved choice, so the next launch detects too. A malformed flag falls through to the env var rather than reading as a request for auto. Reported by Copilot review on B33pBeeps#3.
|
Whenever you have time — no rush. Quick summary in case it helps triage: this is the light-terminal palette. The dark palette is byte-identical to what ships today (there is a test pinning all 19 chrome colors and the tints), so existing users see no change at all. Detection falls back to dark when the terminal swallows the OSC 11 query, which is also current behavior, and The one thing that would benefit from your eye is the light color values themselves — they are my judgment calls, and you know how the board should feel better than I do. Happy to adjust any of them, or to drop the background-picker additions if you would rather keep that list short. Also glad to rebase or split this differently if that makes it easier to review. |
|
Thank you for the PR's, I will review all of them and hopefully have them merged in shortly. I appreciate the bugs found and the methodology of the fixes, and will do a review run of all the pending PR's I currently have. The structure is clear and no other questions! |
|
Thanks for the update, and no rush at all — take them in whatever order suits you. One note that may help sequencing: #3, #4, and #6 are independent, but #5 ($EDITOR) has a small interaction with #4 (undo). I have left the details on #5. Short version: the external-edit path needs three lines to hook into #4's undo seam, and that change cannot compile until #4 is in. So if you merge #5 before #4, it is worth a follow-up; the other order needs nothing. I combined all four locally to check they compose — they do, and that interaction was the only real issue. |
The problem
render.gois deliberately foreground-only — it never sets a backgroundcolor, so the terminal's own background shows through. That's what makes
redthread sit so nicely in a tmux pane, but it also means every color in
the palette is implicitly a claim about what that background looks like.
Every one of them assumed dark:
blue.Ink = {190,220,248})warm white,cool white,bright({255,255,255})Flash,DimText,Footerall brightOn a light terminal the notes wash out to near-unreadable. There was no
light/dark detection anywhere in the codebase, and the background picker
offered ten dark fills and no light ones.
The change
The colors move out of
theme.gointo aPalettevalue in a newpalette.go.darkPalette()reproduces today's colors byte-for-byte —TestDarkPaletteIsUnchangedpins every one of them, so existing users seeno difference at all.
lightPalette()is its counterpart, andApplyTheme(light bool)installs one into the package-level vars the drawsites already read. No call site changed.
The light palette is not a luma flip
Two different rules, because two different jobs:
darkest and most saturated value of its hue, so a "blue" note still
reads as blue.
perceptual distance from a pale background as its dark counterpart
sits from black — hue preserved, chroma eased ~8%.
I got this wrong on the first pass by applying "make it dark enough" to
everything. The result rendered, and passed a naive
luma < 0.5test, butlooked bad: the cork read as noise and the drop shadows punched grey holes
in the page. Dark cork sits 0.16–0.61 from its background; my first light
cork sat 0.39–0.63 — uniformly too far.
theme_test.gonow enforces thetwo rules separately, and
TestLightTextureMirrorsDarkContrastStepsisthe one that would have caught it.
Selecting a theme
Precedence: flag → env → saved → detection.
Auto asks the terminal for its background via
lipgloss.HasDarkBackground()(already an indirect dep), beforetea.NewProgramtakes the screen. When the query goes unanswered — tmuxwithout
allow-passthrough, some ssh sessions, a non-TTY — it reportsdark, which is precisely today's behavior. The override exists for exactly
that case.
Compatibility
themekey. v5 files lack it andload as auto; there's a test for that. The bump is forward-only — an
older binary reading a v6 file just ignores the key.
SelBorderChoicesin the same order, since a board's savedhighlightColoris an index into that slice. A test asserts the namesmatch pairwise.
Also
Five light fills in the background picker (paper, github light, solarized
light, latte, gruvbox light), marked with a faint
·so the two groupsread apart.
T light/darkis in the help panel. README documents all of it.Testing
go test ./...,go test -race ./...,go vet ./...,gofmt -l .allclean. 5 new test files, ~650 lines, covering palette shape, both contrast
rules, flag/env precedence, persistence round-trip, v5 load, and menu
layout at four terminal sizes.
I verified the rendering by dumping
View()to ANSI and converting toHTML, checked at
#e8e8e8and pure white, for the board, both menus, andthe help panel. The screenshots above come from that same dump, so they are
the renderer's real output rather than a mockup. They live on an
assetsbranch of my fork, not in this diff.
One thing I noticed but did not touch
The background picker's panel doesn't blank the cork behind its interior —
specks bleed through the rows. This is pre-existing and identical in the
dark theme (
BackgroundMenuRectcomputes a width the swatch columnoverhangs), so I left it alone rather than mix an unrelated fix into a
color change. Happy to send it separately if you'd like.