From 013a64679947fca3c940d319841069000d5ddf2b Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Mon, 10 Mar 2025 11:39:01 -0400 Subject: [PATCH] git_ui: Branch picker improvements (#26287) - Truncate branch names based on the width of the picker - Use a footer for "Create branch" instead of a picker entry Still to do: - [x] Select the footer button when no matches and run the create logic on `enter` - [x] Make it possible to quickly select the footer button from the keyboard when there are matches Release Notes: - Git Beta: Removed limitation that made it impossible to create a branch from the branch picker when it too closely resembled an existing branch name --- .../context_picker/fetch_context_picker.rs | 4 +- crates/collab/src/tests/integration_tests.rs | 26 ++- .../remote_editing_collaboration_tests.rs | 26 ++- crates/file_finder/src/new_path_prompt.rs | 4 +- crates/file_finder/src/open_path_prompt.rs | 8 +- crates/git_ui/src/branch_picker.rs | 151 ++++++++++++++---- crates/gpui/src/color.rs | 4 +- crates/picker/src/picker.rs | 27 ++-- crates/project/src/git.rs | 10 +- crates/prompt_library/src/prompt_library.rs | 7 +- crates/recent_projects/src/recent_projects.rs | 7 +- .../remote_server/src/remote_editing_tests.rs | 26 ++- crates/tab_switcher/src/tab_switcher.rs | 4 +- 13 files changed, 184 insertions(+), 120 deletions(-) diff --git a/crates/assistant2/src/context_picker/fetch_context_picker.rs b/crates/assistant2/src/context_picker/fetch_context_picker.rs index d8acfe0918..52c682b647 100644 --- a/crates/assistant2/src/context_picker/fetch_context_picker.rs +++ b/crates/assistant2/src/context_picker/fetch_context_picker.rs @@ -167,8 +167,8 @@ impl PickerDelegate for FetchContextPickerDelegate { } } - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - "Enter the URL that you would like to fetch".into() + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + Some("Enter the URL that you would like to fetch".into()) } fn selected_index(&self) -> usize { diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 4e99b67dba..d3cef14b0e 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -6770,7 +6770,7 @@ async fn test_remote_git_branches( assert_eq!(branches_b, branches_set); - cx_b.update(|cx| repo_b.read(cx).change_branch(new_branch.to_string())) + cx_b.update(|cx| repo_b.read(cx).change_branch(new_branch)) .await .unwrap() .unwrap(); @@ -6790,23 +6790,15 @@ async fn test_remote_git_branches( assert_eq!(host_branch.name, branches[2]); // Also try creating a new branch - cx_b.update(|cx| { - repo_b - .read(cx) - .create_branch("totally-new-branch".to_string()) - }) - .await - .unwrap() - .unwrap(); + cx_b.update(|cx| repo_b.read(cx).create_branch("totally-new-branch")) + .await + .unwrap() + .unwrap(); - cx_b.update(|cx| { - repo_b - .read(cx) - .change_branch("totally-new-branch".to_string()) - }) - .await - .unwrap() - .unwrap(); + cx_b.update(|cx| repo_b.read(cx).change_branch("totally-new-branch")) + .await + .unwrap() + .unwrap(); executor.run_until_parked(); diff --git a/crates/collab/src/tests/remote_editing_collaboration_tests.rs b/crates/collab/src/tests/remote_editing_collaboration_tests.rs index fbc25cbf03..bb889f849c 100644 --- a/crates/collab/src/tests/remote_editing_collaboration_tests.rs +++ b/crates/collab/src/tests/remote_editing_collaboration_tests.rs @@ -294,7 +294,7 @@ async fn test_ssh_collaboration_git_branches( assert_eq!(&branches_b, &branches_set); - cx_b.update(|cx| repo_b.read(cx).change_branch(new_branch.to_string())) + cx_b.update(|cx| repo_b.read(cx).change_branch(new_branch)) .await .unwrap() .unwrap(); @@ -316,23 +316,15 @@ async fn test_ssh_collaboration_git_branches( assert_eq!(server_branch.name, branches[2]); // Also try creating a new branch - cx_b.update(|cx| { - repo_b - .read(cx) - .create_branch("totally-new-branch".to_string()) - }) - .await - .unwrap() - .unwrap(); + cx_b.update(|cx| repo_b.read(cx).create_branch("totally-new-branch")) + .await + .unwrap() + .unwrap(); - cx_b.update(|cx| { - repo_b - .read(cx) - .change_branch("totally-new-branch".to_string()) - }) - .await - .unwrap() - .unwrap(); + cx_b.update(|cx| repo_b.read(cx).change_branch("totally-new-branch")) + .await + .unwrap() + .unwrap(); executor.run_until_parked(); diff --git a/crates/file_finder/src/new_path_prompt.rs b/crates/file_finder/src/new_path_prompt.rs index 05b336fc45..5dc4f2c7e5 100644 --- a/crates/file_finder/src/new_path_prompt.rs +++ b/crates/file_finder/src/new_path_prompt.rs @@ -436,8 +436,8 @@ impl PickerDelegate for NewPathDelegate { ) } - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - "Type a path...".into() + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + Some("Type a path...".into()) } fn placeholder_text(&self, _window: &mut Window, _cx: &mut App) -> Arc { diff --git a/crates/file_finder/src/open_path_prompt.rs b/crates/file_finder/src/open_path_prompt.rs index 254e64d83a..73aa55036f 100644 --- a/crates/file_finder/src/open_path_prompt.rs +++ b/crates/file_finder/src/open_path_prompt.rs @@ -347,12 +347,14 @@ impl PickerDelegate for OpenPathDelegate { ) } - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - if let Some(error) = self.directory_state.as_ref().and_then(|s| s.error.clone()) { + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + let text = if let Some(error) = self.directory_state.as_ref().and_then(|s| s.error.clone()) + { error } else { "No such file or directory".into() - } + }; + Some(text) } fn placeholder_text(&self, _window: &mut Window, _cx: &mut App) -> Arc { diff --git a/crates/git_ui/src/branch_picker.rs b/crates/git_ui/src/branch_picker.rs index 9c81b62ff1..3b6ec702b2 100644 --- a/crates/git_ui/src/branch_picker.rs +++ b/crates/git_ui/src/branch_picker.rs @@ -4,13 +4,15 @@ use fuzzy::{StringMatch, StringMatchCandidate}; use git::repository::Branch; use gpui::{ rems, App, Context, DismissEvent, Entity, EventEmitter, FocusHandle, Focusable, - InteractiveElement, IntoElement, ParentElement, Render, SharedString, Styled, Subscription, - Task, Window, + InteractiveElement, IntoElement, Modifiers, ModifiersChangedEvent, ParentElement, Render, + SharedString, Styled, Subscription, Task, Window, }; use picker::{Picker, PickerDelegate}; use project::git::Repository; use std::sync::Arc; -use ui::{prelude::*, HighlightedLabel, ListItem, ListItemSpacing, PopoverMenuHandle}; +use ui::{ + prelude::*, HighlightedLabel, KeyBinding, ListItem, ListItemSpacing, PopoverMenuHandle, Tooltip, +}; use util::ResultExt; use workspace::notifications::DetachAndPromptErr; use workspace::{ModalView, Workspace}; @@ -51,7 +53,7 @@ pub fn open( let repository = workspace.project().read(cx).active_repository(cx).clone(); let style = BranchListStyle::Modal; workspace.toggle_modal(window, cx, |window, cx| { - BranchList::new(repository, style, 34., window, cx) + BranchList::new(repository, style, rems(34.), window, cx) }) } @@ -61,7 +63,7 @@ pub fn popover( cx: &mut App, ) -> Entity { cx.new(|cx| { - let list = BranchList::new(repository, BranchListStyle::Popover, 15., window, cx); + let list = BranchList::new(repository, BranchListStyle::Popover, rems(15.), window, cx); list.focus_handle(cx).focus(window); list }) @@ -74,7 +76,7 @@ enum BranchListStyle { } pub struct BranchList { - rem_width: f32, + width: Rems, pub popover_handle: PopoverMenuHandle, pub picker: Entity>, _subscription: Subscription, @@ -84,7 +86,7 @@ impl BranchList { fn new( repository: Option>, style: BranchListStyle, - rem_width: f32, + width: Rems, window: &mut Window, cx: &mut Context, ) -> Self { @@ -109,7 +111,7 @@ impl BranchList { }) .detach_and_log_err(cx); - let delegate = BranchListDelegate::new(repository.clone(), style, 20); + let delegate = BranchListDelegate::new(repository.clone(), style, (1.6 * width.0) as usize); let picker = cx.new(|cx| Picker::uniform_list(delegate, window, cx)); let _subscription = cx.subscribe(&picker, |_, _, _, cx| { @@ -118,11 +120,21 @@ impl BranchList { Self { picker, - rem_width, + width, popover_handle, _subscription, } } + + fn handle_modifiers_changed( + &mut self, + ev: &ModifiersChangedEvent, + _: &mut Window, + cx: &mut Context, + ) { + self.picker + .update(cx, |picker, _| picker.delegate.modifiers = ev.modifiers) + } } impl ModalView for BranchList {} impl EventEmitter for BranchList {} @@ -136,7 +148,8 @@ impl Focusable for BranchList { impl Render for BranchList { fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { v_flex() - .w(rems(self.rem_width)) + .w(self.width) + .on_modifiers_changed(cx.listener(Self::handle_modifiers_changed)) .child(self.picker.clone()) .on_mouse_down_out({ cx.listener(move |this, _, window, cx| { @@ -152,7 +165,6 @@ impl Render for BranchList { enum BranchEntry { Branch(StringMatch), History(String), - NewBranch { name: String }, } impl BranchEntry { @@ -160,7 +172,6 @@ impl BranchEntry { match self { Self::Branch(branch) => &branch.string, Self::History(branch) => &branch, - Self::NewBranch { name } => &name, } } } @@ -174,6 +185,7 @@ pub struct BranchListDelegate { last_query: String, /// Max length of branch name before we truncate it and add a trailing `...`. branch_name_trailoff_after: usize, + modifiers: Modifiers, } impl BranchListDelegate { @@ -190,14 +202,37 @@ impl BranchListDelegate { selected_index: 0, last_query: Default::default(), branch_name_trailoff_after, + modifiers: Default::default(), } } - pub fn branch_count(&self) -> usize { - self.matches - .iter() - .filter(|item| matches!(item, BranchEntry::Branch(_))) - .count() + fn has_exact_match(&self, target: &str) -> bool { + self.matches.iter().any(|mat| match mat { + BranchEntry::Branch(branch) => branch.string == target, + _ => false, + }) + } + + fn create_branch( + &self, + new_branch_name: SharedString, + window: &mut Window, + cx: &mut Context>, + ) { + let Some(repo) = self.repo.clone() else { + return; + }; + cx.spawn(|_, cx| async move { + cx.update(|cx| repo.read(cx).create_branch(&new_branch_name))? + .await??; + cx.update(|cx| repo.read(cx).change_branch(&new_branch_name))? + .await??; + Ok(()) + }) + .detach_and_prompt_err("Failed to create branch", window, cx, |e, _, _| { + Some(e.to_string()) + }); + cx.emit(DismissEvent); } } @@ -281,12 +316,6 @@ impl PickerDelegate for BranchListDelegate { let delegate = &mut picker.delegate; delegate.matches = matches; if delegate.matches.is_empty() { - if !query.is_empty() { - delegate.matches.push(BranchEntry::NewBranch { - name: query.trim().replace(' ', "-"), - }); - } - delegate.selected_index = 0; } else { delegate.selected_index = @@ -298,7 +327,16 @@ impl PickerDelegate for BranchListDelegate { }) } - fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + fn confirm(&mut self, secondary: bool, window: &mut Window, cx: &mut Context>) { + let new_branch_name = SharedString::from(self.last_query.trim().replace(" ", "-")); + if !new_branch_name.is_empty() + && !self.has_exact_match(&new_branch_name) + && ((self.selected_index == 0 && self.matches.len() == 0) || secondary) + { + self.create_branch(new_branch_name, window, cx); + return; + } + let Some(branch) = self.matches.get(self.selected_index()) else { return; }; @@ -337,13 +375,7 @@ impl PickerDelegate for BranchListDelegate { .. }) | BranchEntry::History(branch_name) => { - cx.update(|cx| repo.read(cx).change_branch(branch_name))? - .await? - } - BranchEntry::NewBranch { name: branch_name } => { - cx.update(|cx| repo.read(cx).create_branch(branch_name.clone()))? - .await??; - cx.update(|cx| repo.read(cx).change_branch(branch_name))? + cx.update(|cx| repo.read(cx).change_branch(&branch_name))? .await? } } @@ -405,10 +437,61 @@ impl PickerDelegate for BranchListDelegate { el.child(HighlightedLabel::new(shortened_branch_name, highlights)) } BranchEntry::History(_) => el.child(Label::new(shortened_branch_name)), - BranchEntry::NewBranch { name } => { - el.child(Label::new(format!("Create branch '{name}'"))) - } }), ) } + + fn render_footer( + &self, + window: &mut Window, + cx: &mut Context>, + ) -> Option { + let new_branch_name = SharedString::from(self.last_query.trim().replace(" ", "-")); + let handle = cx.weak_entity(); + Some( + h_flex() + .w_full() + .p_2() + .gap_2() + .border_t_1() + .border_color(cx.theme().colors().border_variant) + .when( + !new_branch_name.is_empty() && !self.has_exact_match(&new_branch_name), + |el| { + el.child( + Button::new( + "create-branch", + format!("Create branch '{new_branch_name}'",), + ) + .key_binding(KeyBinding::for_action( + &menu::SecondaryConfirm, + window, + cx, + )) + .toggle_state( + self.modifiers.secondary() + || (self.selected_index == 0 && self.matches.len() == 0), + ) + .tooltip(Tooltip::for_action_title( + "Create branch", + &menu::SecondaryConfirm, + )) + .on_click(move |_, window, cx| { + let new_branch_name = new_branch_name.clone(); + if let Some(picker) = handle.upgrade() { + picker.update(cx, |picker, cx| { + picker.delegate.create_branch(new_branch_name, window, cx) + }); + } + }), + ) + }, + ) + .into_any(), + ) + } + + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + None + } } diff --git a/crates/gpui/src/color.rs b/crates/gpui/src/color.rs index ffc729a896..d0d9d36617 100644 --- a/crates/gpui/src/color.rs +++ b/crates/gpui/src/color.rs @@ -683,11 +683,11 @@ impl Default for Background { } /// Creates a hash pattern background -pub fn pattern_slash(color: Hsla, thickness: f32) -> Background { +pub fn pattern_slash(color: Hsla, height: f32) -> Background { Background { tag: BackgroundTag::PatternSlash, solid: color, - gradient_angle_or_pattern_height: thickness, + gradient_angle_or_pattern_height: height, ..Default::default() } } diff --git a/crates/picker/src/picker.rs b/crates/picker/src/picker.rs index fd0d9e6295..8e2e258356 100644 --- a/crates/picker/src/picker.rs +++ b/crates/picker/src/picker.rs @@ -96,8 +96,8 @@ pub trait PickerDelegate: Sized + 'static { None } fn placeholder_text(&self, _window: &mut Window, _cx: &mut App) -> Arc; - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - "No matches".into() + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + Some("No matches".into()) } fn update_matches( &mut self, @@ -844,18 +844,17 @@ impl Render for Picker { ) }) .when(self.delegate.match_count() == 0, |el| { - el.child( - v_flex().flex_grow().py_2().child( - ListItem::new("empty_state") - .inset(true) - .spacing(ListItemSpacing::Sparse) - .disabled(true) - .child( - Label::new(self.delegate.no_matches_text(window, cx)) - .color(Color::Muted), - ), - ), - ) + el.when_some(self.delegate.no_matches_text(window, cx), |el, text| { + el.child( + v_flex().flex_grow().py_2().child( + ListItem::new("empty_state") + .inset(true) + .spacing(ListItemSpacing::Sparse) + .disabled(true) + .child(Label::new(text).color(Color::Muted)), + ), + ) + }) }) .children(self.delegate.render_footer(window, cx)) .children(match &self.head { diff --git a/crates/project/src/git.rs b/crates/project/src/git.rs index 9bd6e015c4..374ec74c81 100644 --- a/crates/project/src/git.rs +++ b/crates/project/src/git.rs @@ -636,7 +636,7 @@ impl GitStore { repository_handle .update(&mut cx, |repository_handle, _| { - repository_handle.create_branch(branch_name) + repository_handle.create_branch(&branch_name) })? .await??; @@ -656,7 +656,7 @@ impl GitStore { repository_handle .update(&mut cx, |repository_handle, _| { - repository_handle.change_branch(branch_name) + repository_handle.change_branch(&branch_name) })? .await??; @@ -1695,7 +1695,8 @@ impl Repository { }) } - pub fn create_branch(&self, branch_name: String) -> oneshot::Receiver> { + pub fn create_branch(&self, branch_name: &str) -> oneshot::Receiver> { + let branch_name = branch_name.to_owned(); self.send_job(|repo| async move { match repo { GitRepo::Local(git_repository) => git_repository.create_branch(&branch_name), @@ -1720,7 +1721,8 @@ impl Repository { }) } - pub fn change_branch(&self, branch_name: String) -> oneshot::Receiver> { + pub fn change_branch(&self, branch_name: &str) -> oneshot::Receiver> { + let branch_name = branch_name.to_owned(); self.send_job(|repo| async move { match repo { GitRepo::Local(git_repository) => git_repository.change_branch(&branch_name), diff --git a/crates/prompt_library/src/prompt_library.rs b/crates/prompt_library/src/prompt_library.rs index fad47abb23..7b7b13d0b2 100644 --- a/crates/prompt_library/src/prompt_library.rs +++ b/crates/prompt_library/src/prompt_library.rs @@ -179,12 +179,13 @@ impl PickerDelegate for PromptPickerDelegate { self.matches.len() } - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - if self.store.prompt_count() == 0 { + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + let text = if self.store.prompt_count() == 0 { "No prompts.".into() } else { "No prompts found matching your search.".into() - } + }; + Some(text) } fn selected_index(&self) -> usize { diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index f06b3ae701..b7238a7e8d 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -351,12 +351,13 @@ impl PickerDelegate for RecentProjectsDelegate { fn dismissed(&mut self, _window: &mut Window, _: &mut Context>) {} - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - if self.workspaces.is_empty() { + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + let text = if self.workspaces.is_empty() { "Recently opened projects will show up here".into() } else { "No matches".into() - } + }; + Some(text) } fn render_match( diff --git a/crates/remote_server/src/remote_editing_tests.rs b/crates/remote_server/src/remote_editing_tests.rs index d0e1722e6a..0c2b700bce 100644 --- a/crates/remote_server/src/remote_editing_tests.rs +++ b/crates/remote_server/src/remote_editing_tests.rs @@ -1361,7 +1361,7 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA assert_eq!(&remote_branches, &branches_set); - cx.update(|cx| repository.read(cx).change_branch(new_branch.to_string())) + cx.update(|cx| repository.read(cx).change_branch(new_branch)) .await .unwrap() .unwrap(); @@ -1383,23 +1383,15 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA assert_eq!(server_branch.name, branches[2]); // Also try creating a new branch - cx.update(|cx| { - repository - .read(cx) - .create_branch("totally-new-branch".to_string()) - }) - .await - .unwrap() - .unwrap(); + cx.update(|cx| repository.read(cx).create_branch("totally-new-branch")) + .await + .unwrap() + .unwrap(); - cx.update(|cx| { - repository - .read(cx) - .change_branch("totally-new-branch".to_string()) - }) - .await - .unwrap() - .unwrap(); + cx.update(|cx| repository.read(cx).change_branch("totally-new-branch")) + .await + .unwrap() + .unwrap(); cx.run_until_parked(); diff --git a/crates/tab_switcher/src/tab_switcher.rs b/crates/tab_switcher/src/tab_switcher.rs index 7a82bf1965..983e29ec4f 100644 --- a/crates/tab_switcher/src/tab_switcher.rs +++ b/crates/tab_switcher/src/tab_switcher.rs @@ -325,8 +325,8 @@ impl PickerDelegate for TabSwitcherDelegate { Arc::default() } - fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> SharedString { - "No tabs".into() + fn no_matches_text(&self, _window: &mut Window, _cx: &mut App) -> Option { + Some("No tabs".into()) } fn match_count(&self) -> usize {