From bcc6d9552990536b9500ccd1ebaf8bbd1c31d9d6 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Tue, 7 Jan 2025 09:18:46 -0300 Subject: [PATCH] assistant2: Make context pill names work like editor tabs (#22741) Context pills for files will now only display the basename of the file. If two files have the same base name, we will also show their parent directories, mimicking the behavior of editor tabs. https://github.com/user-attachments/assets/ee88ee3b-80ff-4115-9ff9-8fe4845a67d8 Note: The double `/` in the file picker is a known separate issue. Release Notes: - N/A --------- Co-authored-by: Danilo Co-authored-by: Danilo Leal --- crates/assistant2/src/active_thread.rs | 12 +- crates/assistant2/src/context.rs | 4 +- crates/assistant2/src/context_store.rs | 47 ++++++- crates/assistant2/src/context_strip.rs | 91 +++++++------- crates/assistant2/src/ui/context_pill.rs | 149 ++++++++++++++++++----- 5 files changed, 218 insertions(+), 85 deletions(-) diff --git a/crates/assistant2/src/active_thread.rs b/crates/assistant2/src/active_thread.rs index c6d432a0a5794245577e388e5635eb164705ff16..434ddd737cb57bf51ad877ac710d6d4c87d5e558 100644 --- a/crates/assistant2/src/active_thread.rs +++ b/crates/assistant2/src/active_thread.rs @@ -282,13 +282,11 @@ impl ActiveThread { .child(div().p_2p5().text_ui(cx).child(markdown.clone())) .when_some(context, |parent, context| { if !context.is_empty() { - parent.child( - h_flex().flex_wrap().gap_1().px_1p5().pb_1p5().children( - context - .iter() - .map(|context| ContextPill::new(context.clone())), - ), - ) + parent.child(h_flex().flex_wrap().gap_1().px_1p5().pb_1p5().children( + context.iter().map(|context| { + ContextPill::new_added(context.clone(), false, None) + }), + )) } else { parent } diff --git a/crates/assistant2/src/context.rs b/crates/assistant2/src/context.rs index 1aff6c982da1301941ba079888560a040998ab0a..0620707e47dd05d88da56cd544117897f06f336d 100644 --- a/crates/assistant2/src/context.rs +++ b/crates/assistant2/src/context.rs @@ -17,6 +17,8 @@ impl ContextId { pub struct Context { pub id: ContextId, pub name: SharedString, + pub parent: Option, + pub tooltip: Option, pub kind: ContextKind, pub text: SharedString, } @@ -40,7 +42,7 @@ pub fn attach_context_to_message( for context in context.into_iter() { match context.kind { - ContextKind::File => { + ContextKind::File { .. } => { file_context.push_str(&context.text); file_context.push('\n'); } diff --git a/crates/assistant2/src/context_store.rs b/crates/assistant2/src/context_store.rs index 9b4753544197b76e84797b76b7ea7186adf0a131..6ce35f8971254db3f40f85e0adcd03c3fda104b3 100644 --- a/crates/assistant2/src/context_store.rs +++ b/crates/assistant2/src/context_store.rs @@ -1,7 +1,7 @@ use std::fmt::Write as _; use std::path::{Path, PathBuf}; -use collections::HashMap; +use collections::{HashMap, HashSet}; use gpui::SharedString; use language::Buffer; @@ -60,7 +60,17 @@ impl ContextStore { let id = self.next_context_id.post_inc(); self.files.insert(path.to_path_buf(), id); - let name = path.to_string_lossy().into_owned().into(); + let full_path: SharedString = path.to_string_lossy().into_owned().into(); + + let name = match path.file_name() { + Some(name) => name.to_string_lossy().into_owned().into(), + None => full_path.clone(), + }; + + let parent = path + .parent() + .and_then(|p| p.file_name()) + .map(|p| p.to_string_lossy().into_owned().into()); let mut text = String::new(); push_fenced_codeblock(path, buffer.text(), &mut text); @@ -68,6 +78,8 @@ impl ContextStore { self.context.push(Context { id, name, + parent, + tooltip: Some(full_path), kind: ContextKind::File, text: text.into(), }); @@ -77,11 +89,23 @@ impl ContextStore { let id = self.next_context_id.post_inc(); self.directories.insert(path.to_path_buf(), id); - let name = path.to_string_lossy().into_owned().into(); + let full_path: SharedString = path.to_string_lossy().into_owned().into(); + + let name = match path.file_name() { + Some(name) => name.to_string_lossy().into_owned().into(), + None => full_path.clone(), + }; + + let parent = path + .parent() + .and_then(|p| p.file_name()) + .map(|p| p.to_string_lossy().into_owned().into()); self.context.push(Context { id, name, + parent, + tooltip: Some(full_path), kind: ContextKind::Directory, text: text.into(), }); @@ -94,6 +118,8 @@ impl ContextStore { self.context.push(Context { id: context_id, name: thread.summary().unwrap_or("New thread".into()), + parent: None, + tooltip: None, kind: ContextKind::Thread, text: thread.text().into(), }); @@ -106,6 +132,8 @@ impl ContextStore { self.context.push(Context { id: context_id, name: url.into(), + parent: None, + tooltip: None, kind: ContextKind::FetchedUrl, text: text.into(), }); @@ -163,6 +191,19 @@ impl ContextStore { pub fn included_url(&self, url: &str) -> Option { self.fetched_urls.get(url).copied() } + + pub fn duplicated_names(&self) -> HashSet { + let mut seen = HashSet::default(); + let mut dupes = HashSet::default(); + + for context in self.context().iter() { + if !seen.insert(&context.name) { + dupes.insert(context.name.clone()); + } + } + + dupes + } } pub enum IncludedFile { diff --git a/crates/assistant2/src/context_strip.rs b/crates/assistant2/src/context_strip.rs index 66aee3463cd261dfb2efb8a9dd43049b685d74ef..ef76d8d70886d7966705ce2f7d74c4d86c634091 100644 --- a/crates/assistant2/src/context_strip.rs +++ b/crates/assistant2/src/context_strip.rs @@ -6,6 +6,7 @@ use language::Buffer; use ui::{prelude::*, KeyBinding, PopoverMenu, PopoverMenuHandle, Tooltip}; use workspace::Workspace; +use crate::context::ContextKind; use crate::context_picker::{ConfirmBehavior, ContextPicker}; use crate::context_store::ContextStore; use crate::thread::Thread; @@ -70,10 +71,13 @@ impl ContextStrip { return None; } - let title = path.to_string_lossy().into_owned().into(); + let name = match path.file_name() { + Some(name) => name.to_string_lossy().into_owned().into(), + None => path.to_string_lossy().into_owned().into(), + }; Some(SuggestedContext::File { - title, + name, buffer: active_buffer.downgrade(), }) } @@ -99,7 +103,7 @@ impl ContextStrip { } Some(SuggestedContext::Thread { - title: active_thread.summary().unwrap_or("Active Thread".into()), + name: active_thread.summary().unwrap_or("New Thread".into()), thread: weak_active_thread, }) } @@ -114,6 +118,8 @@ impl Render for ContextStrip { let suggested_context = self.suggested_context(cx); + let dupe_names = context_store.duplicated_names(); + h_flex() .flex_wrap() .gap_1() @@ -165,40 +171,36 @@ impl Render for ContextStrip { } }) .children(context.iter().map(|context| { - ContextPill::new(context.clone()).on_remove({ - let context = context.clone(); - let context_store = self.context_store.clone(); - Rc::new(cx.listener(move |_this, _event, cx| { - context_store.update(cx, |this, _cx| { - this.remove_context(&context.id); - }); - cx.notify(); - })) - }) + ContextPill::new_added( + context.clone(), + dupe_names.contains(&context.name), + Some({ + let context = context.clone(); + let context_store = self.context_store.clone(); + Rc::new(cx.listener(move |_this, _event, cx| { + context_store.update(cx, |this, _cx| { + this.remove_context(&context.id); + }); + cx.notify(); + })) + }), + ) })) .when_some(suggested_context, |el, suggested| { - el.child( - Button::new("add-suggested-context", suggested.title().clone()) - .on_click({ - let context_store = self.context_store.clone(); + el.child(ContextPill::new_suggested( + suggested.name().clone(), + suggested.kind(), + { + let context_store = self.context_store.clone(); + Rc::new(cx.listener(move |_this, _event, cx| { + context_store.update(cx, |context_store, cx| { + suggested.accept(context_store, cx); + }); - cx.listener(move |_this, _event, cx| { - context_store.update(cx, |context_store, cx| { - suggested.accept(context_store, cx); - }); - cx.notify(); - }) - }) - .icon(IconName::Plus) - .icon_position(IconPosition::Start) - .icon_size(IconSize::XSmall) - .icon_color(Color::Muted) - .label_size(LabelSize::Small) - .style(ButtonStyle::Filled) - .tooltip(|cx| { - Tooltip::with_meta("Suggested Context", None, "Click to add it", cx) - }), - ) + cx.notify(); + })) + }, + )) }) .when(!context.is_empty(), { move |parent| { @@ -227,35 +229,42 @@ pub enum SuggestContextKind { #[derive(Clone)] pub enum SuggestedContext { File { - title: SharedString, + name: SharedString, buffer: WeakModel, }, Thread { - title: SharedString, + name: SharedString, thread: WeakModel, }, } impl SuggestedContext { - pub fn title(&self) -> &SharedString { + pub fn name(&self) -> &SharedString { match self { - Self::File { title, .. } => title, - Self::Thread { title, .. } => title, + Self::File { name, .. } => name, + Self::Thread { name, .. } => name, } } pub fn accept(&self, context_store: &mut ContextStore, cx: &mut AppContext) { match self { - Self::File { buffer, title: _ } => { + Self::File { buffer, name: _ } => { if let Some(buffer) = buffer.upgrade() { context_store.insert_file(buffer.read(cx)); }; } - Self::Thread { thread, title: _ } => { + Self::Thread { thread, name: _ } => { if let Some(thread) = thread.upgrade() { context_store.insert_thread(thread.read(cx)); }; } } } + + pub fn kind(&self) -> ContextKind { + match self { + Self::File { .. } => ContextKind::File, + Self::Thread { .. } => ContextKind::Thread, + } + } } diff --git a/crates/assistant2/src/ui/context_pill.rs b/crates/assistant2/src/ui/context_pill.rs index fb926386e2ac899a4cf9fa6d3a68269107b7d623..093f68dcd22904d1f664396f53f63df7c260a02e 100644 --- a/crates/assistant2/src/ui/context_pill.rs +++ b/crates/assistant2/src/ui/context_pill.rs @@ -1,65 +1,148 @@ use std::rc::Rc; use gpui::ClickEvent; -use ui::{prelude::*, IconButtonShape}; +use ui::{prelude::*, IconButtonShape, Tooltip}; use crate::context::{Context, ContextKind}; #[derive(IntoElement)] -pub struct ContextPill { - context: Context, - on_remove: Option>, +pub enum ContextPill { + Added { + context: Context, + dupe_name: bool, + on_remove: Option>, + }, + Suggested { + name: SharedString, + kind: ContextKind, + on_add: Rc, + }, } impl ContextPill { - pub fn new(context: Context) -> Self { - Self { + pub fn new_added( + context: Context, + dupe_name: bool, + on_remove: Option>, + ) -> Self { + Self::Added { context, - on_remove: None, + dupe_name, + on_remove, } } - pub fn on_remove(mut self, on_remove: Rc) -> Self { - self.on_remove = Some(on_remove); - self + pub fn new_suggested( + name: SharedString, + kind: ContextKind, + on_add: Rc, + ) -> Self { + Self::Suggested { name, kind, on_add } + } + + pub fn id(&self) -> ElementId { + match self { + Self::Added { context, .. } => { + ElementId::NamedInteger("context-pill".into(), context.id.0) + } + Self::Suggested { .. } => "suggested-context-pill".into(), + } + } + + pub fn kind(&self) -> &ContextKind { + match self { + Self::Added { context, .. } => &context.kind, + Self::Suggested { kind, .. } => kind, + } } } impl RenderOnce for ContextPill { fn render(self, cx: &mut WindowContext) -> impl IntoElement { - let padding_right = if self.on_remove.is_some() { - px(2.) - } else { - px(4.) - }; - let icon = match self.context.kind { + let icon = match &self.kind() { ContextKind::File => IconName::File, ContextKind::Directory => IconName::Folder, ContextKind::FetchedUrl => IconName::Globe, ContextKind::Thread => IconName::MessageCircle, }; - h_flex() - .gap_1() + let color = cx.theme().colors(); + + let base_pill = h_flex() + .id(self.id()) .pl_1() - .pr(padding_right) .pb(px(1.)) .border_1() - .border_color(cx.theme().colors().border.opacity(0.5)) - .bg(cx.theme().colors().element_background) .rounded_md() - .child(Icon::new(icon).size(IconSize::XSmall).color(Color::Muted)) - .child(Label::new(self.context.name.clone()).size(LabelSize::Small)) - .when_some(self.on_remove, |parent, on_remove| { - parent.child( - IconButton::new(("remove", self.context.id.0), IconName::Close) - .shape(IconButtonShape::Square) - .icon_size(IconSize::XSmall) - .on_click({ - let on_remove = on_remove.clone(); - move |event, cx| on_remove(event, cx) - }), + .gap_1() + .child(Icon::new(icon).size(IconSize::XSmall).color(Color::Muted)); + + match &self { + ContextPill::Added { + context, + dupe_name, + on_remove, + } => base_pill + .bg(color.element_background) + .border_color(color.border.opacity(0.5)) + .pr(if on_remove.is_some() { px(2.) } else { px(4.) }) + .child(Label::new(context.name.clone()).size(LabelSize::Small)) + .when_some(context.parent.as_ref(), |element, parent_name| { + if *dupe_name { + element.child( + Label::new(parent_name.clone()) + .size(LabelSize::XSmall) + .color(Color::Muted), + ) + } else { + element + } + }) + .when_some(context.tooltip.clone(), |element, tooltip| { + element.tooltip(move |cx| Tooltip::text(tooltip.clone(), cx)) + }) + .when_some(on_remove.as_ref(), |element, on_remove| { + element.child( + IconButton::new(("remove", context.id.0), IconName::Close) + .shape(IconButtonShape::Square) + .icon_size(IconSize::XSmall) + .tooltip(|cx| Tooltip::text("Remove Context", cx)) + .on_click({ + let on_remove = on_remove.clone(); + move |event, cx| on_remove(event, cx) + }), + ) + }), + ContextPill::Suggested { name, kind, on_add } => base_pill + .cursor_pointer() + .pr_1() + .border_color(color.border_variant.opacity(0.5)) + .hover(|style| style.bg(color.element_hover.opacity(0.5))) + .child( + Label::new(name.clone()) + .size(LabelSize::Small) + .color(Color::Muted), ) - }) + .child( + Label::new(match kind { + ContextKind::File => "Open File", + ContextKind::Thread | ContextKind::Directory | ContextKind::FetchedUrl => { + "Active" + } + }) + .size(LabelSize::XSmall) + .color(Color::Muted), + ) + .child( + Icon::new(IconName::Plus) + .size(IconSize::XSmall) + .into_any_element(), + ) + .tooltip(|cx| Tooltip::with_meta("Suggested Context", None, "Click to add it", cx)) + .on_click({ + let on_add = on_add.clone(); + move |event, cx| on_add(event, cx) + }), + } } }