From abff5623609131d6bcf938b41524e6ba07e0f074 Mon Sep 17 00:00:00 2001 From: "Claude (agent-faces)" Date: Thu, 24 Sep 2026 21:54:11 -0700 Subject: [PATCH 1/5] feat(faces): Face, Theme, resolve and the shipped theme in improvise-protocol (improvise-nn4s.1) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-core/src/view/axis.rs | 2 +- crates/improvise-protocol/src/face.rs | 672 ++++++++++++++++++++++++ crates/improvise-protocol/src/lib.rs | 4 +- crates/improvise-protocol/src/render.rs | 2 +- 4 files changed, 677 insertions(+), 3 deletions(-) create mode 100644 crates/improvise-protocol/src/face.rs diff --git a/crates/improvise-core/src/view/axis.rs b/crates/improvise-core/src/view/axis.rs index ae55a145..d81295f6 100644 --- a/crates/improvise-core/src/view/axis.rs +++ b/crates/improvise-core/src/view/axis.rs @@ -1,6 +1,6 @@ use serde::{Deserialize, Serialize}; -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum Axis { Row, diff --git a/crates/improvise-protocol/src/face.rs b/crates/improvise-protocol/src/face.rs new file mode 100644 index 00000000..83ccc6d3 --- /dev/null +++ b/crates/improvise-protocol/src/face.rs @@ -0,0 +1,672 @@ +//! Faces: appearance named by role, and a theme that binds roles to values +//! (ADR 004). The reasoning is `context/code/protocol.md` §10. + +use std::collections::BTreeMap; +use std::fmt; + +use serde::de::{self, Deserializer, Visitor}; +use serde::{Deserialize, Serialize, Serializer}; + +use crate::render::WizardFieldKind; +use crate::{AppMode, Panel}; +use improvise_core::view::Axis; + +/// One of the sixteen palette slots: "whatever THIS display calls that slot". +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum AnsiSlot { + Black, + Red, + Green, + Yellow, + Blue, + Magenta, + Cyan, + White, + BrightBlack, + BrightRed, + BrightGreen, + BrightYellow, + BrightBlue, + BrightMagenta, + BrightCyan, + BrightWhite, +} + +impl AnsiSlot { + pub const ALL: [AnsiSlot; 16] = [ + AnsiSlot::Black, + AnsiSlot::Red, + AnsiSlot::Green, + AnsiSlot::Yellow, + AnsiSlot::Blue, + AnsiSlot::Magenta, + AnsiSlot::Cyan, + AnsiSlot::White, + AnsiSlot::BrightBlack, + AnsiSlot::BrightRed, + AnsiSlot::BrightGreen, + AnsiSlot::BrightYellow, + AnsiSlot::BrightBlue, + AnsiSlot::BrightMagenta, + AnsiSlot::BrightCyan, + AnsiSlot::BrightWhite, + ]; + + /// The slot's number in the 256-colour palette, 0–15. + pub fn index(self) -> u8 { + self as u8 + } + + /// The spelling a theme file uses. + pub fn name(self) -> &'static str { + match self { + AnsiSlot::Black => "black", + AnsiSlot::Red => "red", + AnsiSlot::Green => "green", + AnsiSlot::Yellow => "yellow", + AnsiSlot::Blue => "blue", + AnsiSlot::Magenta => "magenta", + AnsiSlot::Cyan => "cyan", + AnsiSlot::White => "white", + AnsiSlot::BrightBlack => "bright-black", + AnsiSlot::BrightRed => "bright-red", + AnsiSlot::BrightGreen => "bright-green", + AnsiSlot::BrightYellow => "bright-yellow", + AnsiSlot::BrightBlue => "bright-blue", + AnsiSlot::BrightMagenta => "bright-magenta", + AnsiSlot::BrightCyan => "bright-cyan", + AnsiSlot::BrightWhite => "bright-white", + } + } + + pub fn from_name(name: &str) -> Option { + Self::ALL.into_iter().find(|slot| slot.name() == name) + } +} + +/// A colour attribute. `Default`, `Named` and `Indexed` are palette-relative; +/// `Rgb` is absolute, and the shipped theme never uses it (ADR 004 clause 4). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Colour { + Default, + Named(AnsiSlot), + Indexed(u8), + Rgb(u8, u8, u8), +} + +impl Colour { + /// True for the one arm that opts a viewer out of their own palette. + pub fn is_absolute(self) -> bool { + match self { + Colour::Rgb(..) => true, + Colour::Default | Colour::Named(_) | Colour::Indexed(_) => false, + } + } +} + +impl Serialize for Colour { + fn serialize(&self, s: S) -> Result { + match *self { + Colour::Default => s.serialize_str("default"), + Colour::Named(slot) => s.serialize_str(slot.name()), + Colour::Indexed(n) => s.serialize_u8(n), + Colour::Rgb(r, g, b) => s.serialize_str(&format!("#{r:02x}{g:02x}{b:02x}")), + } + } +} + +struct ColourVisitor; + +impl Visitor<'_> for ColourVisitor { + type Value = Colour; + + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("\"default\", a slot name such as \"green\", 0-255, or \"#rrggbb\"") + } + + fn visit_u64(self, n: u64) -> Result { + u8::try_from(n) + .map(Colour::Indexed) + .map_err(|_| E::custom(format!("colour index {n} is outside 0-255"))) + } + + fn visit_str(self, s: &str) -> Result { + parse_colour(s).ok_or_else(|| E::invalid_value(de::Unexpected::Str(s), &self)) + } +} + +fn parse_colour(s: &str) -> Option { + match s.strip_prefix('#') { + Some(hex) if hex.len() == 6 && hex.bytes().all(|b| b.is_ascii_hexdigit()) => { + let byte = |i: usize| u8::from_str_radix(&hex[i..i + 2], 16).ok(); + Some(Colour::Rgb(byte(0)?, byte(2)?, byte(4)?)) + } + Some(_) => None, + None if s == "default" => Some(Colour::Default), + None => AnsiSlot::from_name(s).map(Colour::Named), + } +} + +impl<'de> Deserialize<'de> for Colour { + fn deserialize>(d: D) -> Result { + d.deserialize_any(ColourVisitor) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum Weight { + Normal, + Bold, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum Slant { + Normal, + Italic, +} + +/// The text-entry modes, one per `MinibufferConfig`-carrying `AppMode` arm +/// except that cell editing is one role over two layouts. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum TextEntry { + Edit, + Formula, + Command, + CategoryAdd, + ItemAdd, + Export, + Save, +} + +impl TextEntry { + pub const ALL: [TextEntry; 7] = [ + TextEntry::Edit, + TextEntry::Formula, + TextEntry::Command, + TextEntry::CategoryAdd, + TextEntry::ItemAdd, + TextEntry::Export, + TextEntry::Save, + ]; + + /// The text-entry role of `mode`, or `None` for a mode that takes no text. + pub fn of(mode: &AppMode) -> Option { + match mode { + AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => Some(TextEntry::Edit), + AppMode::FormulaEdit { .. } => Some(TextEntry::Formula), + AppMode::CommandMode { .. } => Some(TextEntry::Command), + AppMode::CategoryAdd { .. } => Some(TextEntry::CategoryAdd), + AppMode::ItemAdd { .. } => Some(TextEntry::ItemAdd), + AppMode::ExportPrompt { .. } => Some(TextEntry::Export), + AppMode::SavePrompt { .. } => Some(TextEntry::Save), + AppMode::Normal + | AppMode::RecordsNormal + | AppMode::FormulaPanel + | AppMode::CategoryPanel + | AppMode::ViewPanel + | AppMode::TileSelect + | AppMode::ImportWizard + | AppMode::Help + | AppMode::Quit => None, + } + } + + fn name(self) -> &'static str { + match self { + TextEntry::Edit => "edit", + TextEntry::Formula => "formula", + TextEntry::Command => "command", + TextEntry::CategoryAdd => "category-add", + TextEntry::ItemAdd => "item-add", + TextEntry::Export => "export", + TextEntry::Save => "save", + } + } +} + +const AXES: [Axis; 5] = [Axis::Row, Axis::Column, Axis::Page, Axis::Filter, Axis::None]; + +const FIELD_KINDS: [WizardFieldKind; 4] = [ + WizardFieldKind::Category, + WizardFieldKind::Measure, + WizardFieldKind::TimeCategory, + WizardFieldKind::Label, +]; + +fn axis_name(axis: Axis) -> &'static str { + match axis { + Axis::Row => "row", + Axis::Column => "column", + Axis::Page => "page", + Axis::Filter => "filter", + Axis::None => "none", + } +} + +fn field_kind_name(kind: WizardFieldKind) -> &'static str { + match kind { + WizardFieldKind::Category => "category", + WizardFieldKind::Measure => "measure", + WizardFieldKind::TimeCategory => "time", + WizardFieldKind::Label => "label", + } +} + +/// A role something is drawn in. Its `name` is a compatibility surface once a +/// theme file carries it (ADR 004 clause 9), so never rename one. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum Face { + Default, + Dim, + Axis(Axis), + WizardField(WizardFieldKind), + Mode, + ModeResting, + ModeTextEntry, + ModeTextEntryOf(TextEntry), + ModePanel(Panel), + ModeOverlay, + ModeTransient, + ModeTileSelect, + Minibuffer, + MinibufferOf(TextEntry), +} + +impl Face { + /// Every face, parents before children. + pub fn all() -> Vec { + let mut out = vec![Face::Default, Face::Dim]; + out.extend(AXES.map(Face::Axis)); + out.extend(FIELD_KINDS.map(Face::WizardField)); + out.extend([Face::Mode, Face::ModeResting, Face::ModeTextEntry]); + out.extend(TextEntry::ALL.map(Face::ModeTextEntryOf)); + out.extend(Panel::ALL.map(Face::ModePanel)); + out.extend([Face::ModeOverlay, Face::ModeTransient, Face::ModeTileSelect]); + out.push(Face::Minibuffer); + out.extend(TextEntry::ALL.map(Face::MinibufferOf)); + out + } + + /// The face this one inherits every attribute it does not state from. + pub fn parent(self) -> Option { + match self { + Face::Default => None, + Face::Dim | Face::Mode | Face::Minibuffer => Some(Face::Default), + Face::Axis(Axis::None) | Face::WizardField(WizardFieldKind::Label) => Some(Face::Dim), + Face::Axis(_) | Face::WizardField(_) => Some(Face::Default), + Face::ModeResting + | Face::ModeTextEntry + | Face::ModePanel(_) + | Face::ModeOverlay + | Face::ModeTransient + | Face::ModeTileSelect => Some(Face::Mode), + Face::ModeTextEntryOf(_) => Some(Face::ModeTextEntry), + Face::MinibufferOf(_) => Some(Face::Minibuffer), + } + } + + /// The spelling a theme file keys this face by. + pub fn name(self) -> String { + match self { + Face::Default => "default".into(), + Face::Dim => "dim".into(), + Face::Axis(axis) => format!("axis.{}", axis_name(axis)), + Face::WizardField(kind) => format!("wizard.field.{}", field_kind_name(kind)), + Face::Mode => "mode".into(), + Face::ModeResting => "mode.resting".into(), + Face::ModeTextEntry => "mode.text-entry".into(), + Face::ModeTextEntryOf(entry) => format!("mode.text-entry.{}", entry.name()), + Face::ModePanel(panel) => format!("mode.panel.{}", panel.name()), + Face::ModeOverlay => "mode.overlay".into(), + Face::ModeTransient => "mode.transient".into(), + Face::ModeTileSelect => "mode.tile-select".into(), + Face::Minibuffer => "minibuffer".into(), + Face::MinibufferOf(entry) => format!("minibuffer.{}", entry.name()), + } + } + + pub fn from_name(name: &str) -> Option { + Face::all().into_iter().find(|face| face.name() == name) + } + + /// The face the mode indicator is drawn in. + pub fn of_mode(mode: &AppMode) -> Face { + match mode { + AppMode::Normal | AppMode::RecordsNormal => Face::ModeResting, + AppMode::FormulaPanel => Face::ModePanel(Panel::Formula), + AppMode::CategoryPanel => Face::ModePanel(Panel::Category), + AppMode::ViewPanel => Face::ModePanel(Panel::View), + AppMode::ImportWizard | AppMode::Help => Face::ModeOverlay, + AppMode::Quit => Face::ModeTransient, + AppMode::TileSelect => Face::ModeTileSelect, + AppMode::Editing { .. } + | AppMode::RecordsEditing { .. } + | AppMode::FormulaEdit { .. } + | AppMode::CommandMode { .. } + | AppMode::CategoryAdd { .. } + | AppMode::ItemAdd { .. } + | AppMode::ExportPrompt { .. } + | AppMode::SavePrompt { .. } => TextEntry::of(mode) + .map(Face::ModeTextEntryOf) + .unwrap_or(Face::ModeTextEntry), + } + } + + /// The face a text-entry mode's minibuffer is drawn in. + pub fn of_minibuffer(mode: &AppMode) -> Option { + TextEntry::of(mode).map(Face::MinibufferOf) + } +} + +impl Serialize for Face { + fn serialize(&self, s: S) -> Result { + s.serialize_str(&self.name()) + } +} + +impl<'de> Deserialize<'de> for Face { + fn deserialize>(d: D) -> Result { + let name = String::deserialize(d)?; + Face::from_name(&name).ok_or_else(|| de::Error::custom(format!("unknown face {name:?}"))) + } +} + +/// What a theme states about one face. `None` inherits from the parent; +/// `Some(Colour::Default)` states "the surface" and does not inherit. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct FaceSpec { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub foreground: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub background: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub weight: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub slant: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub underline: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reverse: Option, +} + +impl FaceSpec { + fn or(self, parent: FaceSpec) -> FaceSpec { + FaceSpec { + foreground: self.foreground.or(parent.foreground), + background: self.background.or(parent.background), + weight: self.weight.or(parent.weight), + slant: self.slant.or(parent.slant), + underline: self.underline.or(parent.underline), + reverse: self.reverse.or(parent.reverse), + } + } + + fn fg(colour: Colour) -> FaceSpec { + FaceSpec { + foreground: Some(colour), + ..FaceSpec::default() + } + } + + fn bg(colour: Colour) -> FaceSpec { + FaceSpec { + background: Some(colour), + ..FaceSpec::default() + } + } +} + +/// A face with every attribute concrete: what a front end translates. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ResolvedFace { + pub foreground: Colour, + pub background: Colour, + pub weight: Weight, + pub slant: Slant, + pub underline: bool, + pub reverse: bool, +} + +/// Faces bound to values. Client-local: never a `Command`, never in +/// `ViewState`, never projected, never in `.improv` (ADR 004 clause 7). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Theme { + pub faces: BTreeMap, +} + +impl Theme { + /// The theme improvise ships: today's terminal rendering, palette-relative + /// throughout, so the user's terminal theme still applies. + pub fn shipped() -> Theme { + use AnsiSlot::*; + let named = Colour::Named; + let faces = [ + (Face::Dim, FaceSpec::fg(named(BrightBlack))), + (Face::Axis(Axis::Row), FaceSpec::fg(named(Green))), + (Face::Axis(Axis::Column), FaceSpec::fg(named(Blue))), + (Face::Axis(Axis::Page), FaceSpec::fg(named(Magenta))), + (Face::Axis(Axis::Filter), FaceSpec::fg(named(Yellow))), + ( + Face::WizardField(WizardFieldKind::Category), + FaceSpec::fg(named(Green)), + ), + ( + Face::WizardField(WizardFieldKind::Measure), + FaceSpec::fg(named(Cyan)), + ), + ( + Face::WizardField(WizardFieldKind::TimeCategory), + FaceSpec::fg(named(Magenta)), + ), + ( + Face::Mode, + FaceSpec { + foreground: Some(named(Black)), + background: Some(named(BrightBlack)), + ..FaceSpec::default() + }, + ), + ( + Face::ModeTextEntryOf(TextEntry::Edit), + FaceSpec::bg(named(Green)), + ), + ( + Face::ModeTextEntryOf(TextEntry::Command), + FaceSpec::bg(named(Yellow)), + ), + (Face::ModeTileSelect, FaceSpec::bg(named(Magenta))), + ( + Face::Minibuffer, + FaceSpec { + background: Some(Colour::Indexed(235)), + weight: Some(Weight::Bold), + ..FaceSpec::default() + }, + ), + ( + Face::MinibufferOf(TextEntry::Edit), + FaceSpec::fg(named(Green)), + ), + ( + Face::MinibufferOf(TextEntry::ItemAdd), + FaceSpec::fg(named(Green)), + ), + ( + Face::MinibufferOf(TextEntry::CategoryAdd), + FaceSpec::fg(named(Green)), + ), + ( + Face::MinibufferOf(TextEntry::Formula), + FaceSpec::fg(named(Cyan)), + ), + ( + Face::MinibufferOf(TextEntry::Command), + FaceSpec::fg(named(Yellow)), + ), + ( + Face::MinibufferOf(TextEntry::Export), + FaceSpec::fg(named(Yellow)), + ), + ( + Face::MinibufferOf(TextEntry::Save), + FaceSpec::fg(named(Yellow)), + ), + ]; + Theme { + faces: faces.into_iter().collect(), + } + } + + /// Walk `face`'s inheritance chain once, so no front end ever does. + pub fn resolve(&self, face: Face) -> ResolvedFace { + let stated = std::iter::successors(Some(face), |f| f.parent()) + .filter_map(|f| self.faces.get(&f).copied()) + .fold(FaceSpec::default(), FaceSpec::or); + ResolvedFace { + foreground: stated.foreground.unwrap_or(Colour::Default), + background: stated.background.unwrap_or(Colour::Default), + weight: stated.weight.unwrap_or(Weight::Normal), + slant: stated.slant.unwrap_or(Slant::Normal), + underline: stated.underline.unwrap_or(false), + reverse: stated.reverse.unwrap_or(false), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_face_name_is_distinct_and_reads_back_as_that_face() { + let all = Face::all(); + for face in &all { + assert_eq!(Face::from_name(&face.name()), Some(*face), "{face:?}"); + } + let mut names: Vec = all.iter().map(|f| f.name()).collect(); + names.sort(); + names.dedup(); + assert_eq!(names.len(), all.len(), "two faces share a name"); + } + + #[test] + fn every_parent_is_a_face_listed_before_its_child() { + let all = Face::all(); + for (i, face) in all.iter().enumerate() { + if let Some(parent) = face.parent() { + let at = all.iter().position(|f| *f == parent); + assert!(at.is_some_and(|p| p < i), "{face:?}'s parent {parent:?}"); + } + } + } + + /// ADR 004 clause 4: the shipped theme keeps the user's terminal palette. + #[test] + fn the_shipped_theme_states_no_absolute_colour() { + let theme = Theme::shipped(); + for face in Face::all() { + let resolved = theme.resolve(face); + assert!(!resolved.foreground.is_absolute(), "{}", face.name()); + assert!(!resolved.background.is_absolute(), "{}", face.name()); + } + } + + /// A child overrides what it states and inherits what it does not, from + /// the nearest ancestor that states it — `context/test-evidence/protocol.md`. + #[test] + fn resolution_takes_each_attribute_from_the_nearest_face_that_states_it() { + let named = Colour::Named; + let theme = Theme { + faces: [ + ( + Face::Default, + FaceSpec { + foreground: Some(named(AnsiSlot::White)), + slant: Some(Slant::Italic), + ..FaceSpec::default() + }, + ), + ( + Face::Mode, + FaceSpec { + foreground: Some(named(AnsiSlot::Black)), + background: Some(named(AnsiSlot::Blue)), + ..FaceSpec::default() + }, + ), + ( + Face::ModeTextEntryOf(TextEntry::Save), + FaceSpec { + background: Some(Colour::Default), + underline: Some(true), + ..FaceSpec::default() + }, + ), + ] + .into_iter() + .collect(), + }; + let got = theme.resolve(Face::ModeTextEntryOf(TextEntry::Save)); + assert_eq!( + got, + ResolvedFace { + foreground: named(AnsiSlot::Black), + background: Colour::Default, + weight: Weight::Normal, + slant: Slant::Italic, + underline: true, + reverse: false, + } + ); + let sibling = theme.resolve(Face::ModeTextEntryOf(TextEntry::Export)); + assert_eq!(sibling.background, named(AnsiSlot::Blue)); + } + + #[test] + fn a_theme_survives_its_own_json() { + let theme = Theme::shipped(); + let text = serde_json::to_string_pretty(&theme).unwrap(); + assert!(text.contains("\"minibuffer.category-add\""), "{text}"); + assert_eq!(serde_json::from_str::(&text).unwrap(), theme); + } + + #[test] + fn a_colour_is_spelled_for_a_person_editing_the_file() { + let cases = [ + (Colour::Default, "\"default\""), + (Colour::Named(AnsiSlot::BrightBlack), "\"bright-black\""), + (Colour::Indexed(235), "235"), + (Colour::Rgb(0x3f, 0x7a, 0x4d), "\"#3f7a4d\""), + ]; + for (colour, json) in cases { + assert_eq!(serde_json::to_string(&colour).unwrap(), json); + assert_eq!(serde_json::from_str::(json).unwrap(), colour); + } + for bad in ["\"grene\"", "\"#3f7a4\"", "256", "\"#gggggg\""] { + assert!(serde_json::from_str::(bad).is_err(), "{bad}"); + } + } + + #[test] + fn a_misspelt_face_or_attribute_is_refused_rather_than_ignored() { + let face = r#"{"faces":{"axis.rwo":{"foreground":"green"}}}"#; + assert!(serde_json::from_str::(face).is_err()); + let attr = r#"{"faces":{"axis.row":{"forground":"green"}}}"#; + assert!(serde_json::from_str::(attr).is_err()); + } + + #[test] + fn every_text_entry_mode_has_a_text_entry_role_and_no_other_mode_does() { + for mode in AppMode::all() { + assert_eq!( + TextEntry::of(&mode).is_some(), + mode.minibuffer().is_some(), + "{mode:?}" + ); + } + } +} diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 6e535f44..e763c3cf 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -6,6 +6,7 @@ use std::collections::HashMap; pub mod basis; pub mod client; +pub mod face; pub mod frame; pub mod help; pub mod key; @@ -23,6 +24,7 @@ mod vocabulary_is_composed; pub use basis::{Basis, BasisCache, Rejection}; pub use client::{ClientState, Sent}; +pub use face::{Face, Theme}; pub use frame::{ClientFrame, ServerFrame}; pub use help::{HelpLine, Hint, hint_line, hints, mode_label}; pub use key::{KeyCode, KeyEvent, KeyModifiers}; @@ -103,7 +105,7 @@ pub struct MinibufferConfig { /// One of the three side panels. Paired with [`AppMode::panel`], which names /// the panel a mode is focused in. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] pub enum Panel { Formula, Category, diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 8c8ba510..30ba0e6f 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -927,7 +927,7 @@ pub enum WizardStepKind { /// What kind of thing a proposed field is. The semantic half of a colour /// decision; the terminal mapping lives in `draw.rs` (gotcha 26). -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] pub enum WizardFieldKind { Category, Measure, From 12d976b432570dd78af7e59ac9ec80203e261427 Mon Sep 17 00:00:00 2001 From: "Claude (agent-faces)" Date: Thu, 24 Sep 2026 21:58:01 -0700 Subject: [PATCH 2/5] feat(faces): the TUI draws axis, wizard-field, mode and minibuffer colours through face_style (improvise-nn4s.2) axis_fg, minibuf_fg, field_kind_fg and mode_style are gone, and AxisColor leaves improvise-core. The category-add prompt turns green, ADR 004 clause 8's one declared exception. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-core/src/view/axis.rs | 33 --- crates/improvise-core/src/view/mod.rs | 2 +- crates/improvise-protocol/src/face.rs | 12 ++ crates/improvise-protocol/src/lib.rs | 2 +- crates/improvise-wasm-client/src/render.rs | 4 +- src/draw.rs | 224 ++++++++++++++++----- src/ui/category_panel.rs | 9 +- src/ui/import_wizard_ui.rs | 18 +- src/ui/tile_bar.rs | 9 +- 9 files changed, 206 insertions(+), 107 deletions(-) diff --git a/crates/improvise-core/src/view/axis.rs b/crates/improvise-core/src/view/axis.rs index d81295f6..f43f0b22 100644 --- a/crates/improvise-core/src/view/axis.rs +++ b/crates/improvise-core/src/view/axis.rs @@ -12,17 +12,6 @@ pub enum Axis { None, } -/// Accent color for an axis. Semantic palette only; `draw.rs::axis_fg` maps -/// each variant to a terminal color, so `Axis` names no ratatui type. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum AxisColor { - Row, - Column, - Page, - Filter, - Inactive, -} - impl Axis { /// The next axis in the Row → Column → Page → None → Row rotation. /// `Filter` is outside it and cycling one is a no-op (improvise-dz8). @@ -48,17 +37,6 @@ impl Axis { !matches!(self, Axis::Filter) } - /// Accent color. The single source for axis coloring: the tile bar and the - /// category panel each carried a copy before (improvise-rml). - pub fn color(self) -> AxisColor { - match self { - Axis::Row => AxisColor::Row, - Axis::Column => AxisColor::Column, - Axis::Page => AxisColor::Page, - Axis::Filter => AxisColor::Filter, - Axis::None => AxisColor::Inactive, - } - } /// Name with no decoration: "Row", "Col", "Page", "None". pub fn plain_label(self) -> &'static str { @@ -176,15 +154,4 @@ mod tests { assert!(!Axis::Column.filters_data()); assert!(!Axis::None.filters_data()); } - - /// A collision defeats the only reason `AxisColor` exists. - #[test] - fn every_axis_gets_its_own_color() { - let colors = ALL.map(Axis::color); - for (i, a) in colors.iter().enumerate() { - for b in &colors[i + 1..] { - assert_ne!(a, b, "two axes share an accent color"); - } - } - } } diff --git a/crates/improvise-core/src/view/mod.rs b/crates/improvise-core/src/view/mod.rs index fe12c4aa..ca2ce52b 100644 --- a/crates/improvise-core/src/view/mod.rs +++ b/crates/improvise-core/src/view/mod.rs @@ -2,7 +2,7 @@ pub mod axis; pub mod layout; pub mod types; -pub use axis::{Axis, AxisColor}; +pub use axis::Axis; pub use layout::{ AxisEntry, GridEval, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, ViewStep, drill_constraint, drill_snapshot, drill_steps, record_row, records_spine, diff --git a/crates/improvise-protocol/src/face.rs b/crates/improvise-protocol/src/face.rs index 83ccc6d3..fd08dd96 100644 --- a/crates/improvise-protocol/src/face.rs +++ b/crates/improvise-protocol/src/face.rs @@ -575,6 +575,18 @@ mod tests { } } + /// A collision defeats the reason the axis faces exist (improvise-rml). + #[test] + fn every_axis_gets_its_own_accent_in_the_shipped_theme() { + let theme = Theme::shipped(); + let accents = AXES.map(|axis| theme.resolve(Face::Axis(axis))); + for (i, a) in accents.iter().enumerate() { + for b in &accents[i + 1..] { + assert_ne!(a, b, "two axes share an accent"); + } + } + } + /// A child overrides what it states and inherits what it does not, from /// the nearest ancestor that states it — `context/test-evidence/protocol.md`. #[test] diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index e763c3cf..71dabf12 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -396,7 +396,7 @@ impl AppMode { minibuf: MinibufferConfig { buffer_key: "category".into(), prompt: "new category: ".into(), - color: MinibufColor::Yellow, + color: MinibufColor::Green, }, } } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 146d7035..718337e0 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -75,8 +75,8 @@ pub struct RenderModel { pub struct RenderMinibuffer { pub prompt: String, pub value: String, - /// Semantic, as `MinibufColor` is: the last translation to a CSS class is - /// the page's, exactly as `draw.rs::minibuf_fg` is the TUI's (gotcha 26). + /// A colour, not a role: the TUI now draws a face per text-entry mode, and + /// improvise-nn4s.4 moves the page onto the same faces (ADR 004). pub color: improvise_protocol::MinibufColor, } diff --git a/src/draw.rs b/src/draw.rs index 65a8eb52..bd0a48a3 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -1,5 +1,6 @@ use std::io::{self, Stdout}; use std::path::PathBuf; +use std::sync::OnceLock; use std::time::Duration; use anyhow::Result; @@ -17,8 +18,9 @@ use ratatui::{ }; use crate::command::key; +use crate::protocol::face::{AnsiSlot, Colour, Face, Slant, Theme, Weight}; use crate::protocol::EditorSession; -use crate::ui::app::{App, AppMode, MinibufColor}; +use crate::ui::app::{App, AppMode}; use crate::ui::category_panel::CategoryContent; use crate::ui::formula_panel::FormulaContent; use crate::ui::grid::GridWidget; @@ -29,7 +31,6 @@ use crate::ui::text::truncate; use crate::ui::tile_bar::TileBar; use crate::ui::view_panel::ViewContent; use crate::ui::which_key::WhichKeyWidget; -use crate::view::AxisColor; use improvise_command::ui::project; use unicode_width::UnicodeWidthStr; @@ -165,24 +166,54 @@ fn convert_modifiers(mods: event::KeyModifiers) -> key::KeyModifiers { // ── Drawing ────────────────────────────────────────────────────────────────── -/// The one place `AxisColor` becomes a terminal color (improvise-rml); the -/// accent itself is decided by `Axis::color`. See `context/code/tui.md`. -pub(crate) fn axis_fg(color: AxisColor) -> Color { - match color { - AxisColor::Row => Color::Green, - AxisColor::Column => Color::Blue, - AxisColor::Page => Color::Magenta, - AxisColor::Filter => Color::Yellow, - AxisColor::Inactive => Color::DarkGray, +/// The one place a face becomes a terminal style (ADR 004): the theme resolves +/// it, and only this last translation is the TUI's. `context/code/tui.md`. +pub(crate) fn face_style(face: Face) -> Style { + static SHIPPED: OnceLock = OnceLock::new(); + let face = SHIPPED.get_or_init(Theme::shipped).resolve(face); + let mut style = Style::default(); + if let Some(fg) = terminal_colour(face.foreground) { + style = style.fg(fg); } + if let Some(bg) = terminal_colour(face.background) { + style = style.bg(bg); + } + let modifiers = [ + (face.weight == Weight::Bold, Modifier::BOLD), + (face.slant == Slant::Italic, Modifier::ITALIC), + (face.underline, Modifier::UNDERLINED), + (face.reverse, Modifier::REVERSED), + ]; + modifiers + .into_iter() + .filter(|(on, _)| *on) + .fold(style, |style, (_, m)| style.add_modifier(m)) } -/// Map the semantic minibuffer accent to a concrete terminal color. -fn minibuf_fg(color: MinibufColor) -> Color { - match color { - MinibufColor::Green => Color::Green, - MinibufColor::Cyan => Color::Cyan, - MinibufColor::Yellow => Color::Yellow, +/// `None` is "state nothing", so the terminal's own colour shows through. +fn terminal_colour(colour: Colour) -> Option { + match colour { + Colour::Default => None, + Colour::Named(slot) => Some(match slot { + AnsiSlot::Black => Color::Black, + AnsiSlot::Red => Color::Red, + AnsiSlot::Green => Color::Green, + AnsiSlot::Yellow => Color::Yellow, + AnsiSlot::Blue => Color::Blue, + AnsiSlot::Magenta => Color::Magenta, + AnsiSlot::Cyan => Color::Cyan, + AnsiSlot::White => Color::Gray, + AnsiSlot::BrightBlack => Color::DarkGray, + AnsiSlot::BrightRed => Color::LightRed, + AnsiSlot::BrightGreen => Color::LightGreen, + AnsiSlot::BrightYellow => Color::LightYellow, + AnsiSlot::BrightBlue => Color::LightBlue, + AnsiSlot::BrightMagenta => Color::LightMagenta, + AnsiSlot::BrightCyan => Color::LightCyan, + AnsiSlot::BrightWhite => Color::White, + }), + Colour::Indexed(n) => Some(Color::Indexed(n)), + Colour::Rgb(r, g, b) => Some(Color::Rgb(r, g, b)), } } @@ -231,30 +262,6 @@ fn draw_popup_frame(f: &mut Frame, popup: Rect, title: &str, border_color: Color inner } -/// Terminal styling for the mode indicator. Exhaustive on purpose, so a new -/// `AppMode` fails the build here (improvise-zx8t) — `context/code/tui.md`. -fn mode_style(mode: &AppMode) -> Style { - let bg = match mode { - AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => Color::Green, - AppMode::CommandMode { .. } => Color::Yellow, - AppMode::TileSelect => Color::Magenta, - AppMode::Normal - | AppMode::RecordsNormal - | AppMode::FormulaEdit { .. } - | AppMode::FormulaPanel - | AppMode::CategoryPanel - | AppMode::CategoryAdd { .. } - | AppMode::ItemAdd { .. } - | AppMode::ViewPanel - | AppMode::ImportWizard - | AppMode::ExportPrompt { .. } - | AppMode::SavePrompt { .. } - | AppMode::Help - | AppMode::Quit => Color::DarkGray, - }; - Style::default().fg(Color::Black).bg(bg) -} - fn draw(f: &mut Frame, app: &App) { let size = f.area(); @@ -442,12 +449,9 @@ fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) { .unwrap_or(""); let text = minibuffer_line(&mb.prompt, buf, &app.view_state.status_msg, area.width); f.render_widget( - Paragraph::new(text).style( - Style::default() - .fg(minibuf_fg(mb.color)) - .bg(Color::Indexed(235)) - .add_modifier(Modifier::BOLD), - ), + Paragraph::new(text).style(face_style( + Face::of_minibuffer(&app.view_state.mode).unwrap_or(Face::Minibuffer), + )), area, ); } else { @@ -484,7 +488,7 @@ fn draw_status(f: &mut Frame, area: Rect, app: &App) { let line = fill_line(left, &view_badge, area.width, Keeps::Left); f.render_widget( - Paragraph::new(line).style(mode_style(&app.view_state.mode)), + Paragraph::new(line).style(face_style(Face::of_mode(&app.view_state.mode))), area, ); } @@ -639,6 +643,39 @@ mod bottom_bar_tests { ); } + /// The first cell of the bottom row, where the prompt's first glyph lands. + fn prompt_cell(app: &App) -> ratatui::buffer::Cell { + let mut terminal = Terminal::new(TestBackend::new(40, 1)).expect("test backend"); + terminal + .draw(|f| draw_bottom_bar(f, Rect::new(0, 0, 40, 1), app)) + .expect("draw"); + terminal.backend().buffer().content()[0].clone() + } + + /// ADR 004 clause 8's one declared exception (the user, Q1): the + /// category-add prompt draws as item-add does. `context/test-evidence/tui.md`. + #[test] + fn the_category_add_prompt_draws_as_the_item_add_prompt_does() { + let mut app = App::new(Workbook::new("T"), None); + app.view_state.mode = AppMode::item_add("Region".into()); + let item = prompt_cell(&app); + app.view_state.mode = AppMode::category_add(); + let category = prompt_cell(&app); + assert_eq!(category.symbol(), "n", "the prompt is not where it was looked for"); + assert_eq!(item.fg, ratatui::style::Color::Green); + assert_eq!(category.style(), item.style()); + } + + /// The status line wears the mode's face, not the minibuffer's. + #[test] + fn the_status_line_is_drawn_in_the_mode_face() { + let mut app = App::new(Workbook::new("T"), None); + app.view_state.mode = AppMode::TileSelect; + let cell = prompt_cell(&app); + assert_eq!(cell.bg, ratatui::style::Color::Magenta); + assert_eq!(cell.fg, ratatui::style::Color::Black); + } + /// A refusal that keeps the user in a minibuffer stands while they fix it, and /// goes when they leave (improvise-su4u). `context/test-evidence/tui.md`. #[test] @@ -1187,3 +1224,94 @@ mod content_tests { } } } + +#[cfg(test)] +mod face_tests { + use super::*; + use crate::protocol::face::TextEntry; + use crate::protocol::render::WizardFieldKind; + use crate::view::Axis; + + /// `mode_style`'s table as it stood before faces (improvise-zx8t). + fn mode_bg_before_faces(mode: &AppMode) -> Color { + match mode { + AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => Color::Green, + AppMode::CommandMode { .. } => Color::Yellow, + AppMode::TileSelect => Color::Magenta, + AppMode::Normal + | AppMode::RecordsNormal + | AppMode::FormulaEdit { .. } + | AppMode::FormulaPanel + | AppMode::CategoryPanel + | AppMode::CategoryAdd { .. } + | AppMode::ItemAdd { .. } + | AppMode::ViewPanel + | AppMode::ImportWizard + | AppMode::ExportPrompt { .. } + | AppMode::SavePrompt { .. } + | AppMode::Help + | AppMode::Quit => Color::DarkGray, + } + } + + /// ADR 004 clause 8: the shipped theme draws every migrated site exactly as + /// its literal did. `context/test-evidence/tui.md`. + #[test] + fn the_shipped_theme_reproduces_every_migrated_literal() { + let fg = |c| Style::default().fg(c); + let axes = [ + (Axis::Row, Color::Green), + (Axis::Column, Color::Blue), + (Axis::Page, Color::Magenta), + (Axis::Filter, Color::Yellow), + (Axis::None, Color::DarkGray), + ]; + for (axis, color) in axes { + assert_eq!(face_style(Face::Axis(axis)), fg(color), "{axis:?}"); + } + let kinds = [ + (WizardFieldKind::Category, Color::Green), + (WizardFieldKind::Measure, Color::Cyan), + (WizardFieldKind::TimeCategory, Color::Magenta), + (WizardFieldKind::Label, Color::DarkGray), + ]; + for (kind, color) in kinds { + assert_eq!(face_style(Face::WizardField(kind)), fg(color), "{kind:?}"); + } + for mode in AppMode::all() { + let before = fg(Color::Black).bg(mode_bg_before_faces(&mode)); + assert_eq!(face_style(Face::of_mode(&mode)), before, "{mode:?}"); + } + let prompts = [ + (TextEntry::Edit, Color::Green), + (TextEntry::ItemAdd, Color::Green), + (TextEntry::Formula, Color::Cyan), + (TextEntry::Command, Color::Yellow), + (TextEntry::Export, Color::Yellow), + (TextEntry::Save, Color::Yellow), + ]; + for (entry, color) in prompts { + let before = fg(color) + .bg(Color::Indexed(235)) + .add_modifier(Modifier::BOLD); + assert_eq!(face_style(Face::MinibufferOf(entry)), before, "{entry:?}"); + } + } + + #[test] + fn every_face_reaches_the_terminal_with_every_attribute_it_states() { + let resolved = |face| Theme::shipped().resolve(face); + for face in Face::all() { + let r = resolved(face); + let style = face_style(face); + assert_eq!(style.fg, terminal_colour(r.foreground), "{}", face.name()); + assert_eq!(style.bg, terminal_colour(r.background), "{}", face.name()); + assert_eq!( + style.add_modifier.contains(Modifier::BOLD), + r.weight == Weight::Bold, + "{}", + face.name() + ); + } + } +} diff --git a/src/ui/category_panel.rs b/src/ui/category_panel.rs index 6ee4b2e2..8fe861f7 100644 --- a/src/ui/category_panel.rs +++ b/src/ui/category_panel.rs @@ -6,13 +6,14 @@ use ratatui::{ use unicode_width::UnicodeWidthStr; +use crate::protocol::face::Face; use crate::protocol::{CatTreeEntry, CategoryTreeCache, flatten_cat_tree}; use crate::ui::app::Panel as PanelKind; use crate::ui::panel::PanelContent; use crate::view::Axis; -fn axis_display(axis: Axis) -> (String, Color) { - (axis.to_string(), crate::draw::axis_fg(axis.color())) +fn axis_display(axis: Axis) -> (String, Style) { + (axis.to_string(), crate::draw::face_style(Face::Axis(axis))) } /// The category panel draws one flattened tree and nothing else: the axis @@ -78,7 +79,7 @@ impl PanelContent for CategoryContent { axis, } => { let indicator = if *expanded { "▼" } else { "▶" }; - let (axis_str, axis_color) = axis_display(*axis); + let (axis_str, axis_style) = axis_display(*axis); let name_part = format!("{indicator} {name} ({item_count})"); let axis_part = format!(" [{axis_str}]"); @@ -91,7 +92,7 @@ impl PanelContent for CategoryContent { if is_selected { base_style } else { - Style::default().fg(axis_color) + axis_style }, ); } diff --git a/src/ui/import_wizard_ui.rs b/src/ui/import_wizard_ui.rs index f9d11183..d28f20f0 100644 --- a/src/ui/import_wizard_ui.rs +++ b/src/ui/import_wizard_ui.rs @@ -7,7 +7,8 @@ use ratatui::{ use unicode_width::UnicodeWidthStr; -use crate::protocol::{WizardCache, WizardFieldKind, WizardStepKind}; +use crate::protocol::face::Face; +use crate::protocol::{WizardCache, WizardStepKind}; use crate::ui::text::wrap; /// The import wizard, drawn from a [`WizardCache`] (improvise-764) and from @@ -22,17 +23,6 @@ impl<'a> ImportWizardWidget<'a> { } } -/// Map a proposed field's kind to its accent, as `axis_fg` does for -/// `AxisColor` (gotcha 26). The decision is the protocol's; the color is not. -fn field_kind_fg(kind: WizardFieldKind) -> Color { - match kind { - WizardFieldKind::Category => Color::Green, - WizardFieldKind::Measure => Color::Cyan, - WizardFieldKind::TimeCategory => Color::Magenta, - WizardFieldKind::Label => Color::DarkGray, - } -} - impl<'a> Widget for ImportWizardWidget<'a> { fn render(self, area: Rect, buf: &mut Buffer) { let popup_w = area.width.min(80); @@ -139,7 +129,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { } let is_sel = i == self.wizard.cursor; - let kind_color = field_kind_fg(proposal.kind); + let kind_style = crate::draw::face_style(Face::WizardField(proposal.kind)); let accept_str = if proposal.accepted { "[\u{2713}]" @@ -159,7 +149,7 @@ impl<'a> Widget for ImportWizardWidget<'a> { .bg(Color::Cyan) .add_modifier(Modifier::BOLD) } else if proposal.accepted { - Style::default().fg(kind_color) + kind_style } else { Style::default().fg(Color::DarkGray) }; diff --git a/src/ui/tile_bar.rs b/src/ui/tile_bar.rs index 60fac265..b6ddf118 100644 --- a/src/ui/tile_bar.rs +++ b/src/ui/tile_bar.rs @@ -7,6 +7,7 @@ use ratatui::{ use unicode_width::UnicodeWidthStr; use crate::protocol::TileBarCache; +use crate::protocol::face::Face; use crate::ui::app::AppMode; use crate::view::Axis; @@ -44,8 +45,8 @@ impl<'a> TileBar<'a> { tile_cat_idx, } } - fn axis_display(axis: Axis) -> (&'static str, Color) { - (axis.short_label(), crate::draw::axis_fg(axis.color())) + fn axis_display(axis: Axis) -> (&'static str, Style) { + (axis.short_label(), crate::draw::face_style(Face::Axis(axis))) } } @@ -101,7 +102,7 @@ impl<'a> Widget for TileBar<'a> { break; } - let (_, axis_color) = TileBar::axis_display(tiles[i].axis); + let (_, axis_style) = TileBar::axis_display(tiles[i].axis); let is_selected = selected_cat_idx == Some(i); let style = if is_selected { Style::default() @@ -109,7 +110,7 @@ impl<'a> Widget for TileBar<'a> { .bg(Color::Cyan) .add_modifier(Modifier::BOLD) } else { - Style::default().fg(axis_color) + axis_style }; buf.set_string(x, area.y, &labels[i], style); From f7d27307369265820241e6f23b73b982e7fb1a08 Mon Sep 17 00:00:00 2001 From: "Claude (agent-faces)" Date: Thu, 24 Sep 2026 22:05:24 -0700 Subject: [PATCH 3/5] style(faces): rustfmt the face migration (improvise-nn4s.2) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-core/src/view/axis.rs | 1 - crates/improvise-protocol/src/face.rs | 8 +++++++- src/draw.rs | 8 ++++++-- src/ui/category_panel.rs | 6 +----- src/ui/tile_bar.rs | 5 ++++- 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/crates/improvise-core/src/view/axis.rs b/crates/improvise-core/src/view/axis.rs index f43f0b22..77860d28 100644 --- a/crates/improvise-core/src/view/axis.rs +++ b/crates/improvise-core/src/view/axis.rs @@ -37,7 +37,6 @@ impl Axis { !matches!(self, Axis::Filter) } - /// Name with no decoration: "Row", "Col", "Page", "None". pub fn plain_label(self) -> &'static str { match self { diff --git a/crates/improvise-protocol/src/face.rs b/crates/improvise-protocol/src/face.rs index fd08dd96..e2ecdbf4 100644 --- a/crates/improvise-protocol/src/face.rs +++ b/crates/improvise-protocol/src/face.rs @@ -226,7 +226,13 @@ impl TextEntry { } } -const AXES: [Axis; 5] = [Axis::Row, Axis::Column, Axis::Page, Axis::Filter, Axis::None]; +const AXES: [Axis; 5] = [ + Axis::Row, + Axis::Column, + Axis::Page, + Axis::Filter, + Axis::None, +]; const FIELD_KINDS: [WizardFieldKind; 4] = [ WizardFieldKind::Category, diff --git a/src/draw.rs b/src/draw.rs index bd0a48a3..c5ccfa83 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -18,8 +18,8 @@ use ratatui::{ }; use crate::command::key; -use crate::protocol::face::{AnsiSlot, Colour, Face, Slant, Theme, Weight}; use crate::protocol::EditorSession; +use crate::protocol::face::{AnsiSlot, Colour, Face, Slant, Theme, Weight}; use crate::ui::app::{App, AppMode}; use crate::ui::category_panel::CategoryContent; use crate::ui::formula_panel::FormulaContent; @@ -661,7 +661,11 @@ mod bottom_bar_tests { let item = prompt_cell(&app); app.view_state.mode = AppMode::category_add(); let category = prompt_cell(&app); - assert_eq!(category.symbol(), "n", "the prompt is not where it was looked for"); + assert_eq!( + category.symbol(), + "n", + "the prompt is not where it was looked for" + ); assert_eq!(item.fg, ratatui::style::Color::Green); assert_eq!(category.style(), item.style()); } diff --git a/src/ui/category_panel.rs b/src/ui/category_panel.rs index 8fe861f7..7bfb865e 100644 --- a/src/ui/category_panel.rs +++ b/src/ui/category_panel.rs @@ -89,11 +89,7 @@ impl PanelContent for CategoryContent { inner.x + name_part.width() as u16, y, &axis_part, - if is_selected { - base_style - } else { - axis_style - }, + if is_selected { base_style } else { axis_style }, ); } } diff --git a/src/ui/tile_bar.rs b/src/ui/tile_bar.rs index b6ddf118..75dc0d27 100644 --- a/src/ui/tile_bar.rs +++ b/src/ui/tile_bar.rs @@ -46,7 +46,10 @@ impl<'a> TileBar<'a> { } } fn axis_display(axis: Axis) -> (&'static str, Style) { - (axis.short_label(), crate::draw::face_style(Face::Axis(axis))) + ( + axis.short_label(), + crate::draw::face_style(Face::Axis(axis)), + ) } } From 92f8ac2113f6103894a5ce6c0e632feedaac732a Mon Sep 17 00:00:00 2001 From: "Claude (agent-faces)" Date: Thu, 24 Sep 2026 22:05:24 -0700 Subject: [PATCH 4/5] 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`). From 3583359a9c36d0a41f4436ff62aa55ef93b8111d Mon Sep 17 00:00:00 2001 From: "Claude (agent-faces)" Date: Thu, 24 Sep 2026 23:02:55 -0700 Subject: [PATCH 5/5] fix(faces): minibuffer faces inherit mode.text-entry, as ADR 004 says (improvise-nn4s.2) Answers ChatGPT Codex's P2 on PR #319. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 19 +++++--- context/test-evidence/protocol.md | 16 ++++++ crates/improvise-protocol/src/face.rs | 70 ++++++++++++--------------- src/draw.rs | 2 +- 4 files changed, 59 insertions(+), 48 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 0e695880..abb34291 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -2489,14 +2489,17 @@ 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. +- **`minibuffer.*` inherits `mode.text-entry` directly, as the ADR says, and + the shipped theme pays for it.** The mode indicator is black text on an + accent, and the minibuffer is accent text on `Indexed(235)`, bold. So under + this chain every `minibuffer.` in the shipped theme states all three of + its attributes and inherits none of them from its parent. A first version gave + them a `minibuffer` base face instead. ChatGPT Codex caught it on PR #319: a + published parent is compatibility behaviour, so a theme written to the + accepted ADR that restyles `mode.text-entry` would have silently missed every + prompt. `a_theme_that_restyles_text_entry_reaches_every_minibuffer` pins the + accepted chain. Changing it is the user's call and belongs in the ADR, not in + this file. - **`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`. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index c44cd45b..882f49ab 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1283,3 +1283,19 @@ 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. + +### a_theme_that_restyles_text_entry_reaches_every_minibuffer + +ADR 004 says `minibuffer.*` inherits `mode.text-entry` directly. PR #319's first +head gave the minibuffer faces a `minibuffer` base face instead, and ChatGPT +Codex flagged it (P2): face parents are compatibility behaviour once a theme +can be downloaded, so a theme that restyles `mode.text-entry` must reach every +prompt. The test overrides only `mode.text-entry` with italic, an attribute no +minibuffer face states in the shipped theme, and asserts every +`minibuffer.` resolves italic. A test that sets a colour instead would +be vacuous, because each minibuffer face states its own colours. + +**Failed first**, against the `minibuffer` base parent: `Edit` resolved to +`Normal`. It passes under the accepted chain. The no-op oracle +`the_shipped_theme_reproduces_every_migrated_literal` passed both before and +after the change, which shows the change moved no pixel. diff --git a/crates/improvise-protocol/src/face.rs b/crates/improvise-protocol/src/face.rs index e2ecdbf4..5173b575 100644 --- a/crates/improvise-protocol/src/face.rs +++ b/crates/improvise-protocol/src/face.rs @@ -276,7 +276,6 @@ pub enum Face { ModeOverlay, ModeTransient, ModeTileSelect, - Minibuffer, MinibufferOf(TextEntry), } @@ -290,7 +289,6 @@ impl Face { out.extend(TextEntry::ALL.map(Face::ModeTextEntryOf)); out.extend(Panel::ALL.map(Face::ModePanel)); out.extend([Face::ModeOverlay, Face::ModeTransient, Face::ModeTileSelect]); - out.push(Face::Minibuffer); out.extend(TextEntry::ALL.map(Face::MinibufferOf)); out } @@ -299,7 +297,7 @@ impl Face { pub fn parent(self) -> Option { match self { Face::Default => None, - Face::Dim | Face::Mode | Face::Minibuffer => Some(Face::Default), + Face::Dim | Face::Mode => Some(Face::Default), Face::Axis(Axis::None) | Face::WizardField(WizardFieldKind::Label) => Some(Face::Dim), Face::Axis(_) | Face::WizardField(_) => Some(Face::Default), Face::ModeResting @@ -308,8 +306,7 @@ impl Face { | Face::ModeOverlay | Face::ModeTransient | Face::ModeTileSelect => Some(Face::Mode), - Face::ModeTextEntryOf(_) => Some(Face::ModeTextEntry), - Face::MinibufferOf(_) => Some(Face::Minibuffer), + Face::ModeTextEntryOf(_) | Face::MinibufferOf(_) => Some(Face::ModeTextEntry), } } @@ -328,7 +325,6 @@ impl Face { Face::ModeOverlay => "mode.overlay".into(), Face::ModeTransient => "mode.transient".into(), Face::ModeTileSelect => "mode.tile-select".into(), - Face::Minibuffer => "minibuffer".into(), Face::MinibufferOf(entry) => format!("minibuffer.{}", entry.name()), } } @@ -450,6 +446,12 @@ impl Theme { pub fn shipped() -> Theme { use AnsiSlot::*; let named = Colour::Named; + let minibuffer = |accent| FaceSpec { + foreground: Some(named(accent)), + background: Some(Colour::Indexed(235)), + weight: Some(Weight::Bold), + ..FaceSpec::default() + }; let faces = [ (Face::Dim, FaceSpec::fg(named(BrightBlack))), (Face::Axis(Axis::Row), FaceSpec::fg(named(Green))), @@ -485,42 +487,16 @@ impl Theme { FaceSpec::bg(named(Yellow)), ), (Face::ModeTileSelect, FaceSpec::bg(named(Magenta))), - ( - Face::Minibuffer, - FaceSpec { - background: Some(Colour::Indexed(235)), - weight: Some(Weight::Bold), - ..FaceSpec::default() - }, - ), - ( - Face::MinibufferOf(TextEntry::Edit), - FaceSpec::fg(named(Green)), - ), - ( - Face::MinibufferOf(TextEntry::ItemAdd), - FaceSpec::fg(named(Green)), - ), + (Face::MinibufferOf(TextEntry::Edit), minibuffer(Green)), + (Face::MinibufferOf(TextEntry::ItemAdd), minibuffer(Green)), ( Face::MinibufferOf(TextEntry::CategoryAdd), - FaceSpec::fg(named(Green)), - ), - ( - Face::MinibufferOf(TextEntry::Formula), - FaceSpec::fg(named(Cyan)), - ), - ( - Face::MinibufferOf(TextEntry::Command), - FaceSpec::fg(named(Yellow)), - ), - ( - Face::MinibufferOf(TextEntry::Export), - FaceSpec::fg(named(Yellow)), - ), - ( - Face::MinibufferOf(TextEntry::Save), - FaceSpec::fg(named(Yellow)), + minibuffer(Green), ), + (Face::MinibufferOf(TextEntry::Formula), minibuffer(Cyan)), + (Face::MinibufferOf(TextEntry::Command), minibuffer(Yellow)), + (Face::MinibufferOf(TextEntry::Export), minibuffer(Yellow)), + (Face::MinibufferOf(TextEntry::Save), minibuffer(Yellow)), ]; Theme { faces: faces.into_iter().collect(), @@ -581,6 +557,22 @@ mod tests { } } + /// ADR 004: `minibuffer.*` inherits `mode.text-entry` directly, so a theme + /// that restyles text entry reaches every prompt. `context/test-evidence/protocol.md`. + #[test] + fn a_theme_that_restyles_text_entry_reaches_every_minibuffer() { + let mut theme = Theme::shipped(); + let italic = FaceSpec { + slant: Some(Slant::Italic), + ..FaceSpec::default() + }; + theme.faces.insert(Face::ModeTextEntry, italic); + for entry in TextEntry::ALL { + let got = theme.resolve(Face::MinibufferOf(entry)); + assert_eq!(got.slant, Slant::Italic, "{entry:?}"); + } + } + /// A collision defeats the reason the axis faces exist (improvise-rml). #[test] fn every_axis_gets_its_own_accent_in_the_shipped_theme() { diff --git a/src/draw.rs b/src/draw.rs index 6005c190..af5a86e7 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -450,7 +450,7 @@ fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) { let text = minibuffer_line(&mb.prompt, buf, &app.view_state.status_msg, area.width); f.render_widget( Paragraph::new(text).style(face_style( - Face::of_minibuffer(&app.view_state.mode).unwrap_or(Face::Minibuffer), + Face::of_minibuffer(&app.view_state.mode).unwrap_or(Face::ModeTextEntry), )), area, );