mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Avoid redundant spacing above the transcript composer (#47331)
## Why An empty composer hint row already separates activity from the input. Adding another separator wastes transcript space, especially in short terminals. ## What changed Reserve the extra separator only when copy status text, a follow control, or a fitting hint occupies the shared row and the viewport has room. Recalculate the transcript layout in the same frame when adding that separator, and use a shared timestamp for copy status visibility and expiry. ## Testing Extend snapshots for running turns with no hint, follow controls in short terminals, and copy status text at narrow widths and limited heights. Update resize assertions to check that the draft and caret remain stable relative to the terminal bottom. GitOrigin-RevId: a73894063b759ec6b9a25f720e0e8007516109c8
This commit is contained in:
@@ -16,6 +16,36 @@ pub(super) struct CopyFeedback {
|
||||
}
|
||||
|
||||
impl TranscriptView {
|
||||
pub(crate) fn composer_gap_has_content(
|
||||
&self,
|
||||
width: u16,
|
||||
hint: Option<&HyperlinkLine>,
|
||||
now: Instant,
|
||||
) -> bool {
|
||||
if let Some(feedback) = self
|
||||
.copy_feedback
|
||||
.as_ref()
|
||||
.filter(|feedback| feedback.expires_at > now)
|
||||
{
|
||||
return feedback.line(width.saturating_sub(/*rhs*/ 1)).width() > 0;
|
||||
}
|
||||
self.follow_control_label(width).is_some()
|
||||
|| self.fitting_composer_hint(width, hint).is_some()
|
||||
}
|
||||
|
||||
fn fitting_composer_hint<'a>(
|
||||
&self,
|
||||
width: u16,
|
||||
hint: Option<&'a HyperlinkLine>,
|
||||
) -> Option<&'a HyperlinkLine> {
|
||||
hint.filter(|hint| {
|
||||
self.is_following()
|
||||
&& !self.has_active_interaction()
|
||||
&& hint.width() > 0
|
||||
&& hint.width() + 2 <= usize::from(width)
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn show_copy_feedback(
|
||||
&mut self,
|
||||
result: &Result<CopyStatus, String>,
|
||||
@@ -33,12 +63,13 @@ impl TranscriptView {
|
||||
area: Option<Rect>,
|
||||
hint: Option<&HyperlinkLine>,
|
||||
buffer: &mut Buffer,
|
||||
now: Instant,
|
||||
) -> Option<Duration> {
|
||||
self.composer_tip = None;
|
||||
if self
|
||||
.copy_feedback
|
||||
.as_ref()
|
||||
.is_some_and(|feedback| feedback.expires_at <= Instant::now())
|
||||
.is_some_and(|feedback| feedback.expires_at <= now)
|
||||
{
|
||||
self.copy_feedback = None;
|
||||
}
|
||||
@@ -48,9 +79,7 @@ impl TranscriptView {
|
||||
};
|
||||
if let Some(feedback) = &self.copy_feedback {
|
||||
let line = feedback.line(area.width.saturating_sub(/*rhs*/ 1));
|
||||
let delay = feedback
|
||||
.expires_at
|
||||
.saturating_duration_since(Instant::now());
|
||||
let delay = feedback.expires_at.saturating_duration_since(now);
|
||||
let width = line.width().min(usize::from(area.width)) as u16;
|
||||
let target = Rect::new(
|
||||
area.right().saturating_sub(width + 1).max(area.x),
|
||||
@@ -63,10 +92,7 @@ impl TranscriptView {
|
||||
return Some(delay);
|
||||
}
|
||||
self.render_follow_control(Some(area), buffer);
|
||||
if self.is_following()
|
||||
&& !self.has_active_interaction()
|
||||
&& let Some(hint) = hint.filter(|hint| hint.width() + 2 <= usize::from(area.width))
|
||||
{
|
||||
if let Some(hint) = self.fitting_composer_hint(area.width, hint) {
|
||||
let width = hint.width() as u16;
|
||||
let target = Rect::new(area.right() - width - 1, area.y, width, /*height*/ 1);
|
||||
HyperlinkParagraph::new(std::slice::from_ref(hint), Style::default())
|
||||
|
||||
@@ -21,11 +21,17 @@ fn copy_feedback_is_right_aligned_and_does_not_claim_terminal_delivery() {
|
||||
for width in [80, 32, 18] {
|
||||
let mut view = TranscriptView::default();
|
||||
view.show_copy_feedback(&result, /*characters*/ 24);
|
||||
assert!(view.composer_gap_has_content(width, /*hint*/ None, Instant::now()));
|
||||
let area = Rect::new(/*x*/ 0, /*y*/ 0, width, /*height*/ 1);
|
||||
let mut buffer = Buffer::empty(area);
|
||||
assert!(
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer)
|
||||
.is_some()
|
||||
view.render_composer_gap(
|
||||
Some(area),
|
||||
/*hint*/ None,
|
||||
&mut buffer,
|
||||
Instant::now()
|
||||
)
|
||||
.is_some()
|
||||
);
|
||||
assert_eq!(buffer[(width - 1, 0)].symbol(), " ");
|
||||
snapshots.push(format!("{result:?}, {width} columns\n{}", text(&buffer)));
|
||||
@@ -45,20 +51,24 @@ fn feedback_releases_navigation_targets_and_expiry_restores_them() {
|
||||
/*x*/ 0, /*y*/ 3, /*width*/ 60, /*height*/ 1,
|
||||
);
|
||||
let mut buffer = Buffer::empty(area);
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer);
|
||||
assert!(view.composer_gap_has_content(area.width, /*hint*/ None, Instant::now()));
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer, Instant::now());
|
||||
assert!(text(&buffer).contains("Back to bottom"));
|
||||
view.show_copy_feedback(&Ok(CopyStatus::Unconfirmed), /*characters*/ 3);
|
||||
buffer.reset();
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer);
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer, Instant::now());
|
||||
assert!(!text(&buffer).contains("Back to bottom"));
|
||||
assert!(!view.is_following());
|
||||
view.copy_feedback.as_mut().unwrap().expires_at = Instant::now();
|
||||
buffer.reset();
|
||||
assert_eq!(
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer),
|
||||
view.render_composer_gap(Some(area), /*hint*/ None, &mut buffer, Instant::now()),
|
||||
None
|
||||
);
|
||||
assert!(text(&buffer).contains("Back to bottom"));
|
||||
view.jump_to_latest();
|
||||
render(&mut view, &cells, /*width*/ 60, /*height*/ 3);
|
||||
assert!(!view.composer_gap_has_content(area.width, /*hint*/ None, Instant::now()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -77,7 +87,7 @@ fn tip_links_follow_alignment_and_release_stale_targets() {
|
||||
row: area.y,
|
||||
modifiers: KeyModifiers::CONTROL,
|
||||
};
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer, Instant::now());
|
||||
assert!(
|
||||
buffer[(click.column, click.row)]
|
||||
.symbol()
|
||||
@@ -120,8 +130,13 @@ fn tip_links_follow_alignment_and_release_stale_targets() {
|
||||
}
|
||||
|
||||
for replacement in [None, Some(HyperlinkLine::from("Other tip"))] {
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(Some(area), replacement.as_ref(), &mut buffer);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer, Instant::now());
|
||||
view.render_composer_gap(
|
||||
Some(area),
|
||||
replacement.as_ref(),
|
||||
&mut buffer,
|
||||
Instant::now(),
|
||||
);
|
||||
assert!(view.handle_mouse(click, &[]).is_none());
|
||||
}
|
||||
for next_area in [
|
||||
@@ -136,16 +151,16 @@ fn tip_links_follow_alignment_and_release_stale_targets() {
|
||||
/*x*/ 4, /*y*/ 3, /*width*/ 20, /*height*/ 1,
|
||||
)),
|
||||
] {
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(next_area, Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer, Instant::now());
|
||||
view.render_composer_gap(next_area, Some(&tip), &mut buffer, Instant::now());
|
||||
assert!(view.handle_mouse(click, &[]).is_none());
|
||||
}
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer, Instant::now());
|
||||
view.show_copy_feedback(&Ok(CopyStatus::Confirmed), /*characters*/ 3);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer, Instant::now());
|
||||
assert!(view.handle_mouse(click, &[]).is_none());
|
||||
view.copy_feedback.as_mut().unwrap().expires_at = Instant::now();
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer);
|
||||
view.render_composer_gap(Some(area), Some(&tip), &mut buffer, Instant::now());
|
||||
assert!(matches!(
|
||||
view.handle_mouse(click, &[]),
|
||||
Some(ViewAction::OpenLink(_))
|
||||
|
||||
@@ -19,39 +19,11 @@ pub(super) struct FollowControl {
|
||||
|
||||
impl TranscriptView {
|
||||
pub(crate) fn render_follow_control(&mut self, area: Option<Rect>, buf: &mut Buffer) {
|
||||
let area = area.filter(|area| {
|
||||
!area.is_empty()
|
||||
&& !self.area.is_empty()
|
||||
&& !self.tail_visible
|
||||
&& self.highlight.is_none()
|
||||
&& self.selection.is_none()
|
||||
&& !self.is_search_active()
|
||||
});
|
||||
let Some(area) = area else {
|
||||
let Some(area) = area.filter(|area| !area.is_empty()) else {
|
||||
self.follow_control = FollowControl::default();
|
||||
return;
|
||||
};
|
||||
let labels = if self.unseen_activity {
|
||||
[
|
||||
" New activity · ↓ Back to bottom · esc ",
|
||||
" New activity · ↓ Bottom ",
|
||||
" New · ↓ Bottom ",
|
||||
" ↓ Bottom ",
|
||||
" ↓ ",
|
||||
]
|
||||
} else {
|
||||
[
|
||||
" ↓ Back to bottom · esc ",
|
||||
" ↓ Back to bottom ",
|
||||
" ↓ Bottom · esc ",
|
||||
" ↓ Bottom ",
|
||||
" ↓ ",
|
||||
]
|
||||
};
|
||||
let Some(label) = labels
|
||||
.into_iter()
|
||||
.find(|label| label.width() <= usize::from(area.width))
|
||||
else {
|
||||
let Some(label) = self.follow_control_label(area.width) else {
|
||||
self.follow_control = FollowControl::default();
|
||||
return;
|
||||
};
|
||||
@@ -77,6 +49,37 @@ impl TranscriptView {
|
||||
Paragraph::new(label).style(style).render(target, buf);
|
||||
}
|
||||
|
||||
pub(super) fn follow_control_label(&self, width: u16) -> Option<&'static str> {
|
||||
if self.area.is_empty()
|
||||
|| self.tail_visible
|
||||
|| self.highlight.is_some()
|
||||
|| self.selection.is_some()
|
||||
|| self.is_search_active()
|
||||
{
|
||||
return None;
|
||||
}
|
||||
let labels = if self.unseen_activity {
|
||||
[
|
||||
" New activity · ↓ Back to bottom · esc ",
|
||||
" New activity · ↓ Bottom ",
|
||||
" New · ↓ Bottom ",
|
||||
" ↓ Bottom ",
|
||||
" ↓ ",
|
||||
]
|
||||
} else {
|
||||
[
|
||||
" ↓ Back to bottom · esc ",
|
||||
" ↓ Back to bottom ",
|
||||
" ↓ Bottom · esc ",
|
||||
" ↓ Bottom ",
|
||||
" ↓ ",
|
||||
]
|
||||
};
|
||||
labels
|
||||
.into_iter()
|
||||
.find(|label| label.width() <= usize::from(width))
|
||||
}
|
||||
|
||||
pub(super) fn handle_follow_control_mouse(&mut self, event: MouseEvent) -> Option<ViewAction> {
|
||||
let control = &mut self.follow_control;
|
||||
let area = control.area?;
|
||||
|
||||
@@ -6,6 +6,7 @@ use crate::transcript_view::tests::cell;
|
||||
use crate::transcript_view::tests::render;
|
||||
use crate::transcript_view::tests::text;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::time::Instant;
|
||||
|
||||
#[test]
|
||||
fn right_click_retains_selection_until_confirmed_and_preserves_reading_position() {
|
||||
@@ -62,6 +63,7 @@ fn right_click_retains_selection_until_confirmed_and_preserves_reading_position(
|
||||
)),
|
||||
/*hint*/ None,
|
||||
&mut buffer,
|
||||
Instant::now(),
|
||||
);
|
||||
frames.push(format!("{result:?}\n{}", text(&buffer)));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user