From d3c68650c079917e873eb3347f7b04d0277c99dc Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 6 Mar 2025 02:41:13 +0200 Subject: [PATCH] Improve cmd-click in terminal to find more paths (#26174) Closes https://github.com/zed-industries/zed/issues/25701 Reworks the way cmd-click is handled: * first, all worktree entries are checked for existence This allows more fine-grained lookup of entries that are in the worktree, but their path in the terminal is not "full": in case neither `cwd` no worktree's root + that temrinal paths form a valid path (https://github.com/zed-industries/zed/issues/25701) The worktrees are sorted by "the most close to cwd first" so such files are attempted to resolved in the most specific worktree. This also fixes no cmd-click working in the remote ssh. * second, only if the client is local, do the FS checks to find non-indexed files Release Notes: - Improved cmd-click in terminal to find more paths --- Cargo.lock | 1 + crates/terminal_view/Cargo.toml | 4 + crates/terminal_view/src/terminal_view.rs | 364 ++++++++++++---------- 3 files changed, 203 insertions(+), 166 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 080a21e5c6d560..da5ad8e1cf13ca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13610,6 +13610,7 @@ dependencies = [ "gpui", "itertools 0.14.0", "language", + "log", "project", "rand 0.8.5", "schemars", diff --git a/crates/terminal_view/Cargo.toml b/crates/terminal_view/Cargo.toml index 57479bb54609eb..c62240a41f8ebc 100644 --- a/crates/terminal_view/Cargo.toml +++ b/crates/terminal_view/Cargo.toml @@ -27,6 +27,7 @@ futures.workspace = true gpui.workspace = true itertools.workspace = true language.workspace = true +log.workspace = true project.workspace = true task.workspace = true schemars.workspace = true @@ -50,3 +51,6 @@ gpui = { workspace = true, features = ["test-support"] } project = { workspace = true, features = ["test-support"] } rand.workspace = true workspace = { workspace = true, features = ["test-support"] } + +[package.metadata.cargo-machete] +ignored = ["log"] diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index 203620a5cb9889..027ceb4307d9b1 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -4,16 +4,15 @@ pub mod terminal_panel; pub mod terminal_scrollbar; pub mod terminal_tab_tooltip; -use collections::HashSet; use editor::{actions::SelectAll, scroll::ScrollbarAutoHide, Editor, EditorSettings}; -use futures::{stream::FuturesUnordered, StreamExt}; use gpui::{ anchored, deferred, div, impl_actions, AnyElement, App, DismissEvent, Entity, EventEmitter, FocusHandle, Focusable, KeyContext, KeyDownEvent, Keystroke, MouseButton, MouseDownEvent, Pixels, Render, ScrollWheelEvent, Stateful, Styled, Subscription, Task, WeakEntity, }; +use itertools::Itertools; use persistence::TERMINAL_DB; -use project::{search::SearchQuery, terminals::TerminalKind, Fs, Metadata, Project}; +use project::{search::SearchQuery, terminals::TerminalKind, Entry, Metadata, Project}; use schemars::JsonSchema; use terminal::{ alacritty_terminal::{ @@ -32,10 +31,7 @@ use terminal_tab_tooltip::TerminalTooltip; use ui::{ h_flex, prelude::*, ContextMenu, Icon, IconName, Label, Scrollbar, ScrollbarState, Tooltip, }; -use util::{ - paths::{PathWithPosition, SanitizedPath}, - ResultExt, -}; +use util::{debug_panic, paths::PathWithPosition, ResultExt}; use workspace::{ item::{ BreadcrumbText, Item, ItemEvent, SerializableItem, TabContentParams, TabTooltipContent, @@ -67,7 +63,7 @@ const REGEX_SPECIAL_CHARS: &[char] = &[ const CURSOR_BLINK_INTERVAL: Duration = Duration::from_millis(500); -const GIT_DIFF_PATH_PREFIXES: &[char] = &['a', 'b']; +const GIT_DIFF_PATH_PREFIXES: &[&str] = &["a", "b"]; /// Event to transmit the scroll from the element to the view #[derive(Clone, Debug, PartialEq)] @@ -876,20 +872,13 @@ fn subscribe_for_terminal_events( this.can_navigate_to_selected_word = match maybe_navigation_target { Some(MaybeNavigationTarget::Url(_)) => true, Some(MaybeNavigationTarget::PathLike(path_like_target)) => { - if let Ok(fs) = workspace.update(cx, |workspace, cx| { - workspace.project().read(cx).fs().clone() - }) { - let valid_files_to_open_task = possible_open_targets( - fs, - &workspace, - &path_like_target.terminal_dir, - &path_like_target.maybe_path, - cx, - ); - !smol::block_on(valid_files_to_open_task).is_empty() - } else { - false - } + let valid_files_to_open_task = possible_open_target( + &workspace, + &path_like_target.terminal_dir, + &path_like_target.maybe_path, + cx, + ); + smol::block_on(valid_files_to_open_task).is_some() } None => false, }; @@ -904,21 +893,11 @@ fn subscribe_for_terminal_events( return; } let task_workspace = workspace.clone(); - let Some(fs) = workspace - .update(cx, |workspace, cx| { - workspace.project().read(cx).fs().clone() - }) - .ok() - else { - return; - }; - let path_like_target = path_like_target.clone(); cx.spawn_in(window, |terminal_view, mut cx| async move { - let valid_files_to_open = terminal_view + let open_target = terminal_view .update(&mut cx, |_, cx| { - possible_open_targets( - fs, + possible_open_target( &task_workspace, &path_like_target.terminal_dir, &path_like_target.maybe_path, @@ -926,60 +905,60 @@ fn subscribe_for_terminal_events( ) })? .await; - let paths_to_open = valid_files_to_open - .iter() - .map(|(p, _)| p.path.clone()) - .collect(); - let opened_items = task_workspace - .update_in(&mut cx, |workspace, window, cx| { - workspace.open_paths( - paths_to_open, - OpenVisible::OnlyDirectories, - None, - window, - cx, - ) - }) - .context("workspace update")? - .await; + if let Some((path_to_open, open_target)) = open_target { + let opened_items = task_workspace + .update_in(&mut cx, |workspace, window, cx| { + workspace.open_paths( + vec![path_to_open.path.clone()], + OpenVisible::OnlyDirectories, + None, + window, + cx, + ) + }) + .context("workspace update")? + .await; + if opened_items.len() != 1 { + debug_panic!( + "Received {} items for one path {path_to_open:?}", + opened_items.len(), + ); + } - let mut has_dirs = false; - for ((path, metadata), opened_item) in valid_files_to_open - .into_iter() - .zip(opened_items.into_iter()) - { - if metadata.is_dir { - has_dirs = true; - } else if let Some(Ok(opened_item)) = opened_item { - if let Some(row) = path.row { - let col = path.column.unwrap_or(0); - if let Some(active_editor) = opened_item.downcast::() { - active_editor - .downgrade() - .update_in(&mut cx, |editor, window, cx| { - editor.go_to_singleton_buffer_point( - language::Point::new( - row.saturating_sub(1), - col.saturating_sub(1), - ), - window, - cx, - ) - }) - .log_err(); + if let Some(opened_item) = opened_items.first() { + if open_target.is_file() { + if let Some(Ok(opened_item)) = opened_item { + if let Some(row) = path_to_open.row { + let col = path_to_open.column.unwrap_or(0); + if let Some(active_editor) = + opened_item.downcast::() + { + active_editor + .downgrade() + .update_in(&mut cx, |editor, window, cx| { + editor.go_to_singleton_buffer_point( + language::Point::new( + row.saturating_sub(1), + col.saturating_sub(1), + ), + window, + cx, + ) + }) + .log_err(); + } + } } + } else if open_target.is_dir() { + task_workspace.update(&mut cx, |workspace, cx| { + workspace.project().update(cx, |_, cx| { + cx.emit(project::Event::ActivateProjectPanel); + }) + })?; } } } - if has_dirs { - task_workspace.update(&mut cx, |workspace, cx| { - workspace.project().update(cx, |_, cx| { - cx.emit(project::Event::ActivateProjectPanel); - }) - })?; - } - anyhow::Ok(()) }) .detach_and_log_err(cx) @@ -996,105 +975,158 @@ fn subscribe_for_terminal_events( vec![terminal_subscription, terminal_events_subscription] } -fn possible_open_paths_metadata( - fs: Arc, - row: Option, - column: Option, - potential_paths: HashSet, +#[derive(Debug, Clone)] +enum OpenTarget { + Worktree(Entry), + File(Metadata), +} + +impl OpenTarget { + fn is_file(&self) -> bool { + match self { + OpenTarget::Worktree(entry) => entry.is_file(), + OpenTarget::File(metadata) => !metadata.is_dir, + } + } + + fn is_dir(&self) -> bool { + match self { + OpenTarget::Worktree(entry) => entry.is_dir(), + OpenTarget::File(metadata) => metadata.is_dir, + } + } +} + +fn possible_open_target( + workspace: &WeakEntity, + cwd: &Option, + maybe_path: &str, cx: &mut Context, -) -> Task> { - cx.background_spawn(async move { - let mut canonical_paths = HashSet::default(); - for path in potential_paths { - if let Ok(canonical) = fs.canonicalize(&path).await { - let sanitized = SanitizedPath::from(canonical); - canonical_paths.insert(sanitized.as_path().to_path_buf()); - } else { - canonical_paths.insert(path); - } +) -> Task> { + let Some(workspace) = workspace.upgrade() else { + return Task::ready(None); + }; + // We have to check for both paths, as on Unix, certain paths with positions are valid file paths too. + // We can be on FS remote part, without real FS, so cannot canonicalize or check for existence the path right away. + let mut potential_paths = Vec::new(); + let original_path = PathWithPosition::from_path(PathBuf::from(maybe_path)); + let path_with_position = PathWithPosition::parse_str(maybe_path); + for prefix_str in GIT_DIFF_PATH_PREFIXES { + if let Some(stripped) = original_path.path.strip_prefix(prefix_str).ok() { + potential_paths.push(PathWithPosition { + path: stripped.to_owned(), + row: original_path.row, + column: original_path.column, + }); + } + if let Some(stripped) = path_with_position.path.strip_prefix(prefix_str).ok() { + potential_paths.push(PathWithPosition { + path: stripped.to_owned(), + row: path_with_position.row, + column: path_with_position.column, + }); } + } + potential_paths.insert(0, original_path); + potential_paths.insert(1, path_with_position); - let mut paths_with_metadata = Vec::with_capacity(canonical_paths.len()); + for worktree in workspace.read(cx).worktrees(cx).sorted_by_key(|worktree| { + let worktree_root = worktree.read(cx).abs_path(); + match cwd + .as_ref() + .and_then(|cwd| worktree_root.strip_prefix(cwd).ok()) + { + Some(cwd_child) => cwd_child.components().count(), + None => usize::MAX, + } + }) { + let worktree_root = worktree.read(cx).abs_path(); + let paths_to_check = potential_paths + .iter() + .map(|path_with_position| PathWithPosition { + path: path_with_position + .path + .strip_prefix(&worktree_root) + .unwrap_or(&path_with_position.path) + .to_owned(), + row: path_with_position.row, + column: path_with_position.column, + }) + .collect::>(); - let mut fetch_metadata_tasks = canonical_paths - .into_iter() - .map(|potential_path| async { - let metadata = fs.metadata(&potential_path).await.ok().flatten(); - ( + let mut traversal = worktree + .read(cx) + .traverse_from_path(true, true, false, "".as_ref()); + while let Some(entry) = traversal.next() { + if let Some(path_in_worktree) = paths_to_check + .iter() + .find(|path_to_check| entry.path.ends_with(&path_to_check.path)) + { + return Task::ready(Some(( PathWithPosition { - path: potential_path, - row, - column, + path: worktree_root.join(&entry.path), + row: path_in_worktree.row, + column: path_in_worktree.column, }, - metadata, - ) - }) - .collect::>(); - - while let Some((path, metadata)) = fetch_metadata_tasks.next().await { - if let Some(metadata) = metadata { - paths_with_metadata.push((path, metadata)); + OpenTarget::Worktree(entry.clone()), + ))); } } + } - paths_with_metadata - }) -} - -fn possible_open_targets( - fs: Arc, - workspace: &WeakEntity, - cwd: &Option, - maybe_path: &String, + if !workspace.read(cx).project().read(cx).is_local() { + return Task::ready(None); + } + let fs = workspace.read(cx).project().read(cx).fs().clone(); - cx: &mut Context, -) -> Task> { - let path_position = PathWithPosition::parse_str(maybe_path.as_str()); - let row = path_position.row; - let column = path_position.column; - let maybe_path = path_position.path; - - let potential_paths = if maybe_path.is_absolute() { - HashSet::from_iter([maybe_path]) - } else if maybe_path.starts_with("~") { - maybe_path - .strip_prefix("~") - .ok() - .and_then(|maybe_path| Some(dirs::home_dir()?.join(maybe_path))) - .map_or_else(HashSet::default, |p| HashSet::from_iter([p])) - } else { - let mut potential_cwd_and_workspace_paths = HashSet::default(); - if let Some(cwd) = cwd { - let abs_path = Path::join(cwd, &maybe_path); - potential_cwd_and_workspace_paths.insert(abs_path); - } - if let Some(workspace) = workspace.upgrade() { - workspace.update(cx, |workspace, cx| { - for potential_worktree_path in workspace - .worktrees(cx) - .map(|worktree| worktree.read(cx).abs_path().join(&maybe_path)) + let paths_to_check = potential_paths + .into_iter() + .flat_map(|path_to_check| { + let mut paths_to_check = Vec::new(); + let maybe_path = &path_to_check.path; + if maybe_path.starts_with("~") { + if let Some(home_path) = + maybe_path + .strip_prefix("~") + .ok() + .and_then(|stripped_maybe_path| { + Some(dirs::home_dir()?.join(stripped_maybe_path)) + }) { - potential_cwd_and_workspace_paths.insert(potential_worktree_path); + paths_to_check.push(PathWithPosition { + path: home_path, + row: path_to_check.row, + column: path_to_check.column, + }); } - - for prefix in GIT_DIFF_PATH_PREFIXES { - let prefix_str = &prefix.to_string(); - if maybe_path.starts_with(prefix_str) { - let stripped = maybe_path.strip_prefix(prefix_str).unwrap_or(&maybe_path); - for potential_worktree_path in workspace - .worktrees(cx) - .map(|worktree| worktree.read(cx).abs_path().join(&stripped)) - { - potential_cwd_and_workspace_paths.insert(potential_worktree_path); - } + } else { + paths_to_check.push(PathWithPosition { + path: maybe_path.clone(), + row: path_to_check.row, + column: path_to_check.column, + }); + if maybe_path.is_relative() { + if let Some(cwd) = &cwd { + paths_to_check.push(PathWithPosition { + path: cwd.join(maybe_path), + row: path_to_check.row, + column: path_to_check.column, + }); } } - }); - } - potential_cwd_and_workspace_paths - }; + } + paths_to_check + }) + .collect::>(); - possible_open_paths_metadata(fs, row, column, potential_paths, cx) + cx.background_spawn(async move { + for path_to_check in paths_to_check { + if let Some(metadata) = fs.metadata(&path_to_check.path).await.ok().flatten() { + return Some((path_to_check, OpenTarget::File(metadata))); + } + } + None + }) } fn regex_to_literal(regex: &str) -> String {