mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
Keep submitted prompts shaded when terminal colors are unavailable (#57554)
## Why Terminals that do not answer the default-color probe lose the background shading on submitted prompts. ## What changed Use an ANSI `DarkGray` background as a fallback. Keep the default text foreground when the background's RGB value is unknown, and preserve the red voice marker in terminal history and transcript selections. ## Testing Add regression coverage for prompt shading without a terminal palette, voice marker colors under both history wrapping policies, and spoken-prompt selection contrast in compact and detailed transcripts. GitOrigin-RevId: b8217a2ee1d37a2ea4a011faa75427c881202b82
This commit is contained in:
committed by
copyberry
parent
f088e0c4ff
commit
33d306d457
+3
-2
@@ -25,8 +25,9 @@ Buffer {
|
||||
x: 32, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 34, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 4, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 5, fg: Reset, bg: DarkGray, underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 5, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: BOLD,
|
||||
x: 6, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 13, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
|
||||
@@ -167,6 +167,19 @@ fn build_user_message_lines_with_elements(
|
||||
raw_lines
|
||||
}
|
||||
|
||||
fn user_message_element_style(style: Style, accent: Color) -> Style {
|
||||
let unresolved_background = match style.bg {
|
||||
Some(Color::Indexed(index)) => index < 16,
|
||||
Some(Color::Rgb(..) | Color::Reset) | None => false,
|
||||
Some(_) => true,
|
||||
};
|
||||
if accent == Color::Reset || unresolved_background {
|
||||
style
|
||||
} else {
|
||||
style.fg(accent)
|
||||
}
|
||||
}
|
||||
|
||||
impl UserHistoryCell {
|
||||
pub(crate) fn with_async_replies(
|
||||
mut self,
|
||||
@@ -234,7 +247,7 @@ impl HistoryCell for UserHistoryCell {
|
||||
.max(/*other*/ 1);
|
||||
|
||||
let style = history_prompt_style();
|
||||
let element_style = style.fg(accent_color_on(style.bg));
|
||||
let element_style = user_message_element_style(style, accent_color_on(style.bg));
|
||||
|
||||
let wrapped_images = plain_hyperlink_lines(adaptive_wrap_lines(
|
||||
self.image_labels_not_in_message()
|
||||
@@ -243,7 +256,7 @@ impl HistoryCell for UserHistoryCell {
|
||||
.wrap_algorithm(textwrap::WrapAlgorithm::FirstFit),
|
||||
));
|
||||
|
||||
let wrapped_message =
|
||||
let mut wrapped_message =
|
||||
if let UserMessagePresentation::AsyncReplies { replies, cwd } = &self.presentation {
|
||||
super::async_replies::render(replies, cwd, usize::from(wrap_width))
|
||||
} else {
|
||||
@@ -272,6 +285,13 @@ impl HistoryCell for UserHistoryCell {
|
||||
lines.push(HyperlinkLine::new(Line::from("").style(style)));
|
||||
}
|
||||
}
|
||||
if self.is_spoken()
|
||||
&& let Some(first) = wrapped_message.first_mut()
|
||||
{
|
||||
// Wrapping already applied the foreground to the text. Native history
|
||||
// applies line styles over spans, so leave the voice marker's color intact.
|
||||
first.line.style.fg = None;
|
||||
}
|
||||
lines.extend(wrapped_message);
|
||||
lines.push(HyperlinkLine::new(Line::from("").style(style)));
|
||||
for source in lines.iter_mut().filter_map(|line| line.source.as_mut()) {
|
||||
@@ -284,8 +304,20 @@ impl HistoryCell for UserHistoryCell {
|
||||
lines
|
||||
}
|
||||
|
||||
fn compact_hyperlink_lines(&self, width: u16) -> Vec<HyperlinkLine> {
|
||||
self.transcript_hyperlink_lines(width)
|
||||
}
|
||||
|
||||
fn transcript_hyperlink_lines(&self, width: u16) -> Vec<HyperlinkLine> {
|
||||
self.display_hyperlink_lines(width)
|
||||
let mut lines = self.display_hyperlink_lines(width);
|
||||
if self.is_spoken() {
|
||||
for line in &mut lines {
|
||||
if let Some(source) = &line.source {
|
||||
line.line.style.fg = source.line_style.fg;
|
||||
}
|
||||
}
|
||||
}
|
||||
lines
|
||||
}
|
||||
|
||||
fn raw_lines(&self) -> Vec<Line<'static>> {
|
||||
@@ -309,7 +341,7 @@ fn wrap_user_message(
|
||||
style: Style,
|
||||
wrap_width: u16,
|
||||
) -> Vec<HyperlinkLine> {
|
||||
let element_style = style.fg(accent_color_on(style.bg));
|
||||
let element_style = user_message_element_style(style, accent_color_on(style.bg));
|
||||
if message.is_empty() && text_elements.is_empty() {
|
||||
return Vec::new();
|
||||
}
|
||||
|
||||
+3
-3
@@ -1,6 +1,5 @@
|
||||
---
|
||||
source: tui/src/history_cell/messages_tests.rs
|
||||
assertion_line: 107
|
||||
expression: "format!(\"{buf:?}\")"
|
||||
---
|
||||
Buffer {
|
||||
@@ -12,7 +11,8 @@ Buffer {
|
||||
],
|
||||
styles: [
|
||||
x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Red, bg: Reset, underline: Reset, modifier: BOLD,
|
||||
x: 2, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Red, bg: DarkGray, underline: Reset, modifier: BOLD,
|
||||
x: 2, y: 1, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 18, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
]
|
||||
}
|
||||
|
||||
@@ -816,6 +816,49 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_spoken_prompt_keeps_its_marker_color_without_a_terminal_palette() {
|
||||
use crate::history_cell::HistoryCell;
|
||||
use crate::history_cell::new_spoken_user_prompt;
|
||||
|
||||
crate::terminal_palette::with_test_default_colors(
|
||||
/*colors*/ None::<crate::terminal_probe::DefaultColors>,
|
||||
|| {
|
||||
let width = 30;
|
||||
let lines = new_spoken_user_prompt("hello from voice".into())
|
||||
.display_hyperlink_lines(width);
|
||||
let mut snapshots = Vec::new();
|
||||
for policy in [
|
||||
HistoryLineWrapPolicy::PreWrap,
|
||||
HistoryLineWrapPolicy::Terminal,
|
||||
] {
|
||||
let mut backend = VT100Backend::new(width, /*height*/ 1);
|
||||
let (wrapped, _) =
|
||||
wrap_history_hyperlink_lines(&lines[1..2], usize::from(width), policy);
|
||||
write_history_line(&mut backend, &wrapped[0], usize::from(width))
|
||||
.expect("write spoken prompt");
|
||||
let screen = backend.vt100().screen();
|
||||
let marker = screen.cell(/*row*/ 0, /*col*/ 0).unwrap();
|
||||
let text = screen.cell(/*row*/ 0, /*col*/ 2).unwrap();
|
||||
let margin = screen.cell(/*row*/ 0, width - 1).unwrap();
|
||||
snapshots.push(format!(
|
||||
"{policy:?}: {}\nmarker: {:?}, text: {:?}, background: {:?}",
|
||||
screen.rows(/*start*/ 0, width).next().unwrap().trim_end(),
|
||||
marker.fgcolor(),
|
||||
text.fgcolor(),
|
||||
margin.bgcolor(),
|
||||
));
|
||||
}
|
||||
insta::assert_snapshot!(snapshots.join("\n"), @"
|
||||
PreWrap: › hello from voice
|
||||
marker: Idx(1), text: Default, background: Idx(8)
|
||||
Terminal: › hello from voice
|
||||
marker: Idx(1), text: Default, background: Idx(8)
|
||||
");
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_user_message_url_wrap_preserves_gutter_and_background() {
|
||||
use crate::history_cell::HistoryCell;
|
||||
|
||||
+23
@@ -0,0 +1,23 @@
|
||||
---
|
||||
source: tui/src/transcript_view_tests.rs
|
||||
expression: "format!(\"{buffer:?}\")"
|
||||
---
|
||||
Buffer {
|
||||
area: Rect { x: 0, y: 0, width: 32, height: 8 },
|
||||
content: [
|
||||
" ",
|
||||
" [Image #1] ",
|
||||
" ",
|
||||
"› First steering message ",
|
||||
" ",
|
||||
" ",
|
||||
" ",
|
||||
" ",
|
||||
],
|
||||
styles: [
|
||||
x: 0, y: 0, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 3, fg: Reset, bg: DarkGray, underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 3, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
]
|
||||
}
|
||||
@@ -62,9 +62,12 @@ pub fn user_message_style() -> Style {
|
||||
}
|
||||
|
||||
/// Submitted prompts use a lighter fill than the editable composer in either theme.
|
||||
///
|
||||
/// Some terminals do not answer the optional default-color probe. Keep submitted prompts shaded
|
||||
/// with an ANSI-safe fallback instead of dropping the surface.
|
||||
pub(crate) fn history_prompt_style() -> Style {
|
||||
let Some(background) = default_bg() else {
|
||||
return Style::default();
|
||||
return Style::default().bg(Color::DarkGray);
|
||||
};
|
||||
let (foreground, alpha) = if is_light(background) {
|
||||
((0, 0, 0), 0.02)
|
||||
|
||||
@@ -42,7 +42,7 @@ pub fn best_color_for_level(target: (u8, u8, u8), color_level: StdoutColorLevel)
|
||||
|
||||
pub(crate) fn effective_stdout_color_level() -> StdoutColorLevel {
|
||||
#[cfg(test)]
|
||||
if TEST_DEFAULT_COLORS.with(|colors| colors.get().is_some()) {
|
||||
if TEST_DEFAULT_COLORS.with(|colors| colors.get().flatten().is_some()) {
|
||||
return StdoutColorLevel::TrueColor;
|
||||
}
|
||||
|
||||
@@ -96,7 +96,7 @@ pub struct DefaultColors {
|
||||
|
||||
#[cfg(test)]
|
||||
thread_local! {
|
||||
static TEST_DEFAULT_COLORS: std::cell::Cell<Option<DefaultColors>> = const {
|
||||
static TEST_DEFAULT_COLORS: std::cell::Cell<Option<Option<DefaultColors>>> = const {
|
||||
std::cell::Cell::new(None)
|
||||
};
|
||||
}
|
||||
@@ -104,23 +104,24 @@ thread_local! {
|
||||
pub fn default_colors() -> Option<DefaultColors> {
|
||||
#[cfg(test)]
|
||||
if let Some(colors) = TEST_DEFAULT_COLORS.with(std::cell::Cell::get) {
|
||||
return Some(colors);
|
||||
return colors;
|
||||
}
|
||||
|
||||
imp::default_colors()
|
||||
}
|
||||
|
||||
/// Scope a truecolor terminal palette to the current test thread while rendering a real widget.
|
||||
/// Scope a terminal palette (or its absence) to the current test thread while rendering a widget.
|
||||
#[cfg(test)]
|
||||
pub(crate) fn with_test_default_colors<T>(
|
||||
colors: crate::terminal_probe::DefaultColors,
|
||||
colors: impl Into<Option<crate::terminal_probe::DefaultColors>>,
|
||||
render: impl FnOnce() -> T,
|
||||
) -> T {
|
||||
TEST_DEFAULT_COLORS.with(|override_colors| {
|
||||
let previous = override_colors.replace(Some(DefaultColors {
|
||||
let colors = colors.into().map(|colors| DefaultColors {
|
||||
fg: colors.fg,
|
||||
bg: colors.bg,
|
||||
}));
|
||||
});
|
||||
let previous = override_colors.replace(Some(colors));
|
||||
let result = render();
|
||||
override_colors.set(previous);
|
||||
result
|
||||
|
||||
@@ -1284,3 +1284,80 @@ fn history_prefetch_counts_rows_in_preceding_entries() {
|
||||
assert!(view.near_start(&cells));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn user_prompt_has_a_background_without_a_terminal_palette() {
|
||||
use crate::history_cell::new_user_prompt;
|
||||
use codex_protocol::user_input::TextElement;
|
||||
use std::path::PathBuf;
|
||||
|
||||
let message = "First steering message";
|
||||
let cells: Vec<Arc<dyn HistoryCell>> = vec![Arc::new(new_user_prompt(
|
||||
message.into(),
|
||||
vec![TextElement::new((0..5).into(), Some("First".into()))],
|
||||
vec![PathBuf::from("screenshot.png")],
|
||||
Vec::new(),
|
||||
))];
|
||||
crate::terminal_palette::with_test_default_colors(
|
||||
/*colors*/ None::<crate::terminal_probe::DefaultColors>,
|
||||
|| {
|
||||
let mut view = TranscriptView::default();
|
||||
let buffer = render(&mut view, &cells, /*width*/ 32, /*height*/ 8);
|
||||
insta::assert_snapshot!(
|
||||
"user_prompt_has_a_background_without_a_terminal_palette",
|
||||
format!("{buffer:?}")
|
||||
);
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spoken_prompt_selection_keeps_its_fallback_contrast() {
|
||||
use crate::history_cell::new_spoken_user_prompt;
|
||||
use ratatui::style::Color;
|
||||
use ratatui::style::Modifier;
|
||||
|
||||
crate::terminal_palette::with_test_default_colors(
|
||||
/*colors*/ None::<crate::terminal_probe::DefaultColors>,
|
||||
|| {
|
||||
let cells: Vec<Arc<dyn HistoryCell>> =
|
||||
vec![Arc::new(new_spoken_user_prompt("one\nnext".into()))];
|
||||
for detailed in [false, true] {
|
||||
let mut view = TranscriptView {
|
||||
detailed,
|
||||
..TranscriptView::default()
|
||||
};
|
||||
let before = render(&mut view, &cells, /*width*/ 20, /*height*/ 5);
|
||||
let rendered = text(&before);
|
||||
let first = rendered
|
||||
.lines()
|
||||
.position(|row| row.contains("› one"))
|
||||
.unwrap() as u16;
|
||||
let second = rendered
|
||||
.lines()
|
||||
.position(|row| row.contains("next"))
|
||||
.unwrap() as u16;
|
||||
view.begin_selection(&cells, /*column*/ 2, first, /*clicks*/ 1);
|
||||
view.extend_selection(/*column*/ 2, second);
|
||||
let selected = render(&mut view, &cells, /*width*/ 20, /*height*/ 5);
|
||||
let newline = &selected[(5, first)];
|
||||
assert_eq!(
|
||||
(
|
||||
view.selected_text(&cells).as_deref(),
|
||||
selected[(0, first)].fg,
|
||||
newline.fg,
|
||||
newline.bg,
|
||||
newline.modifier.contains(Modifier::REVERSED),
|
||||
),
|
||||
(
|
||||
Some("one\n"),
|
||||
Color::Red,
|
||||
Color::Reset,
|
||||
Color::DarkGray,
|
||||
true
|
||||
)
|
||||
);
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user