From 978951b79a418ac9fd29b55e86b3210f0af27917 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sat, 4 Oct 2025 00:49:33 -0400 Subject: [PATCH] Don't use PTY in the display-only terminal (#39510) This only affects `codex-acp` for now. Not using the PTY in display-only terminals means they don't display the login prompt (or spurious `%`s) at the end of terminal output renderings. Release Notes: - N/A --- crates/acp_thread/src/acp_thread.rs | 4 - crates/agent_servers/src/acp.rs | 2 - crates/agent_ui/src/acp/thread_view.rs | 2 +- crates/assistant_tools/src/terminal_tool.rs | 2 +- crates/debugger_ui/src/session/running.rs | 1 - crates/terminal/src/terminal.rs | 165 +++++++++++++++----- crates/terminal_view/src/terminal_view.rs | 2 +- 7 files changed, 127 insertions(+), 51 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 607e2af2c0bfb5be23db9ba1fd77838ce931d659..ed4441b9cdd28978dd8a74385824193e7e3df2a5 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -2331,12 +2331,10 @@ mod tests { // Create a display-only terminal and then send Created let lower = cx.new(|cx| { let builder = ::terminal::TerminalBuilder::new_display_only( - None, ::terminal::terminal_settings::CursorShape::default(), ::terminal::terminal_settings::AlternateScroll::On, None, 0, - cx, ) .unwrap(); builder.subscribe(cx) @@ -2410,12 +2408,10 @@ mod tests { // Now create a display-only lower-level terminal and send Created let lower = cx.new(|cx| { let builder = ::terminal::TerminalBuilder::new_display_only( - None, ::terminal::terminal_settings::CursorShape::default(), ::terminal::terminal_settings::AlternateScroll::On, None, 0, - cx, ) .unwrap(); builder.subscribe(cx) diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index 1c8b34a0e5669c5249559ccf5f885fecd1f622f7..2b977d60df54e32daf967678bc77fba3566edbbd 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -717,12 +717,10 @@ impl acp::Client for ClientDelegate { // Create a minimal display-only lower-level terminal and register it. let _ = session.thread.update(&mut self.cx.clone(), |thread, cx| { let builder = TerminalBuilder::new_display_only( - None, CursorShape::default(), AlternateScroll::On, None, 0, - cx, )?; let lower = cx.new(|cx| builder.subscribe(cx)); thread.on_terminal_provider_event( diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index 1f156376c596159816fdb6edfcc53efed5c9e24c..0bf75d7dd098fd0ccee5db90a551d72d74a08e67 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -2716,7 +2716,7 @@ impl AcpThreadView { let working_dir = working_dir .as_ref() - .map(|path| format!("{}", path.display())) + .map(|path| path.display().to_string()) .unwrap_or_else(|| "current directory".to_string()); let is_expanded = self.expanded_tool_calls.contains(&tool_call.id); diff --git a/crates/assistant_tools/src/terminal_tool.rs b/crates/assistant_tools/src/terminal_tool.rs index e98d1b87d30c3544e266024a25cc52139208618b..b38c811faa494af8223a07d1f339934241956c47 100644 --- a/crates/assistant_tools/src/terminal_tool.rs +++ b/crates/assistant_tools/src/terminal_tool.rs @@ -477,7 +477,7 @@ impl ToolCard for TerminalToolCard { .as_ref() .cloned() .or_else(|| env::current_dir().ok()) - .map(|path| format!("{}", path.display())) + .map(|path| path.display().to_string()) .unwrap_or_else(|| "current directory".to_string()); let header = h_flex() diff --git a/crates/debugger_ui/src/session/running.rs b/crates/debugger_ui/src/session/running.rs index c9272601da0459fc48baf5115ddc2458c58ae5da..fe8cf083fa885246197707dc0783b7f327b57fa8 100644 --- a/crates/debugger_ui/src/session/running.rs +++ b/crates/debugger_ui/src/session/running.rs @@ -1232,7 +1232,6 @@ impl RunningState { terminal.read_with(cx, |terminal, _| { terminal - .pty_info .pid() .map(|pid| pid.as_u32()) .context("Terminal was spawned but PID was not available") diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index da0d686710be5e51f63304c4c2533b5f52273939..91c972ed8d3f01633ba9c628739a202bd625a494 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -41,7 +41,7 @@ use mappings::mouse::{ use collections::{HashMap, VecDeque}; use futures::StreamExt; -use pty_info::PtyProcessInfo; +use pty_info::{ProcessIdGetter, PtyProcessInfo}; use serde::{Deserialize, Serialize}; use settings::Settings; use smol::channel::{Receiver, Sender}; @@ -340,27 +340,74 @@ pub struct TerminalBuilder { impl TerminalBuilder { pub fn new_display_only( - working_directory: Option, cursor_shape: CursorShape, alternate_scroll: AlternateScroll, max_scroll_history_lines: Option, window_id: u64, - cx: &App, ) -> Result { - Self::new( - working_directory, - None, - Shell::System, - HashMap::default(), - cursor_shape, - alternate_scroll, - max_scroll_history_lines, - false, - window_id, - None, - cx, - Vec::new(), - ) + // Create a display-only terminal (no actual PTY). + let default_cursor_style = AlacCursorStyle::from(cursor_shape); + let scrolling_history = max_scroll_history_lines + .unwrap_or(DEFAULT_SCROLL_HISTORY_LINES) + .min(MAX_SCROLL_HISTORY_LINES); + let config = Config { + scrolling_history, + default_cursor_style, + ..Config::default() + }; + + let (events_tx, events_rx) = unbounded(); + let mut term = Term::new( + config.clone(), + &TerminalBounds::default(), + ZedListener(events_tx), + ); + + if let AlternateScroll::Off = alternate_scroll { + term.unset_private_mode(PrivateMode::Named(NamedPrivateMode::AlternateScroll)); + } + + let term = Arc::new(FairMutex::new(term)); + + let terminal = Terminal { + task: None, + terminal_type: TerminalType::DisplayOnly, + completion_tx: None, + term, + term_config: config, + title_override: None, + events: VecDeque::with_capacity(10), + last_content: Default::default(), + last_mouse: None, + matches: Vec::new(), + selection_head: None, + breadcrumb_text: String::new(), + scroll_px: px(0.), + next_link_id: 0, + selection_phase: SelectionPhase::Ended, + hyperlink_regex_searches: RegexSearches::new(), + vi_mode_enabled: false, + is_ssh_terminal: false, + last_mouse_move_time: Instant::now(), + last_hyperlink_search_position: None, + #[cfg(windows)] + shell_program: None, + activation_script: Vec::new(), + template: CopyTemplate { + shell: Shell::System, + env: HashMap::default(), + cursor_shape, + alternate_scroll, + max_scroll_history_lines, + window_id, + }, + child_exited: None, + }; + + Ok(TerminalBuilder { + terminal, + events_rx, + }) } pub fn new( working_directory: Option, @@ -533,7 +580,10 @@ impl TerminalBuilder { let mut terminal = Terminal { task, - pty_tx: Notifier(pty_tx), + terminal_type: TerminalType::Pty { + pty_tx: Notifier(pty_tx), + info: pty_info, + }, completion_tx, term, term_config: config, @@ -543,12 +593,10 @@ impl TerminalBuilder { last_mouse: None, matches: Vec::new(), selection_head: None, - pty_info, breadcrumb_text: String::new(), scroll_px: px(0.), next_link_id: 0, selection_phase: SelectionPhase::Ended, - // hovered_word: false, hyperlink_regex_searches: RegexSearches::new(), vi_mode_enabled: false, is_ssh_terminal, @@ -733,8 +781,16 @@ pub enum SelectionPhase { Ended, } +enum TerminalType { + Pty { + pty_tx: Notifier, + info: PtyProcessInfo, + }, + DisplayOnly, +} + pub struct Terminal { - pty_tx: Notifier, + terminal_type: TerminalType, completion_tx: Option>>, term: Arc>>, term_config: Config, @@ -745,7 +801,6 @@ pub struct Terminal { pub last_content: TerminalContent, pub selection_head: Option, pub breadcrumb_text: String, - pub pty_info: PtyProcessInfo, title_override: Option, scroll_px: Pixels, next_link_id: usize, @@ -862,8 +917,10 @@ impl Terminal { AlacTermEvent::Wakeup => { cx.emit(Event::Wakeup); - if self.pty_info.has_changed() { - cx.emit(Event::TitleChanged); + if let TerminalType::Pty { info, .. } = &mut self.terminal_type { + if info.has_changed() { + cx.emit(Event::TitleChanged); + } } } AlacTermEvent::ColorRequest(index, format) => { @@ -905,7 +962,9 @@ impl Terminal { self.last_content.terminal_bounds = new_bounds; - self.pty_tx.0.send(Msg::Resize(new_bounds.into())).ok(); + if let TerminalType::Pty { pty_tx, .. } = &self.terminal_type { + pty_tx.0.send(Msg::Resize(new_bounds.into())).ok(); + } term.resize(new_bounds); } @@ -1288,9 +1347,12 @@ impl Terminal { } } - ///Write the Input payload to the tty. + /// Write the Input payload to the PTY, if applicable. + /// (This is a no-op for display-only terminals.) fn write_to_pty(&self, input: impl Into>) { - self.pty_tx.notify(input.into()); + if let TerminalType::Pty { pty_tx, .. } = &self.terminal_type { + pty_tx.notify(input.into()); + } } pub fn input(&mut self, input: impl Into>) { @@ -1594,7 +1656,7 @@ impl Terminal { && let Some(bytes) = mouse_moved_report(point, e.pressed_button, e.modifiers, self.last_content.mode) { - self.pty_tx.notify(bytes); + self.write_to_pty(bytes); } } else if e.modifiers.secondary() { self.word_from_position(e.position); @@ -1701,7 +1763,7 @@ impl Terminal { if let Some(bytes) = mouse_button_report(point, e.button, e.modifiers, true, self.last_content.mode) { - self.pty_tx.notify(bytes); + self.write_to_pty(bytes); } } else { match e.button { @@ -1760,7 +1822,7 @@ impl Terminal { if let Some(bytes) = mouse_button_report(point, e.button, e.modifiers, false, self.last_content.mode) { - self.pty_tx.notify(bytes); + self.write_to_pty(bytes); } } else { if e.button == MouseButton::Left && setting.copy_on_select { @@ -1799,7 +1861,7 @@ impl Terminal { if let Some(scrolls) = scroll_report(point, scroll_lines, e, self.last_content.mode) { for scroll in scrolls { - self.pty_tx.notify(scroll); + self.write_to_pty(scroll); } }; } else if self @@ -1808,7 +1870,7 @@ impl Terminal { .contains(TermMode::ALT_SCREEN | TermMode::ALTERNATE_SCROLL) && !e.shift { - self.pty_tx.notify(alt_scroll(scroll_lines)) + self.write_to_pty(alt_scroll(scroll_lines)); } else if scroll_lines != 0 { let scroll = AlacScroll::Delta(scroll_lines); @@ -1879,10 +1941,12 @@ impl Terminal { /// This does *not* return the working directory of the shell that runs on the /// remote host, in case Zed is connected to a remote host. fn client_side_working_directory(&self) -> Option { - self.pty_info - .current - .as_ref() - .map(|process| process.cwd.clone()) + match &self.terminal_type { + TerminalType::Pty { info, .. } => { + info.current.as_ref().map(|process| process.cwd.clone()) + } + TerminalType::DisplayOnly => None, + } } pub fn title(&self, truncate: bool) -> String { @@ -1899,8 +1963,8 @@ impl Terminal { .title_override .as_ref() .map(|title_override| title_override.to_string()) - .unwrap_or_else(|| { - self.pty_info + .unwrap_or_else(|| match &self.terminal_type { + TerminalType::Pty { info, .. } => info .current .as_ref() .map(|fpi| { @@ -1930,7 +1994,8 @@ impl Terminal { }; format!("{process_file} — {process_name}") }) - .unwrap_or_else(|| "Terminal".to_string()) + .unwrap_or_else(|| "Terminal".to_string()), + TerminalType::DisplayOnly => "Terminal".to_string(), }), } } @@ -1939,7 +2004,23 @@ impl Terminal { if let Some(task) = self.task() && task.status == TaskStatus::Running { - self.pty_info.kill_current_process(); + if let TerminalType::Pty { info, .. } = &mut self.terminal_type { + info.kill_current_process(); + } + } + } + + pub fn pid(&self) -> Option { + match &self.terminal_type { + TerminalType::Pty { info, .. } => info.pid(), + TerminalType::DisplayOnly => None, + } + } + + pub fn pid_getter(&self) -> Option<&ProcessIdGetter> { + match &self.terminal_type { + TerminalType::Pty { info, .. } => Some(info.pid_getter()), + TerminalType::DisplayOnly => None, } } @@ -2134,7 +2215,9 @@ unsafe fn append_text_to_term(term: &mut Term, text_lines: &[&str]) impl Drop for Terminal { fn drop(&mut self) { - self.pty_tx.0.send(Msg::Shutdown).ok(); + if let TerminalType::Pty { pty_tx, .. } = &self.terminal_type { + pty_tx.0.send(Msg::Shutdown).ok(); + } } } diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index ca6b7c056225c1d0587ade0e5131d942f7256603..a99d68609a6bef5b449c7217a1613846d7c4b9ce 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -1142,7 +1142,7 @@ impl Item for TerminalView { fn tab_tooltip_content(&self, cx: &App) -> Option { let terminal = self.terminal().read(cx); let title = terminal.title(false); - let pid = terminal.pty_info.pid_getter().fallback_pid(); + let pid = terminal.pid_getter()?.fallback_pid(); Some(TabTooltipContent::Custom(Box::new(move |_window, cx| { cx.new(|_| TerminalTooltip::new(title.clone(), pid)).into()