From 8d6abf65375089b2aa2b1c4536802639c26cce23 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 12 Mar 2025 00:17:12 +0200 Subject: [PATCH] Improve terminal hover tooltips (#26487) Follow-up of https://github.com/zed-industries/zed/pull/26174 * Fixes `./path/foo.bar` not properly parsed as valid open target * Shows full open target's path in cmd-hover tooltips Before: before_1 before_2 After: after_1 after_2 Release Notes: - N/A --- crates/terminal_view/src/terminal_element.rs | 28 ++--- crates/terminal_view/src/terminal_view.rs | 104 ++++++++++++------- 2 files changed, 79 insertions(+), 53 deletions(-) diff --git a/crates/terminal_view/src/terminal_element.rs b/crates/terminal_view/src/terminal_element.rs index 26b1e7ac03fdc4c316806b7193763b9079a5b423..1f39e64b552aae4ce84bb115d728024085bec7c4 100644 --- a/crates/terminal_view/src/terminal_element.rs +++ b/crates/terminal_view/src/terminal_element.rs @@ -21,7 +21,7 @@ use terminal::{ }, }, terminal_settings::TerminalSettings, - HoveredWord, IndexedCell, Terminal, TerminalBounds, TerminalContent, + IndexedCell, Terminal, TerminalBounds, TerminalContent, }; use theme::{ActiveTheme, Theme, ThemeSettings}; use ui::{ParentElement, Tooltip}; @@ -45,7 +45,6 @@ pub struct LayoutState { display_offset: usize, hyperlink_tooltip: Option, gutter: Pixels, - last_hovered_word: Option, block_below_cursor_element: Option, } @@ -157,7 +156,6 @@ pub struct TerminalElement { focus: FocusHandle, focused: bool, cursor_visible: bool, - can_navigate_to_selected_word: bool, interactivity: Interactivity, block_below_cursor: Option>, } @@ -178,7 +176,6 @@ impl TerminalElement { focus: FocusHandle, focused: bool, cursor_visible: bool, - can_navigate_to_selected_word: bool, block_below_cursor: Option>, ) -> TerminalElement { TerminalElement { @@ -188,7 +185,6 @@ impl TerminalElement { focused, focus: focus.clone(), cursor_visible, - can_navigate_to_selected_word, block_below_cursor, interactivity: Default::default(), } @@ -695,27 +691,29 @@ impl Element for TerminalElement { let background_color = theme.colors().terminal_background; - let last_hovered_word = self.terminal.update(cx, |terminal, cx| { + let (last_hovered_word, hover_target) = self.terminal.update(cx, |terminal, cx| { terminal.set_size(dimensions); terminal.sync(window, cx); - if self.can_navigate_to_selected_word - && window.modifiers().secondary() + if window.modifiers().secondary() && bounds.contains(&window.mouse_position()) + && self.terminal_view.read(cx).hover_target_tooltip.is_some() { - terminal.last_content.last_hovered_word.clone() + let hover_target = self.terminal_view.read(cx).hover_target_tooltip.clone(); + let last_hovered_word = terminal.last_content.last_hovered_word.clone(); + (last_hovered_word, hover_target) } else { - None + (None, None) } }); let scroll_top = self.terminal_view.read(cx).scroll_top; - let hyperlink_tooltip = last_hovered_word.clone().map(|hovered_word| { + let hyperlink_tooltip = hover_target.as_ref().map(|hover_target| { let offset = bounds.origin + point(gutter, px(0.)) - point(px(0.), scroll_top); let mut element = div() .size_full() .id("terminal-element") - .tooltip(Tooltip::text(hovered_word.word.clone())) + .tooltip(Tooltip::text(hover_target.clone())) .into_any_element(); element.prepaint_as_root(offset, bounds.size.into(), window, cx); element @@ -851,7 +849,6 @@ impl Element for TerminalElement { display_offset, hyperlink_tooltip, gutter, - last_hovered_word, block_below_cursor_element, } }, @@ -884,7 +881,10 @@ impl Element for TerminalElement { }; self.register_mouse_listeners(layout.mode, &layout.hitbox, window); - if self.can_navigate_to_selected_word && layout.last_hovered_word.is_some() { + if window.modifiers().secondary() + && bounds.contains(&window.mouse_position()) + && self.terminal_view.read(cx).hover_target_tooltip.is_some() + { window.set_cursor_style(gpui::CursorStyle::PointingHand, &layout.hitbox); } else { window.set_cursor_style(gpui::CursorStyle::IBeam, &layout.hitbox); diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index 7db75af327ea5dc444d1c1467eed6caac71e5527..e810f7cffa75ba14a836d58b719b846878aecb96 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -114,7 +114,7 @@ pub struct TerminalView { blinking_terminal_enabled: bool, blinking_paused: bool, blink_epoch: usize, - can_navigate_to_selected_word: bool, + hover_target_tooltip: Option, workspace_id: Option, show_breadcrumbs: bool, block_below_cursor: Option>, @@ -196,7 +196,7 @@ impl TerminalView { blinking_terminal_enabled: false, blinking_paused: false, blink_epoch: 0, - can_navigate_to_selected_word: false, + hover_target_tooltip: None, workspace_id, show_breadcrumbs: TerminalSettings::get_global(cx).toolbar.breadcrumbs, block_below_cursor: None, @@ -869,19 +869,25 @@ fn subscribe_for_terminal_events( } Event::NewNavigationTarget(maybe_navigation_target) => { - this.can_navigate_to_selected_word = match maybe_navigation_target { - Some(MaybeNavigationTarget::Url(_)) => true, - Some(MaybeNavigationTarget::PathLike(path_like_target)) => { - 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, - }; + this.hover_target_tooltip = + maybe_navigation_target + .as_ref() + .and_then(|navigation_target| match navigation_target { + MaybeNavigationTarget::Url(url) => Some(url.clone()), + MaybeNavigationTarget::PathLike(path_like_target) => { + let valid_files_to_open_task = possible_open_target( + &workspace, + &path_like_target.terminal_dir, + &path_like_target.maybe_path, + cx, + ); + Some(match smol::block_on(valid_files_to_open_task)? { + OpenTarget::File(path, _) | OpenTarget::Worktree(path, _) => { + path.to_string(|path| path.to_string_lossy().to_string()) + } + }) + } + }); cx.notify() } @@ -889,7 +895,7 @@ fn subscribe_for_terminal_events( MaybeNavigationTarget::Url(url) => cx.open_url(url), MaybeNavigationTarget::PathLike(path_like_target) => { - if !this.can_navigate_to_selected_word { + if this.hover_target_tooltip.is_none() { return; } let task_workspace = workspace.clone(); @@ -905,7 +911,8 @@ fn subscribe_for_terminal_events( ) })? .await; - if let Some((path_to_open, open_target)) = open_target { + if let Some(open_target) = open_target { + let path_to_open = open_target.path(); let opened_items = task_workspace .update_in(&mut cx, |workspace, window, cx| { workspace.open_paths( @@ -980,22 +987,29 @@ fn subscribe_for_terminal_events( #[derive(Debug, Clone)] enum OpenTarget { - Worktree(Entry), - File(Metadata), + Worktree(PathWithPosition, Entry), + File(PathWithPosition, Metadata), } impl OpenTarget { fn is_file(&self) -> bool { match self { - OpenTarget::Worktree(entry) => entry.is_file(), - OpenTarget::File(metadata) => !metadata.is_dir, + 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, + OpenTarget::Worktree(_, entry) => entry.is_dir(), + OpenTarget::File(_, metadata) => metadata.is_dir, + } + } + + fn path(&self) -> &PathWithPosition { + match self { + OpenTarget::Worktree(path, _) => path, + OpenTarget::File(path, _) => path, } } } @@ -1005,7 +1019,7 @@ fn possible_open_target( cwd: &Option, maybe_path: &str, cx: &mut Context, -) -> Task> { +) -> Task> { let Some(workspace) = workspace.upgrade() else { return Task::ready(None); }; @@ -1014,7 +1028,22 @@ fn possible_open_target( 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 { + let worktree_candidates = 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, + } + }) + .collect::>(); + // Since we do not check paths via FS and joining, we need to strip off potential `./`, `a/`, `b/` prefixes out of it. + for prefix_str in GIT_DIFF_PATH_PREFIXES.iter().chain(std::iter::once(&".")) { if let Some(stripped) = original_path.path.strip_prefix(prefix_str).ok() { potential_paths.push(PathWithPosition { path: stripped.to_owned(), @@ -1033,16 +1062,7 @@ fn possible_open_target( potential_paths.insert(0, original_path); potential_paths.insert(1, path_with_position); - 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, - } - }) { + for worktree in &worktree_candidates { let worktree_root = worktree.read(cx).abs_path(); let paths_to_check = potential_paths .iter() @@ -1065,13 +1085,13 @@ fn possible_open_target( .iter() .find(|path_to_check| entry.path.ends_with(&path_to_check.path)) { - return Task::ready(Some(( + return Task::ready(Some(OpenTarget::Worktree( PathWithPosition { path: worktree_root.join(&entry.path), row: path_in_worktree.row, column: path_in_worktree.column, }, - OpenTarget::Worktree(entry.clone()), + entry.clone(), ))); } } @@ -1116,6 +1136,13 @@ fn possible_open_target( column: path_to_check.column, }); } + for worktree in &worktree_candidates { + paths_to_check.push(PathWithPosition { + path: worktree.read(cx).abs_path().join(maybe_path), + row: path_to_check.row, + column: path_to_check.column, + }); + } } } paths_to_check @@ -1125,7 +1152,7 @@ fn possible_open_target( 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))); + return Some(OpenTarget::File(path_to_check, metadata)); } } None @@ -1247,7 +1274,6 @@ impl Render for TerminalView { self.focus_handle.clone(), focused, self.should_show_cursor(focused, cx), - self.can_navigate_to_selected_word, self.block_below_cursor.clone(), )) .when_some(self.render_scrollbar(cx), |div, scrollbar| {