diff --git a/CHANGELOG.md b/CHANGELOG.md index 075bb9dd..1ef7f465 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -235,6 +235,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 already staged; the toast now carries the plan's actual reason ("Write a commit message first"), the same words the panel's own button shows on its tooltip. (#546) +- **Terminal pop-up menus no longer leak clicks into the grid behind them** — + the completion menu and the reverse-search menu (both the floating panel and + the input-bar row) inserted no hitbox of their own, so a press on one fell + straight through to the terminal: it cleared whatever was selected and + dragged out a new selection, merely moving over a row underlined the text + beneath it, and a Ctrl+click opened the link the menu was covering. All + three occlude now, so a press on a menu stops at the menu. (#541) +- **A file link that fails to open says so** — clicking a file path whose + opener is missing, or whose `link_file_command` template expands to nothing, + used to fail into a logfile line and nothing else; the click now raises the + same kind of toast a failed image upload does, naming the path and the + error. (#542) ## [26.8.3] - 2026-08-12 diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 3d1fc94d..843bc526 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -2810,6 +2810,29 @@ impl TerminalView { ); } + /// Same shape as `warn_image_upload_failed`: one toast per failed open, so + /// a broken `link_file_command` surfaces as a config problem instead of a + /// "dead link" (#542). Spawn is all that is reported — a spawned opener + /// that exits non-zero is nobody's to see. + fn warn_file_open_failed( + &self, + path: &std::path::Path, + reason: &std::io::Error, + window: &mut Window, + cx: &mut Context, + ) { + log::warn!("failed to open file link {}: {reason}", path.display()); + let path = path.display().to_string(); + let reason = reason.to_string(); + window.push_notification( + crate::ui::i18n::t_fmt( + crate::ui::i18n::L10nKey::LinkFileOpenFailed, + &[("path", path.as_str()), ("error", reason.as_str())], + ), + cx, + ); + } + pub fn clear_scrollback(&mut self, cx: &mut Context) { use alacritty_terminal::vte::ansi::{ClearMode, Handler as _}; @@ -4716,7 +4739,7 @@ impl TerminalView { is_dir, }, .., - ) => self.open_file_link(path, line, column, is_dir, cx), + ) => self.open_file_link(path, line, column, is_dir, window, cx), LinkAt::Unresolved { candidate, pending } => { return self.report_unresolved_link(&candidate, pending, window, cx); } @@ -4737,6 +4760,7 @@ impl TerminalView { line: Option, column: Option, is_dir: bool, + window: &mut Window, cx: &mut Context, ) { let cfg = cx.global::(); @@ -4752,19 +4776,30 @@ impl TerminalView { true => mode, false => LinkFileOpen::Internal, }; - match (mode, command) { + // Both external arms have to answer for a failed spawn (#542): a + // misspelled `link_file_command` or a missing opener used to hit only + // the log, and the link read as dead — while the path was only ever + // underlined because it verifiably exists. The built-in editor has + // its own error path downstream of OpenFileRequested. + let outcome = match (mode, command) { (LinkFileOpen::Command, Some(template)) => { run_file_command(&template, &path, line, column) } (LinkFileOpen::System, _) => open_file_path(&path), // Told to run a command, with no command left to run: falling back // to the built-in editor beats the click doing nothing. - (LinkFileOpen::Internal | LinkFileOpen::Command, _) => cx.emit(OpenFileRequested { - path, - line, - column, - is_dir, - }), + (LinkFileOpen::Internal | LinkFileOpen::Command, _) => { + cx.emit(OpenFileRequested { + path, + line, + column, + is_dir, + }); + return; + } + }; + if let Err(e) = outcome { + self.warn_file_open_failed(&path, &e, window, cx); } } @@ -5166,6 +5201,10 @@ impl TerminalView { .top(cy_top) .right_4() .h(self.line_height) + // The row floats over live grid cells; a bare div inserts no + // hitbox, so a click aimed at it started a selection in the + // text underneath (#541). + .occlude() .flex() .items_center() .font_family(self.font.family.clone()) @@ -5461,6 +5500,11 @@ impl TerminalView { .absolute() .left(x) .top(y) + // The menu has no click handlers of its own, and a handlerless + // element inserts no hitbox — so without this the press falls + // through to the grid: the selection there is cleared, or a + // modified click opens whatever link lies under the row (#541). + .occlude() .flex() .flex_col() .py_1() @@ -5594,6 +5638,10 @@ impl TerminalView { .absolute() .left(px(GRID_PAD_X)) .top(y) + // Same fall-through as the completion menu (#541): a bare div + // inserts no hitbox, so a click on a history row landed on the + // grid beneath it. + .occlude() .flex() .flex_col() .py_1() @@ -6064,7 +6112,7 @@ fn select_end_copy(enabled: bool, grid: bool, editor: bool) -> SelectEndCopy { /// Hands a path to whatever the OS has it associated with. Also the fallback /// for a directory the file tree cannot reach. -pub(crate) fn open_file_path(path: &std::path::Path) { +pub(crate) fn open_file_path(path: &std::path::Path) -> std::io::Result<()> { let opener = if cfg!(target_os = "macos") { "open" } else if cfg!(windows) { @@ -6072,9 +6120,8 @@ pub(crate) fn open_file_path(path: &std::path::Path) { } else { "xdg-open" }; - if let Err(e) = std::process::Command::new(opener).arg(path).spawn() { - log::warn!("failed to open {}: {e}", path.display()); - } + std::process::Command::new(opener).arg(path).spawn()?; + Ok(()) } fn run_file_command( @@ -6082,15 +6129,19 @@ fn run_file_command( path: &std::path::Path, line: Option, column: Option, -) { +) -> std::io::Result<()> { let argv = expand_file_command_template(template, path, line, column); let Some((program, args)) = argv.split_first() else { - log::warn!("link_file_command is empty; ignoring file link"); - return; + // Sanitize maps a blank template to None, so the only way here is a + // template whose tokens all expand to nothing (a lone `{line}` on a + // link with no line number). Still a config error worth reporting. + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "link_file_command expanded to nothing", + )); }; - if let Err(e) = std::process::Command::new(program).args(args).spawn() { - log::warn!("failed to run link_file_command {template:?}: {e}"); - } + std::process::Command::new(program).args(args).spawn()?; + Ok(()) } fn expand_file_command_template( @@ -7065,6 +7116,18 @@ mod tests { assert_eq!(argv, vec!["code", "--goto", "{other}"]); } + /// #542's contract: a failed open is an `Err` the click site can toast, + /// not a line in a logfile nobody is watching. + #[test] + fn a_file_command_that_cannot_spawn_comes_back_as_an_error() { + let path = Path::new("/tmp/wherever.rs"); + // The binary does not exist, so the spawn itself fails. + assert!(super::run_file_command("tty7-no-such-binary {path}", path, None, None).is_err()); + // A template whose tokens all expand to nothing is a config error, + // not a silent no-op. + assert!(super::run_file_command("{line}", path, None, None).is_err()); + } + #[test] fn clipboard_image_transcodes_bmp_to_png_and_passes_png_through() { use gpui::{Image, ImageFormat}; @@ -11739,4 +11802,104 @@ mod gpui_tests { }) .unwrap(); } + + /// #541: the history menu floats over live grid cells, and a `div` that + /// carries no handler of its own inserts no hitbox — so the press went + /// straight through to the grid behind the menu, which cleared the + /// selection, dragged out a new one, or (with the link modifier held) + /// opened whatever link the menu was covering. + /// + /// The second half is the other side of the pair `src/ui/app.rs` keeps for + /// the resize handle: with the menu gone the very same press must still + /// reach the grid, or this would pass on a pane that never sees a mouse. + #[gpui::test] + fn a_press_on_the_history_menu_never_reaches_the_grid(cx: &mut TestAppContext) { + use gpui::{MouseMoveEvent, PlatformInput}; + + crate::core::config::pin_test_config_dir(); + let (window, mut daemon) = harness(cx); + prompt_ready(&window, cx, &mut daemon); + wait_for_input_active(&window, cx); + + window + .update(cx, |view, window, cx| { + window.activate_window(); + view.focus_handle.focus(window, cx); + view.history = vec!["echo one".to_string(), "echo two".to_string()]; + view.history_frecency = vec![1.0, 1.0]; + view.start_reverse_search(); + cx.notify(); + }) + .unwrap(); + + let mut vcx = gpui::VisualTestContext::from_window(window.into(), cx); + vcx.update(|window, _| window.refresh()); + vcx.run_until_parked(); + + // The menu is laid out one row under the cursor, plus the gap + // `render_reverse_search_menu` leaves; this lands in the middle of its + // first row, where a candidate is drawn. + let lh = window.update(cx, |view, _, _| view.line_height).unwrap(); + let at = point( + px(GRID_PAD_X) + px(10.), + px(GRID_PAD_Y) + lh * 2.0 + px(10.), + ); + + let press = |vcx: &mut gpui::VisualTestContext| { + vcx.update(|window, cx| { + window.dispatch_event( + PlatformInput::MouseMove(MouseMoveEvent { + position: at, + pressed_button: None, + modifiers: Modifiers::none(), + }), + cx, + ); + window.dispatch_event( + PlatformInput::MouseDown(MouseDownEvent { + button: MouseButton::Left, + position: at, + modifiers: Modifiers::none(), + click_count: 1, + first_mouse: false, + }), + cx, + ); + }); + vcx.run_until_parked(); + }; + + press(&mut vcx); + window + .update(cx, |view, _, _| { + assert!( + view.reverse_search.is_some(), + "the press must not have closed the menu either" + ); + assert!( + !view.selecting, + "the menu swallowed the press, so no selection started under it" + ); + }) + .unwrap(); + + window + .update(cx, |view, _, cx| { + view.reverse_search = None; + cx.notify(); + }) + .unwrap(); + vcx.update(|window, _| window.refresh()); + vcx.run_until_parked(); + + press(&mut vcx); + window + .update(cx, |view, _, _| { + assert!( + view.selecting, + "with no menu over it the same press is the grid's to take" + ); + }) + .unwrap(); + } } diff --git a/src/ui/file_tree.rs b/src/ui/file_tree.rs index e594548a..6794b2e7 100644 --- a/src/ui/file_tree.rs +++ b/src/ui/file_tree.rs @@ -1024,7 +1024,21 @@ impl Tty7App { // another machine, where handing the name to a local file manager // would open whatever this one keeps at that path, or nothing. if self.can_spawn_locally(cx) { - crate::terminal::view::open_file_path(path); + // The OS association can fail to spawn like any other opener + // (#542): say so with the same words a failed file link uses. + if let Err(e) = crate::terminal::view::open_file_path(path) { + log::warn!("failed to open {}: {e}", path.display()); + window.push_notification( + t_fmt( + L10nKey::LinkFileOpenFailed, + &[ + ("path", &path.display().to_string()), + ("error", &e.to_string()), + ], + ), + cx, + ); + } } else { window.push_notification( t_fmt( diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 1e6e4470..a035a8e0 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -892,6 +892,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SftpImagePasteUploadFailed => { "Could not upload the pasted image to {host}: {error}" } + L10nKey::LinkFileOpenFailed => "Could not open {path}: {error}", L10nKey::ForwardPanelTitle => "Forwards", L10nKey::ForwardDisconnected => "Disconnected", L10nKey::ForwardDisconnectedFrom => "Disconnected from {host}", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 4d42121d..c46c02f5 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -942,6 +942,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SftpImagePasteUploadFailed => { "貼り付けた画像を {host} にアップロードできませんでした: {error}" } + L10nKey::LinkFileOpenFailed => "{path} を開けませんでした: {error}", L10nKey::ForwardPanelTitle => "ポートフォワード", L10nKey::ForwardDisconnected => "切断済み", L10nKey::ForwardDisconnectedFrom => "{host} から切断されました", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index a06915e0..441ebcb8 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -662,6 +662,7 @@ l10n_keys! { SftpTransferError, SftpTransferListFailed, SftpImagePasteUploadFailed, + LinkFileOpenFailed, ForwardPanelTitle, ForwardDisconnected, ForwardDisconnectedFrom, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index e2fea02d..d6ad26ea 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -848,6 +848,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SftpTransferError => "错误", L10nKey::SftpTransferListFailed => "无法获取传输状态：{error}", L10nKey::SftpImagePasteUploadFailed => "无法将粘贴的图片上传到 {host}：{error}", + L10nKey::LinkFileOpenFailed => "无法打开 {path}：{error}", L10nKey::ForwardPanelTitle => "端口转发", L10nKey::ForwardDisconnected => "已断开", L10nKey::ForwardDisconnectedFrom => "与 {host} 的连接已断开",