From 92f8ac2113f6103894a5ce6c0e632feedaac732a Mon Sep 17 00:00:00 2001 From: "Claude (agent-faces)" Date: Thu, 24 Sep 2026 22:05:24 -0700 Subject: [PATCH] docs(faces): where faces live and what the first two steps settled (improvise-nn4s) context/code/protocol.md gains section 10 and tui.md's colour section now describes face_style; the pointers to axis_fg, minibuf_fg and AxisColor elsewhere follow. Evidence for the new tests. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/code-review/SKILL.md | 3 +- context/code/clients.md | 14 ++--- context/code/core.md | 14 +++-- context/code/protocol.md | 85 ++++++++++++++++++++++++++++- context/code/tui.md | 73 +++++++++++++------------ context/design-principles.md | 4 +- context/test-evidence/core.md | 5 -- context/test-evidence/protocol.md | 24 ++++++++ context/test-evidence/tui.md | 33 +++++++++++ 9 files changed, 196 insertions(+), 59 deletions(-) diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 7b3d29c3..1594097a 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -59,7 +59,8 @@ they mean, rather than testing for the variant **Layering.** `forbidden/2` in `kb.pl` is the authority: `improvise-core` no UI/IO, `improvise-io` no UI/commands, `improvise-command` no ratatui/crossterm. A ratatui type in core is the usual violation — the fix -is a semantic enum in core that the render layer maps (see `AxisColor`). +is a semantic value the render layer maps; for appearance that is a `Face` in +`improvise-protocol`, resolved by `draw.rs::face_style` (ADR 004). **`Option`/`Result`/`Box` at the boundaries.** Wrap at parse/lookup/IO, unwrap early, keep core logic on concrete values. Heap-container plumbing should be diff --git a/context/code/clients.md b/context/code/clients.md index 08f49850..5a359f1f 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1652,7 +1652,7 @@ through the server's own types; with one definition there is nothing left for them to compare, which is what improvise-xks6 was for. **`rejection_message` is spelled here rather than on `Rejection`** for the -reason `AxisColor` is not a ratatui colour: the protocol carries the *fact*, and +reason a `Face` is not a ratatui colour: the protocol carries the *fact*, and each front end words it. The wording tracks ADR 001's force table, because "try again" and "this cannot be forced" are the two different things a user must be able to tell apart — a `RecordGone` has nothing to write to, and offering force @@ -1877,8 +1877,8 @@ Four smaller decisions that would be re-derived wrongly: `improvise_protocol::read_buffer` so the `"search"` special case keeps its one home (gotcha 11); `None` is "no text-entry mode is open", so the page asks nothing about the mode. The colour stays semantic — `MinibufColor` crosses and - the CSS class is the page's last translation, exactly as `draw.rs::minibuf_fg` - is the TUI's (gotcha 26). It is drawn as a surface of its own rather than *in + the CSS class is the page's last translation. (The TUI no longer reads it: it + draws a face per text-entry mode, and improvise-nn4s.4 moves the page there.) It is drawn as a surface of its own rather than *in place of* `#status` as the TUI's bottom bar does, because `#status` also carries the transport's failures and a repaint must not erase one (gotcha 48). Since improvise-n6t8 it shares `#bottom-dock` with the toast, which stacks @@ -1920,7 +1920,7 @@ home for it. The rows themselves are `improvise_protocol::help::pages()`, the same ones `src/ui/help.rs` draws. `RenderHelpRow` carries a `kind` string (`heading`, `keys`, `accent`, …) which is a *semantic* name the page maps to a -CSS class — the `AxisColor → axis_fg` shape again, and the reason the page can +CSS class — the `Face → draw.rs::face_style` shape again, and the reason the page can style a heading without knowing what makes a line a heading. Where the content comes from and what is derived from the keymap is `context/code/protocol.md` §8. @@ -2353,7 +2353,7 @@ truncation. A browser has no column to fit, so the abbreviation buys nothing and reads to a user as a rendering defect — it was spotted in PR #120's screenshots as "Quarter Pag". `render.rs` now asks `plain_label`, which is the other vocabulary already on `Axis`: the decision stays in the shared layer and only -the *choice* of vocabulary is local, exactly the `AxisColor → ratatui::Color` +the *choice* of vocabulary is local, exactly the `Face → ratatui::Style` shape §5 blesses. `short_label` is untouched, because the TUI's width is load-bearing. @@ -2439,8 +2439,8 @@ So it is now a raised bar: fixed, centred over the grid, larger, bordered in the mode's own colour and shadowed. **Every part of that is paint.** The content is `RenderMinibuffer`, built in `render.rs` from `state.view.mode.minibuffer()` — the same semantic value the TUI reads — and the colour is `MinibufColor` -resolved to a concrete colour exactly as `draw.rs::minibuf_fg` resolves it for -the terminal. Nothing here asks which mode is open, which buffer is being typed +resolved to a concrete colour by `style.css`. (The TUI now draws a face per +text-entry mode instead; improvise-nn4s.4 moves the page onto the same faces.) Nothing here asks which mode is open, which buffer is being typed into, or what the prompt says; a second answer to any of those is the disease design-principles §5 is about. The caret is drawn as a CSS `::after` rather than appended to the text for the same reason: the text is the host's, and the page diff --git a/context/code/core.md b/context/code/core.md index ed64269e..a28c36c6 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -357,12 +357,14 @@ browser symptom and silently break drill. `Axis` also owns four presentation vocabularies — `plain_label()`, `glyph()` (which `Display` appends, giving "Row ↕"), `short_label()` (three columns for the tile bar; `None` is "·", **not** a truncation of "None") and `initial()` -(one letter for the view panel; `None` is empty) — plus `color() -> AxisColor`. -`AxisColor` is a *semantic* palette because `improvise-core` must not name a -ratatui type; `draw.rs::axis_fg` does the one mapping, exactly as `minibuf_fg` -does for `MinibufColor` (improvise-rml). The tile bar and the category panel each -carried a copy of the colour table before, with nothing keeping them in -agreement. +(one letter for the view panel; `None` is empty). Its **colour is no longer +here**. It was `color() -> AxisColor` (improvise-rml), a semantic palette that +lived in core only because it was a method on a core type; ADR 004 clause 6 +moved it to `improvise-protocol` as `Face::Axis(axis)`, because a face is UI and +this crate is the one that must have none. The TUI resolves it through +`draw.rs::face_style` (`context/code/tui.md`). The tile bar and the category +panel each carried a copy of the colour table before improvise-rml, with nothing +keeping them in agreement, and they still share one: the face. ### `on_category_added`: a virtual on an axis is a configuration, not a vacancy diff --git a/context/code/protocol.md b/context/code/protocol.md index 81151eb8..0e695880 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1311,8 +1311,8 @@ re-derived from `kind`, so the two ends cannot disagree about what "Measure rather than re-exported, because this crate must not depend on `improvise-io` (the wizard is an io type, gotcha 30) and a client that draws the wizard must still be able to name the step it is on. `WizardFieldKind` is the semantic -half of a colour decision — the terminal mapping lives in `draw.rs`, exactly -as `AxisColor`/`axis_fg` does (gotcha 26). +half of a colour decision: it indexes the `wizard.field.*` faces (§10), and the +terminal mapping is `draw.rs::face_style`. ### `Projection` carries no multi-widget variant @@ -2353,7 +2353,7 @@ deserializes now carries no English at all. `Hint { keys, command, caption }` is the unit: `keys` come off `Keymaps::default_keymaps()`, `command` is the semantic value a localized or non-terminal front end keys off, and `caption` is the default English — exactly -the `AxisColor`/`draw.rs::axis_fg` shape, with the decision shared and only the +the shape faces now have (`Face` → `draw.rs::face_style`, §10), with the decision shared and only the last translation local (`invariant(semantic_palette_at_the_core_boundary)`). ### What that bought, and what it cost @@ -2441,3 +2441,82 @@ the mode, the same shape `col_groups` uses for "no groups". `web/app.js` paints rows it does not author; `src/ui/help.rs` keeps its ratatui styles and lost ~310 lines of content that is now shared. Neither end restates what the keys are. + +## 10. `face.rs` — appearance named by role (ADR 004) + +ADR 004 (`context/adr/004-faces-and-themes.md`) is the decision; this section +is where its first two migration steps landed and what they settled that the +ADR left open (improvise-nn4s.1, improvise-nn4s.2). + +### What is here + +- `Face`: one variant per role. Where a domain enum already exists the face is + **parameterised by it** — `Face::Axis(Axis)`, `Face::WizardField(WizardFieldKind)`, + `Face::ModePanel(Panel)`, `Face::ModeTextEntryOf(TextEntry)`, + `Face::MinibufferOf(TextEntry)` — so adding an arm to that enum adds a face by + construction (clause 2). `Face::name` spells each one as a theme file keys it + (`axis.row`, `mode.text-entry.command`, `minibuffer.category-add`), and those + names are a compatibility surface once a theme can be downloaded (clause 9). +- `Face::parent`: the inheritance, **fixed in the vocabulary rather than stated + by the theme**. A theme that omits a face must still get the face's parent's + values ("a theme written today keeps working when a face is added tomorrow", + clause 9), and that is only true if the chain does not depend on what the + theme says. +- `Colour` (`Default | Named(AnsiSlot) | Indexed(u8) | Rgb`), `Weight`, `Slant`, + and `FaceSpec`, whose six attributes are all `Option`: `None` inherits, and + `Some(Colour::Default)` states "the surface" and stops inheritance. The two + are different answers and the type keeps them apart. +- `Theme { faces }` and `Theme::resolve(Face) -> ResolvedFace`, which walks the + chain once and returns every attribute concrete. **No front end walks the + chain** (clause 3). `resolve` takes a face and nothing else. +- `Theme::shipped()`: today's terminal rendering, palette-relative throughout + (clause 4). `the_shipped_theme_states_no_absolute_colour` holds that line. +- `Face::of_mode(&AppMode)` and `Face::of_minibuffer(&AppMode)`, both + exhaustive over `AppMode`, so a new mode fails the build here as it used to + in `draw.rs::mode_style` (improvise-zx8t). + +### The JSON shape + +Face names are the keys and a colour is spelled for a person: `"default"`, a +slot name (`"green"`, `"bright-black"`), a bare number for an index (`235`), +`"#rrggbb"` for `Rgb`. A misspelt face or attribute is **refused**, not +ignored (`deny_unknown_fields`, and `Face`'s `Deserialize` rejects an unknown +name): a file the user edits by hand should say where it is wrong rather than +silently draw the default. The faces map is a `BTreeMap` keyed on `Face`'s +derived order, so a re-download after an edit is a reviewable diff (clause 9). +The protocol crate derives and the front ends encode, so `serde_json` stays a +dev-dependency here. + +### Choices the ADR did not make, made here + +- **`minibuffer.*` inherits a `minibuffer` base face, not `mode.text-entry`.** + The ADR says the minibuffer faces "inherit `mode.text-entry` directly". But + the mode indicator and the minibuffer paint opposite halves of the cell — the + indicator is Black ink on an accent background, the minibuffer is accent ink + on `Indexed(235)` — so inheriting one from the other makes every attribute an + override and the chain decorative. The `minibuffer` face carries only what + the strip has in common (its background and bold), and encodes nothing about + green and yellow, which is the property the ADR's sentence was protecting. +- **`mode.tile-select` hangs directly off `mode`.** The ADR's decomposition + covers the thirteen DarkGray modes and the text-entry chain, and does not + place `TileSelect`. +- **The mode faces still state `Named(Black)` ink.** Clause 5 wants the twelve + `fg(Black).bg(accent)` sites to become `reverse` over the accent, but that is + not a no-op on a `TestBackend` buffer, and clause 8 names only one exception. + So it waits for improvise-nn4s.6 and a ruling. +- **No slot tables yet.** `Theme` has no `slots`/`dark`/`default_scheme`: they + are the browser's substitute for a terminal palette (clause 4), and the first + consumer is improvise-nn4s.5. Adding them then is a field with a serde + default, not a format break. +- **Only the faces some call site draws with exist.** The ADR's full vocabulary + (`emphasis`, `error`, `selection`, `grid.*`, `panel.*`) arrives with the + call sites that use it, because a face with no caller is a published name + with no evidence behind it. + +### The category-add exception + +`minibuffer.category-add` resolves to green in the shipped theme, where the old +literal was yellow: the user ruled the split an accident (ADR 004 Q1), and it +is clause 8's one declared exception. `AppMode::category_add` now carries +`MinibufColor::Green` too, so the browser, which still reads the wire colour +until improvise-nn4s.4, agrees with the TUI rather than drifting from it. diff --git a/context/code/tui.md b/context/code/tui.md index eeae4517..eff93fb6 100644 --- a/context/code/tui.md +++ b/context/code/tui.md @@ -104,46 +104,49 @@ appear. Three rules: - `SHIFT`/`CONTROL`/`ALT` survive; every other modifier bit (`SUPER`, `HYPER`, `META`) is dropped. -### Semantic-to-terminal colour, and the one that is wrong +### `face_style` — the one place a face becomes a terminal style -This file holds the mapping design-principles §5 names as the shape everything -else should imitate: **the decision lives in the shared layer as a semantic -enum, and only the last translation is local.** +ADR 004 replaced the four per-family maps this section used to describe +(`axis_fg`, `minibuf_fg`, `field_kind_fg`, `mode_style`) with one function: +`face_style(Face) -> Style` resolves the face against the shipped theme +(`Theme::resolve`, in `improvise-protocol`) and translates the result. That is +design-principles §5's shape with nothing left over: **the decision is shared, +and only the last translation is local** — `Colour` → `ratatui::Color` and the +six attributes → `Modifier`. The chain is never walked here (ADR 004 clause 3). +What the vocabulary is and why is `context/code/protocol.md` §10. -- `axis_fg(AxisColor) -> Color` — `Axis::color` is the single place each axis's - accent is decided; this is the single place axis colouring is *rendered* - (improvise-rml, gotcha 26). Before it, the tile bar and the category panel - each carried their own copy of the colour table with nothing keeping them in - agreement. -- `minibuf_fg(MinibufColor) -> Color` — the same shape for the minibuffer. -- `src/ui/import_wizard_ui.rs::field_kind_fg(WizardFieldKind) -> Color` — the - same shape again. `improvise-protocol` must not name a ratatui `Color`. +Three things to know. -`mode_style(&AppMode) -> Style` stays here for the same reason (`Style` and -`Color` are ratatui types and `improvise-command`, where `AppMode` lives, -carries no rendering-framework dependency), and since `improvise-zx8t` it is -**exhaustive**: the thirteen variants that take the default chrome are spelled -out rather than swept up by a `_ =>` wildcard, so adding an `AppMode` fails the -build here until someone chooses its colour. The arithmetic is the check a -prose count does not get: 13 + `Editing`/`RecordsEditing` + `CommandMode` + -`TileSelect` = 17, which is `AppMode`'s variant count. It read *fourteen* until -2026-09-22, and nothing could have caught that — a count written in prose is -checked by no one. +- **`Colour::Default` sets nothing.** It maps to "no `fg`/`bg` on the `Style`", + so the terminal's own colour shows through. That is also why a face with no + stated colour produces `Style::default()` and not a `Reset`. +- **The slot names follow ANSI, not ratatui.** ANSI slot 7 is `White`, which + ratatui calls `Gray`; slot 8 is `BrightBlack`, ratatui's `DarkGray`; slot 15 + is `BrightWhite`, ratatui's `White`. `terminal_colour` is the one place that + translation is written. +- **The shipped theme is resolved once**, into a `OnceLock`, because there is no + theme picker yet (ADR 004 "Deferred"). When one arrives the theme becomes the + TUI's own state rather than a static, and a theme is still never on `App`'s + `ViewState` (clause 7). -It is still not quite the same shape as `axis_fg` and its two siblings, and the -difference is deliberate rather than pending. Those three map a *semantic enum* -the shared layer owns; `mode_style` matches `AppMode` itself, because there is -no `ModeColor`. `improvise-zx8t` offered making one, and it was declined here -for two reasons: the colour is the only thing at stake, and the ticket says to -decide it together with `improvise-k099`, which asks the same question of -`hint_text`/`label` and is not in this unit. Exhaustiveness is what the -repo map's "the compiler names every one you must fill in" promise needed; a -semantic enum is a separate, larger answer and is still open. +**The mode indicator is still exhaustive over `AppMode`**, now in +`Face::of_mode` rather than here: adding a variant fails the build there +(improvise-zx8t's guard, moved with the table). The thirteen DarkGray variants +are no longer one arm: they are the five faces ADR 004 derives +(`mode.resting`, `mode.text-entry`, `mode.panel.*`, `mode.overlay`, +`mode.transient`), which all still inherit the same colours from `mode`. -**The guard is the compiler, not a test.** Nothing asserts the colours, and a -test enumerating the variants would not fail when a fifteenth is added — only -the missing match arm does. Verified by deleting the `AppMode::Help` arm: -`error[E0004]: non-exhaustive patterns: `&AppMode::Help` not covered`. +**No-op evidence.** `face_tests::the_shipped_theme_reproduces_every_migrated_literal` +holds the old literals as a table and asserts the shipped theme draws each one, +which is clause 8's property. The one deliberate difference, the category-add +prompt turning green, is asserted by +`the_category_add_prompt_draws_as_the_item_add_prompt_does` rather than being +left out of the table silently. + +**Still literal**: every other `Color::` in `src/` (the selection sites, the +panels' accents, the grid's headers, help, the welcome popup). They move as +their files are touched (improvise-nn4s.3, improvise-nn4s.6). Do not touch +`src/ui/grid.rs` for this while improvise-felo.6 is in flight. ### The minibuffer shares the bottom row with the status line diff --git a/context/design-principles.md b/context/design-principles.md index e9036e59..3e35f6a2 100644 --- a/context/design-principles.md +++ b/context/design-principles.md @@ -269,8 +269,8 @@ why `doctrine(mirror_the_cmd_line_for_line)` is a rule about *resolvers* — the narrow, unavoidable residue — and never a licence to restate a table. **What legitimately stays per-framework.** Painting, event wiring, and the -mapping from a semantic value to a framework type: `AxisColor → ratatui::Color` -in `draw.rs::axis_fg`, `MinibufColor → minibuf_fg`. That mapping is the shape +mapping from a semantic value to a framework type: a `Face`, resolved by the +shared `Theme`, becomes a `ratatui::Style` in `draw.rs::face_style` (ADR 004). That mapping is the shape to imitate — the *decision* is in the shared layer as a semantic enum, and only the last translation is local. Anything larger than a lookup table is a candidate for the store. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index fb4b2e6e..19a208d1 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -710,11 +710,6 @@ Applying `next` four times is the identity from every starting point and no navigable axis is a fixed point, so no variant is stranded or skipped. Together with the entry above this pins the rotation without enumerating it twice. -### every_axis_gets_its_own_color - -Distinct axes must be distinguishable at a glance; `color()` is the only reason -`AxisColor` exists, so a collision defeats it. - ### a_routing_formula_over_a_group_sums_the_members_answers improvise-4zl7, and the hand-computed half of its evidence. Every number in diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 8b1ead78..c44cd45b 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1259,3 +1259,27 @@ the Enter commit. - **Rival** (the commit naming `state.view.active_view`, the view at `Enter`), run across the whole workspace: fails **only** this test, with `["Later"]` against `["Other"]`. + +### resolution_takes_each_attribute_from_the_nearest_face_that_states_it + +ADR 004 clause 3: inheritance resolves once, in shared code. The fixture states +a different attribute at each of three levels — `default` (foreground White, +italic), `mode` (foreground Black, background Blue), and +`mode.text-entry.save` (background `Colour::Default`, underline) — so the one +resolved face has to take its foreground from the middle, its slant from the +root and its background from itself. `Some(Colour::Default)` is the case that +matters: it is a stated value and must not fall through to `mode`'s Blue, which +the sibling `mode.text-entry.export` (stating nothing) does inherit. + +**Injected fault**: fold the chain so the parent wins. This test fails, and so +does the TUI's `the_shipped_theme_reproduces_every_migrated_literal`, because +`mode.text-entry.edit` then draws its parent's DarkGray. + +### every_axis_gets_its_own_accent_in_the_shipped_theme + +Moved from `improvise-core`'s `every_axis_gets_its_own_color` when `AxisColor` +left core (ADR 004 clause 6). Distinct axes must be distinguishable at a +glance, and the axis faces exist for exactly that, so two axes resolving alike +defeats them. It now asks the shipped theme rather than an enum's arms, which +is the stronger question: two distinct `AxisColor` arms could always have been +mapped to one terminal colour. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 69cf6582..114c74be 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -1429,3 +1429,36 @@ to the list's header row, which no item can cover. This test fills three rows with a three-line item and a second item after it. It asserts the first item is drawn whole and the header reads `1–1 of 2`. The rival **the header claims every item shown** fails it. + +### the_category_add_prompt_draws_as_the_item_add_prompt_does + +ADR 004 clause 8's one declared exception, decided by the user (Q1 on PR #122: +the green/yellow split between item-add and category-add is an accident). It +draws both prompts through the real `draw_bottom_bar` into a `TestBackend` and +compares the style of the prompt's first cell, after checking that cell really +holds the prompt's `n`. + +**Failed first**, at `gh/main` `b639266b` before any face existed: `left: +...yellow().bg(Indexed(235)).bold()`, `right: ...green()...` — the category-add +prompt was yellow. **Injected fault** after the migration: set the shipped +theme's `minibuffer.category-add` back to `Named(Yellow)`; this test fails and +nothing else in the TUI does, which is the point of naming the exception in a +test of its own rather than leaving it out of the no-op table. + +### the_shipped_theme_reproduces_every_migrated_literal + +ADR 004 clause 8's property: migrating a call site onto a face is a no-op. The +oracle is the literal tables as they stood before the migration — axis +accents, wizard field kinds, `mode_style`'s exhaustive match (kept verbatim as +`mode_bg_before_faces`) and the minibuffer's accent-on-`Indexed(235)`-bold — +and every one is compared with `face_style` of its face. Category-add is left +out on purpose (the entry above). It was first written against the old +functions themselves and passed, then the functions were deleted and their +tables moved into the test. + +**Injected faults**, each failing here by the case name in the message: +`axis.column` changed from Blue to Cyan (fails at `Column`); `face_style` made +to add `BOLD` for `Weight::Normal` instead of `Bold`; `Theme::resolve` folded so +that the parent wins over the child (fails at `Editing`, which draws DarkGray +instead of its own Green); and `Face::of_mode(TileSelect)` returning +`mode.resting` (also caught by `the_status_line_is_drawn_in_the_mode_face`).