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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
committed by
Edward Langley
parent
e376034453
commit
3583359a9c
@@ -2489,14 +2489,17 @@ dev-dependency here.
|
|||||||
|
|
||||||
### Choices the ADR did not make, made here
|
### Choices the ADR did not make, made here
|
||||||
|
|
||||||
- **`minibuffer.*` inherits a `minibuffer` base face, not `mode.text-entry`.**
|
- **`minibuffer.*` inherits `mode.text-entry` directly, as the ADR says, and
|
||||||
The ADR says the minibuffer faces "inherit `mode.text-entry` directly". But
|
the shipped theme pays for it.** The mode indicator is black text on an
|
||||||
the mode indicator and the minibuffer paint opposite halves of the cell — the
|
accent, and the minibuffer is accent text on `Indexed(235)`, bold. So under
|
||||||
indicator is Black ink on an accent background, the minibuffer is accent ink
|
this chain every `minibuffer.<entry>` in the shipped theme states all three of
|
||||||
on `Indexed(235)` — so inheriting one from the other makes every attribute an
|
its attributes and inherits none of them from its parent. A first version gave
|
||||||
override and the chain decorative. The `minibuffer` face carries only what
|
them a `minibuffer` base face instead. ChatGPT Codex caught it on PR #319: a
|
||||||
the strip has in common (its background and bold), and encodes nothing about
|
published parent is compatibility behaviour, so a theme written to the
|
||||||
green and yellow, which is the property the ADR's sentence was protecting.
|
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
|
- **`mode.tile-select` hangs directly off `mode`.** The ADR's decomposition
|
||||||
covers the thirteen DarkGray modes and the text-entry chain, and does not
|
covers the thirteen DarkGray modes and the text-entry chain, and does not
|
||||||
place `TileSelect`.
|
place `TileSelect`.
|
||||||
|
|||||||
@@ -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
|
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
|
is the stronger question: two distinct `AxisColor` arms could always have been
|
||||||
mapped to one terminal colour.
|
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.<entry>` 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.
|
||||||
|
|||||||
@@ -276,7 +276,6 @@ pub enum Face {
|
|||||||
ModeOverlay,
|
ModeOverlay,
|
||||||
ModeTransient,
|
ModeTransient,
|
||||||
ModeTileSelect,
|
ModeTileSelect,
|
||||||
Minibuffer,
|
|
||||||
MinibufferOf(TextEntry),
|
MinibufferOf(TextEntry),
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -290,7 +289,6 @@ impl Face {
|
|||||||
out.extend(TextEntry::ALL.map(Face::ModeTextEntryOf));
|
out.extend(TextEntry::ALL.map(Face::ModeTextEntryOf));
|
||||||
out.extend(Panel::ALL.map(Face::ModePanel));
|
out.extend(Panel::ALL.map(Face::ModePanel));
|
||||||
out.extend([Face::ModeOverlay, Face::ModeTransient, Face::ModeTileSelect]);
|
out.extend([Face::ModeOverlay, Face::ModeTransient, Face::ModeTileSelect]);
|
||||||
out.push(Face::Minibuffer);
|
|
||||||
out.extend(TextEntry::ALL.map(Face::MinibufferOf));
|
out.extend(TextEntry::ALL.map(Face::MinibufferOf));
|
||||||
out
|
out
|
||||||
}
|
}
|
||||||
@@ -299,7 +297,7 @@ impl Face {
|
|||||||
pub fn parent(self) -> Option<Face> {
|
pub fn parent(self) -> Option<Face> {
|
||||||
match self {
|
match self {
|
||||||
Face::Default => None,
|
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(Axis::None) | Face::WizardField(WizardFieldKind::Label) => Some(Face::Dim),
|
||||||
Face::Axis(_) | Face::WizardField(_) => Some(Face::Default),
|
Face::Axis(_) | Face::WizardField(_) => Some(Face::Default),
|
||||||
Face::ModeResting
|
Face::ModeResting
|
||||||
@@ -308,8 +306,7 @@ impl Face {
|
|||||||
| Face::ModeOverlay
|
| Face::ModeOverlay
|
||||||
| Face::ModeTransient
|
| Face::ModeTransient
|
||||||
| Face::ModeTileSelect => Some(Face::Mode),
|
| Face::ModeTileSelect => Some(Face::Mode),
|
||||||
Face::ModeTextEntryOf(_) => Some(Face::ModeTextEntry),
|
Face::ModeTextEntryOf(_) | Face::MinibufferOf(_) => Some(Face::ModeTextEntry),
|
||||||
Face::MinibufferOf(_) => Some(Face::Minibuffer),
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -328,7 +325,6 @@ impl Face {
|
|||||||
Face::ModeOverlay => "mode.overlay".into(),
|
Face::ModeOverlay => "mode.overlay".into(),
|
||||||
Face::ModeTransient => "mode.transient".into(),
|
Face::ModeTransient => "mode.transient".into(),
|
||||||
Face::ModeTileSelect => "mode.tile-select".into(),
|
Face::ModeTileSelect => "mode.tile-select".into(),
|
||||||
Face::Minibuffer => "minibuffer".into(),
|
|
||||||
Face::MinibufferOf(entry) => format!("minibuffer.{}", entry.name()),
|
Face::MinibufferOf(entry) => format!("minibuffer.{}", entry.name()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -450,6 +446,12 @@ impl Theme {
|
|||||||
pub fn shipped() -> Theme {
|
pub fn shipped() -> Theme {
|
||||||
use AnsiSlot::*;
|
use AnsiSlot::*;
|
||||||
let named = Colour::Named;
|
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 = [
|
let faces = [
|
||||||
(Face::Dim, FaceSpec::fg(named(BrightBlack))),
|
(Face::Dim, FaceSpec::fg(named(BrightBlack))),
|
||||||
(Face::Axis(Axis::Row), FaceSpec::fg(named(Green))),
|
(Face::Axis(Axis::Row), FaceSpec::fg(named(Green))),
|
||||||
@@ -485,42 +487,16 @@ impl Theme {
|
|||||||
FaceSpec::bg(named(Yellow)),
|
FaceSpec::bg(named(Yellow)),
|
||||||
),
|
),
|
||||||
(Face::ModeTileSelect, FaceSpec::bg(named(Magenta))),
|
(Face::ModeTileSelect, FaceSpec::bg(named(Magenta))),
|
||||||
(
|
(Face::MinibufferOf(TextEntry::Edit), minibuffer(Green)),
|
||||||
Face::Minibuffer,
|
(Face::MinibufferOf(TextEntry::ItemAdd), minibuffer(Green)),
|
||||||
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),
|
Face::MinibufferOf(TextEntry::CategoryAdd),
|
||||||
FaceSpec::fg(named(Green)),
|
minibuffer(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)),
|
|
||||||
),
|
),
|
||||||
|
(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 {
|
Theme {
|
||||||
faces: faces.into_iter().collect(),
|
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).
|
/// A collision defeats the reason the axis faces exist (improvise-rml).
|
||||||
#[test]
|
#[test]
|
||||||
fn every_axis_gets_its_own_accent_in_the_shipped_theme() {
|
fn every_axis_gets_its_own_accent_in_the_shipped_theme() {
|
||||||
|
|||||||
+1
-1
@@ -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);
|
let text = minibuffer_line(&mb.prompt, buf, &app.view_state.status_msg, area.width);
|
||||||
f.render_widget(
|
f.render_widget(
|
||||||
Paragraph::new(text).style(face_style(
|
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,
|
area,
|
||||||
);
|
);
|
||||||
|
|||||||
Reference in New Issue
Block a user