From f6c81a0595f518bf2f245a199248d50245afacac Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Fri, 28 Mar 2025 11:26:54 +0100 Subject: [PATCH] assistant2: Unify path rendering for file context (#27537) This ensures that we render path matches the same in both the file context picker and the messag editor completion provider. Release Notes: - N/A --- .../src/context_picker/completion_provider.rs | 198 ++++++------------ .../src/context_picker/file_context_picker.rs | 37 ++-- 2 files changed, 93 insertions(+), 142 deletions(-) diff --git a/crates/assistant2/src/context_picker/completion_provider.rs b/crates/assistant2/src/context_picker/completion_provider.rs index 20d16970a74ad651fdbef40e19ceaa2bb170e6c1..8f59a0d34f6421e03d87ae69ad02f260ab266186 100644 --- a/crates/assistant2/src/context_picker/completion_provider.rs +++ b/crates/assistant2/src/context_picker/completion_provider.rs @@ -1,6 +1,6 @@ use std::cell::RefCell; use std::ops::Range; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::rc::Rc; use std::sync::atomic::AtomicBool; use std::sync::Arc; @@ -70,18 +70,18 @@ impl ContextPickerCompletionProvider { .filter_map(|entry| match entry { super::RecentEntry::File { project_path, - path_prefix: _, - } => Self::completion_for_path( + path_prefix, + } => Some(Self::completion_for_path( project_path.clone(), + path_prefix, true, false, excerpt_id, source_range.clone(), editor.clone(), context_store.clone(), - workspace.clone(), cx, - ), + )), super::RecentEntry::Thread(thread_context_entry) => { let thread_store = thread_store .as_ref() @@ -120,56 +120,24 @@ impl ContextPickerCompletionProvider { completions } - fn full_path_for_entry( - worktree_id: WorktreeId, - path: &Path, - workspace: Entity, - cx: &App, - ) -> Option { - let worktree = workspace - .read(cx) - .project() - .read(cx) - .worktree_for_id(worktree_id, cx)? - .read(cx); - - let mut full_path = PathBuf::from(worktree.root_name()); - full_path.push(path); - Some(full_path) - } - fn build_code_label_for_full_path( - worktree_id: WorktreeId, - path: &Path, - workspace: Entity, + file_name: &str, + directory: Option<&str>, cx: &App, - ) -> Option { + ) -> CodeLabel { let comment_id = cx.theme().syntax().highlight_id("comment").map(HighlightId); let mut label = CodeLabel::default(); - let worktree = workspace - .read(cx) - .project() - .read(cx) - .worktree_for_id(worktree_id, cx)?; - - let entry = worktree.read(cx).entry_for_path(&path)?; - let file_name = path.file_name()?.to_string_lossy(); + label.push_str(&file_name, None); - if entry.is_dir() { - label.push_str("/ ", None); - } else { - label.push_str(" ", None); - }; + label.push_str(" ", None); - let mut path_hint = PathBuf::from(worktree.read(cx).root_name()); - if let Some(path_to_entry) = path.parent() { - path_hint.push(path_to_entry); + if let Some(directory) = directory { + label.push_str(&directory, comment_id); } - label.push_str(&path_hint.to_string_lossy(), comment_id); label.filter_range = 0..label.text().len(); - Some(label) + label } fn completion_for_thread( @@ -274,32 +242,36 @@ impl ContextPickerCompletionProvider { fn completion_for_path( project_path: ProjectPath, + path_prefix: &str, is_recent: bool, is_directory: bool, excerpt_id: ExcerptId, source_range: Range, editor: Entity, context_store: Entity, - workspace: Entity, cx: &App, - ) -> Option { - let label = Self::build_code_label_for_full_path( - project_path.worktree_id, - &project_path.path, - workspace.clone(), - cx, - )?; - let full_path = Self::full_path_for_entry( - project_path.worktree_id, + ) -> Completion { + let (file_name, directory) = super::file_context_picker::extract_file_name_and_directory( &project_path.path, - workspace.clone(), + path_prefix, + ); + + let label = Self::build_code_label_for_full_path( + &file_name, + directory.as_ref().map(|s| s.as_ref()), cx, - )?; + ); + let full_path = if let Some(directory) = directory { + format!("{}{}", directory, file_name) + } else { + file_name.to_string() + }; let crease_icon_path = if is_directory { FileIcons::get_folder_icon(false, cx).unwrap_or_else(|| IconName::Folder.path().into()) } else { - FileIcons::get_icon(&full_path, cx).unwrap_or_else(|| IconName::File.path().into()) + FileIcons::get_icon(Path::new(&full_path), cx) + .unwrap_or_else(|| IconName::File.path().into()) }; let completion_icon_path = if is_recent { IconName::HistoryRerun.path().into() @@ -307,15 +279,9 @@ impl ContextPickerCompletionProvider { crease_icon_path.clone() }; - let crease_name = project_path - .path - .file_name() - .map(|file_name| file_name.to_string_lossy().to_string()) - .unwrap_or_else(|| "untitled".to_string()); - - let new_text = format!("@file {}", full_path.to_string_lossy()); + let new_text = format!("@file {}", full_path); let new_text_len = new_text.len(); - Some(Completion { + Completion { old_range: source_range.clone(), new_text, label, @@ -324,7 +290,7 @@ impl ContextPickerCompletionProvider { icon_path: Some(completion_icon_path), confirm: Some(confirm_completion_callback( crease_icon_path, - crease_name.into(), + file_name, excerpt_id, source_range.start, new_text_len, @@ -340,7 +306,7 @@ impl ContextPickerCompletionProvider { }) }, )), - }) + } } } @@ -397,33 +363,34 @@ impl CompletionProvider for ContextPickerCompletionProvider { .update(|cx| { super::file_context_picker::search_paths( query, - Arc::new(AtomicBool::default()), + Arc::::default(), &workspace, cx, ) })? .await; - completions.reserve(path_matches.len()); - cx.update(|cx| { - completions.extend(path_matches.iter().filter_map(|mat| { - let editor = editor.upgrade()?; - Self::completion_for_path( - ProjectPath { - worktree_id: WorktreeId::from_usize(mat.worktree_id), - path: mat.path.clone(), - }, - false, - mat.is_dir, - excerpt_id, - source_range.clone(), - editor.clone(), - context_store.clone(), - workspace.clone(), - cx, - ) - })); - })?; + if let Some(editor) = editor.upgrade() { + completions.reserve(path_matches.len()); + cx.update(|cx| { + completions.extend(path_matches.iter().map(|mat| { + Self::completion_for_path( + ProjectPath { + worktree_id: WorktreeId::from_usize(mat.worktree_id), + path: mat.path.clone(), + }, + &mat.path_prefix, + false, + mat.is_dir, + excerpt_id, + source_range.clone(), + editor.clone(), + context_store.clone(), + cx, + ) + })); + })?; + } } Some(ContextPickerMode::Fetch) => { if let Some(editor) = editor.upgrade() { @@ -771,7 +738,6 @@ mod tests { .unwrap(); } - //TODO: Construct the editor without an actual buffer that points to a file let item = workspace .update_in(&mut cx, |workspace, window, cx| { workspace.open_path( @@ -821,10 +787,10 @@ mod tests { assert_eq!( current_completion_labels(editor), &[ - format!("seven.txt {}", separator!("dir/b")).as_str(), - format!("six.txt {}", separator!("dir/b")).as_str(), - format!("five.txt {}", separator!("dir/b")).as_str(), - format!("four.txt {}", separator!("dir/a")).as_str(), + "seven.txt dir/b/", + "six.txt dir/b/", + "five.txt dir/b/", + "four.txt dir/a/", "Files & Directories", "Fetch" ] @@ -853,10 +819,7 @@ mod tests { editor.update(&mut cx, |editor, cx| { assert_eq!(editor.text(cx), "Lorem @file one"); assert!(editor.has_visible_completions_menu()); - assert_eq!( - current_completion_labels(editor), - vec![format!("one.txt {}", separator!("dir/a")).as_str(),] - ); + assert_eq!(current_completion_labels(editor), vec!["one.txt dir/a/"]); }); editor.update_in(&mut cx, |editor, window, cx| { @@ -865,10 +828,7 @@ mod tests { }); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - format!("Lorem @file {}", separator!("dir/a/one.txt")) - ); + assert_eq!(editor.text(cx), "Lorem @file dir/a/one.txt",); assert!(!editor.has_visible_completions_menu()); assert_eq!( crease_ranges(editor, cx), @@ -879,10 +839,7 @@ mod tests { cx.simulate_input(" "); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - format!("Lorem @file {} ", separator!("dir/a/one.txt")) - ); + assert_eq!(editor.text(cx), "Lorem @file dir/a/one.txt ",); assert!(!editor.has_visible_completions_menu()); assert_eq!( crease_ranges(editor, cx), @@ -893,10 +850,7 @@ mod tests { cx.simulate_input("Ipsum "); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - format!("Lorem @file {} Ipsum ", separator!("dir/a/one.txt")) - ); + assert_eq!(editor.text(cx), "Lorem @file dir/a/one.txt Ipsum ",); assert!(!editor.has_visible_completions_menu()); assert_eq!( crease_ranges(editor, cx), @@ -907,10 +861,7 @@ mod tests { cx.simulate_input("@file "); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - format!("Lorem @file {} Ipsum @file ", separator!("dir/a/one.txt")) - ); + assert_eq!(editor.text(cx), "Lorem @file dir/a/one.txt Ipsum @file ",); assert!(editor.has_visible_completions_menu()); assert_eq!( crease_ranges(editor, cx), @@ -927,11 +878,7 @@ mod tests { editor.update(&mut cx, |editor, cx| { assert_eq!( editor.text(cx), - format!( - "Lorem @file {} Ipsum @file {}", - separator!("dir/a/one.txt"), - separator!("dir/b/seven.txt") - ) + "Lorem @file dir/a/one.txt Ipsum @file dir/b/seven.txt" ); assert!(!editor.has_visible_completions_menu()); assert_eq!( @@ -948,11 +895,7 @@ mod tests { editor.update(&mut cx, |editor, cx| { assert_eq!( editor.text(cx), - format!( - "Lorem @file {} Ipsum @file {}\n@", - separator!("dir/a/one.txt"), - separator!("dir/b/seven.txt") - ) + "Lorem @file dir/a/one.txt Ipsum @file dir/b/seven.txt\n@" ); assert!(editor.has_visible_completions_menu()); assert_eq!( @@ -973,12 +916,7 @@ mod tests { editor.update(&mut cx, |editor, cx| { assert_eq!( editor.text(cx), - format!( - "Lorem @file {} Ipsum @file {}\n@file {}", - separator!("dir/a/one.txt"), - separator!("dir/b/seven.txt"), - separator!("dir/b/six.txt"), - ) + "Lorem @file dir/a/one.txt Ipsum @file dir/b/seven.txt\n@file dir/b/six.txt" ); assert!(!editor.has_visible_completions_menu()); assert_eq!( diff --git a/crates/assistant2/src/context_picker/file_context_picker.rs b/crates/assistant2/src/context_picker/file_context_picker.rs index 65bbb4e302c15cb62d35e2c3fbdeadd2c9473637..388fdcbc9f38c0f12a2f24919ef0344cbf9055c0 100644 --- a/crates/assistant2/src/context_picker/file_context_picker.rs +++ b/crates/assistant2/src/context_picker/file_context_picker.rs @@ -273,17 +273,17 @@ pub(crate) fn search_paths( } } -pub fn render_file_context_entry( - id: ElementId, +pub fn extract_file_name_and_directory( path: &Path, - path_prefix: &Arc, - is_directory: bool, - context_store: WeakEntity, - cx: &App, -) -> Stateful
{ - let (file_name, directory) = if path == Path::new("") { + path_prefix: &str, +) -> (SharedString, Option) { + if path == Path::new("") { ( - SharedString::from(path_prefix.trim_end_matches('/').to_string()), + SharedString::from( + path_prefix + .trim_end_matches(std::path::MAIN_SEPARATOR) + .to_string(), + ), None, ) } else { @@ -294,7 +294,9 @@ pub fn render_file_context_entry( .to_string() .into(); - let mut directory = path_prefix.to_string(); + let mut directory = path_prefix + .trim_end_matches(std::path::MAIN_SEPARATOR) + .to_string(); if !directory.ends_with('/') { directory.push('/'); } @@ -303,8 +305,19 @@ pub fn render_file_context_entry( directory.push('/'); } - (file_name, Some(directory)) - }; + (file_name, Some(directory.into())) + } +} + +pub fn render_file_context_entry( + id: ElementId, + path: &Path, + path_prefix: &Arc, + is_directory: bool, + context_store: WeakEntity, + cx: &App, +) -> Stateful
{ + let (file_name, directory) = extract_file_name_and_directory(path, path_prefix); let added = context_store.upgrade().and_then(|context_store| { if is_directory {