From 66e2028313ff937373ff8b961e8ecc3ecb1497df Mon Sep 17 00:00:00 2001 From: Nate Butler Date: Fri, 31 Jan 2025 22:32:41 -0500 Subject: [PATCH] git_ui: More git panel refinement (#24065) - Removes flakey keybindings from buttons - Moves git panel entries to use a standard ListItem - Show a repo selector in the panel when more than one repo is present - Remove temporary repo selector from title bar Release Notes: - N/A --- Cargo.lock | 1 - crates/git_ui/src/git_panel.rs | 447 +++++++++++------------ crates/git_ui/src/repository_selector.rs | 17 +- crates/title_bar/Cargo.toml | 1 - crates/title_bar/src/title_bar.rs | 53 +-- 5 files changed, 207 insertions(+), 312 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 211c27c9b27d2d..1237f24ac42772 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13520,7 +13520,6 @@ dependencies = [ "client", "collections", "feature_flags", - "git_ui", "gpui", "http_client", "notifications", diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index e928a4b9c4aea2..cef3155bf57b96 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -1,5 +1,8 @@ use crate::git_panel_settings::StatusStyle; -use crate::{git_panel_settings::GitPanelSettings, git_status_icon}; +use crate::repository_selector::RepositorySelectorPopoverMenu; +use crate::{ + git_panel_settings::GitPanelSettings, git_status_icon, repository_selector::RepositorySelector, +}; use anyhow::Result; use db::kvp::KEY_VALUE_STORE; use editor::actions::MoveToEnd; @@ -19,7 +22,8 @@ use settings::Settings as _; use std::{collections::HashSet, ops::Range, path::PathBuf, sync::Arc, time::Duration, usize}; use theme::ThemeSettings; use ui::{ - prelude::*, Checkbox, Divider, DividerColor, ElevationIndex, Scrollbar, ScrollbarState, Tooltip, + prelude::*, ButtonLike, Checkbox, Divider, DividerColor, ElevationIndex, ListItem, + ListItemSpacing, Scrollbar, ScrollbarState, Tooltip, }; use util::{ResultExt, TryFutureExt}; use workspace::notifications::{DetachAndPromptErr, NotificationId}; @@ -91,6 +95,7 @@ pub struct GitPanel { selected_entry: Option, show_scrollbar: bool, update_visible_entries_task: Task<()>, + repository_selector: Entity, commit_editor: Entity, visible_entries: Vec, all_staged: Option, @@ -185,6 +190,9 @@ impl GitPanel { .detach(); } + let repository_selector = + cx.new(|cx| RepositorySelector::new(project.clone(), window, cx)); + let mut git_panel = Self { focus_handle: cx.focus_handle(), pending_serialization: Task::ready(None), @@ -194,6 +202,7 @@ impl GitPanel { width: Some(px(360.)), scrollbar_state: ScrollbarState::new(scroll_handle.clone()) .parent_model(&cx.entity()), + repository_selector, selected_entry: None, show_scrollbar: false, hide_scrollbar_task: None, @@ -828,11 +837,16 @@ impl GitPanel { pub fn render_panel_header( &self, - window: &mut Window, - has_write_access: bool, + _window: &mut Window, cx: &mut Context, ) -> impl IntoElement { let focus_handle = self.focus_handle(cx).clone(); + let all_repositories = self + .project + .read(cx) + .git_state() + .map(|state| state.read(cx).all_repositories()) + .unwrap_or_default(); let entry_count = self .active_repository .as_ref() @@ -844,123 +858,102 @@ impl GitPanel { n => format!("{} changes", n), }; - // for our use case treat None as false - let all_staged = self.all_staged.unwrap_or(false); - h_flex() .h(px(32.)) .items_center() .px_2() .bg(ElevationIndex::Surface.bg(cx)) - .child( - h_flex() - .gap_2() + .child(h_flex().gap_2().child(if all_repositories.len() <= 1 { + div() + .id("changes-label") + .text_buffer(cx) + .text_ui_sm(cx) .child( - Checkbox::new( - "all-changes", - if entry_count == 0 { - ToggleState::Selected - } else { - self.all_staged - .map_or(ToggleState::Indeterminate, ToggleState::from) - }, - ) - .fill() - .elevation(ElevationIndex::Surface) - .tooltip(if all_staged { - Tooltip::text("Unstage all changes") - } else { - Tooltip::text("Stage all changes") - }) - .disabled(!has_write_access || entry_count == 0) - .on_click(cx.listener( - move |git_panel, _, window, cx| match all_staged { - true => git_panel.unstage_all(&UnstageAll, window, cx), - false => git_panel.stage_all(&StageAll, window, cx), - }, - )), + Label::new(changes_string) + .single_line() + .size(LabelSize::Small), ) - .child( - div() - .id("changes-checkbox-label") - .text_buffer(cx) - .text_ui_sm(cx) - .child(changes_string) - .on_click(cx.listener( - move |git_panel, _, window, cx| match all_staged { - true => git_panel.unstage_all(&UnstageAll, window, cx), - false => git_panel.stage_all(&StageAll, window, cx), - }, - )), - ), - ) + .into_any_element() + } else { + self.render_repository_selector(cx).into_any_element() + })) .child(div().flex_grow()) - .child( - h_flex() - .gap_2() - // TODO: Re-add once revert all is added - // .child( - // IconButton::new("discard-changes", IconName::Undo) - // .tooltip({ - // let focus_handle = focus_handle.clone(); - // move |cx| { - // Tooltip::for_action_in( - // "Discard all changes", - // &RevertAll, - // &focus_handle, - // cx, - // ) - // } - // }) - // .icon_size(IconSize::Small) - // .disabled(true), - // ) - .child(if self.all_staged.unwrap_or(false) { - self.panel_button("unstage-all", "Unstage All") - .tooltip({ - let focus_handle = focus_handle.clone(); - move |window, cx| { - Tooltip::for_action_in( - "Unstage all changes", - &UnstageAll, - &focus_handle, - window, - cx, - ) - } - }) - .key_binding(ui::KeyBinding::for_action_in( + .child(h_flex().gap_2().child(if self.all_staged.unwrap_or(false) { + self.panel_button("unstage-all", "Unstage All") + .tooltip({ + let focus_handle = focus_handle.clone(); + move |window, cx| { + Tooltip::for_action_in( + "Unstage all changes", &UnstageAll, &focus_handle, window, - )) - .on_click(cx.listener(move |this, _, window, cx| { - this.unstage_all(&UnstageAll, window, cx) - })) - } else { - self.panel_button("stage-all", "Stage All") - .tooltip({ - let focus_handle = focus_handle.clone(); - move |window, cx| { - Tooltip::for_action_in( - "Stage all changes", - &StageAll, - &focus_handle, - window, - cx, - ) - } - }) - .key_binding(ui::KeyBinding::for_action_in( + cx, + ) + } + }) + .on_click(cx.listener(move |this, _, window, cx| { + this.unstage_all(&UnstageAll, window, cx) + })) + } else { + self.panel_button("stage-all", "Stage All") + .tooltip({ + let focus_handle = focus_handle.clone(); + move |window, cx| { + Tooltip::for_action_in( + "Stage all changes", &StageAll, &focus_handle, window, - )) - .on_click(cx.listener(move |this, _, window, cx| { - this.stage_all(&StageAll, window, cx) - })) - }), - ) + cx, + ) + } + }) + .on_click( + cx.listener(move |this, _, window, cx| { + this.stage_all(&StageAll, window, cx) + }), + ) + })) + } + + pub fn render_repository_selector(&self, cx: &mut Context) -> impl IntoElement { + let active_repository = self.project.read(cx).active_repository(cx); + let repository_display_name = active_repository + .as_ref() + .map(|repo| repo.display_name(self.project.read(cx), cx)) + .unwrap_or_default(); + + let entry_count = self.visible_entries.len(); + + RepositorySelectorPopoverMenu::new( + self.repository_selector.clone(), + ButtonLike::new("active-repository") + .style(ButtonStyle::Subtle) + .child( + h_flex().w_full().gap_0p5().child( + div() + .overflow_x_hidden() + .flex_grow() + .whitespace_nowrap() + .child( + h_flex() + .gap_1() + .child( + Label::new(repository_display_name).size(LabelSize::Small), + ) + .when(entry_count > 0, |flex| { + flex.child( + Label::new(format!("({})", entry_count)) + .size(LabelSize::Small) + .color(Color::Muted), + ) + }) + .into_any_element(), + ), + ), + ), + ) } pub fn render_commit_editor( @@ -1041,12 +1034,10 @@ impl GitPanel { .absolute() .bottom_2p5() .right_3() + .gap_1p5() .child(div().gap_1().flex_grow()) - .child(if self.current_modifiers.alt { - commit_all_button - } else { - commit_staged_button - }), + .child(commit_all_button) + .child(commit_staged_button), ), ) } @@ -1124,7 +1115,7 @@ impl GitPanel { fn render_entries(&self, has_write_access: bool, cx: &mut Context) -> impl IntoElement { let entry_count = self.visible_entries.len(); - h_flex() + v_flex() .size_full() .overflow_hidden() .child( @@ -1140,12 +1131,15 @@ impl GitPanel { .size_full() .with_sizing_behavior(ListSizingBehavior::Infer) .with_horizontal_sizing_behavior(ListHorizontalSizingBehavior::Unconstrained) - // .with_width_from_item(self.max_width_item_index) .track_scroll(self.scroll_handle.clone()), ) .children(self.render_scrollbar(cx)) } + fn entry_label(&self, label: impl Into, color: Color) -> Label { + Label::new(label.into()).color(color).single_line() + } + fn render_entry( &self, ix: usize, @@ -1157,149 +1151,117 @@ impl GitPanel { let selected = self.selected_entry == Some(ix); let status_style = GitPanelSettings::get_global(cx).status_style; let status = entry_details.status; - - let mut label_color = cx.theme().colors().text; - if status_style == StatusStyle::LabelColor { - label_color = if status.is_conflicted() { - cx.theme().colors().version_control_conflict - } else if status.is_modified() { - cx.theme().colors().version_control_modified - } else if status.is_deleted() { - // Don't use `version_control_deleted` here or all the - // deleted entries will be likely a red color. - cx.theme().colors().text_disabled + let has_conflict = status.is_conflicted(); + let is_modified = status.is_modified(); + let is_deleted = status.is_deleted(); + + let label_color = if status_style == StatusStyle::LabelColor { + if has_conflict { + Color::Conflict + } else if is_modified { + Color::Modified + } else if is_deleted { + // We don't want a bunch of red labels in the list + Color::Disabled } else { - cx.theme().colors().version_control_added + Color::Created } - } + } else { + Color::Default + }; - let path_color = status - .is_deleted() - .then_some(cx.theme().colors().text_disabled) - .unwrap_or(cx.theme().colors().text_muted); + let path_color = if status.is_deleted() { + Color::Disabled + } else { + Color::Muted + }; - let entry_id = ElementId::Name(format!("entry_{}", entry_details.display_name).into()); - let checkbox_id = - ElementId::Name(format!("checkbox_{}", entry_details.display_name).into()); - let is_tree_view = false; - let handle = cx.entity().downgrade(); + let id: ElementId = ElementId::Name(format!("entry_{}", entry_details.display_name).into()); - let end_slot = h_flex() - .invisible() - .when(selected, |this| this.visible()) - .when(!selected, |this| { - this.group_hover("git-panel-entry", |this| this.visible()) - }) - .gap_1() - .items_center() - .child( - IconButton::new("more", IconName::EllipsisVertical) - .icon_color(Color::Placeholder) - .icon_size(IconSize::Small), - ); - - let mut entry = h_flex() - .id(entry_id) - .group("git-panel-entry") - .h(px(28.)) - .w_full() - .pr(px(4.)) - .items_center() - .gap_2() - .font_buffer(cx) - .text_ui_sm(cx) - .when(!selected, |this| { - this.hover(|this| this.bg(cx.theme().colors().ghost_element_hover)) - }); + let checkbox = Checkbox::new( + id, + entry_details + .is_staged + .map_or(ToggleState::Indeterminate, ToggleState::from), + ) + .disabled(!has_write_access) + .fill() + .elevation(ElevationIndex::Surface) + .on_click({ + let handle = cx.entity().downgrade(); + let repo_path = repo_path.clone(); + move |toggle, _window, cx| { + let Some(this) = handle.upgrade() else { + return; + }; + this.update(cx, |this, cx| { + this.visible_entries[ix].is_staged = match *toggle { + ToggleState::Selected => Some(true), + ToggleState::Unselected => Some(false), + ToggleState::Indeterminate => None, + }; + let repo_path = repo_path.clone(); + let Some(active_repository) = this.active_repository.as_ref() else { + return; + }; + let result = match toggle { + ToggleState::Selected | ToggleState::Indeterminate => active_repository + .stage_entries(vec![repo_path], this.err_sender.clone()), + ToggleState::Unselected => active_repository + .unstage_entries(vec![repo_path], this.err_sender.clone()), + }; + if let Err(e) = result { + this.show_err_toast("toggle staged error", e, cx); + } + }); + } + }); - if is_tree_view { - entry = entry.pl(px(8. + 12. * entry_details.depth as f32)) - } else { - entry = entry.pl(px(8.)) - } + let start_slot = h_flex() + .gap(DynamicSpacing::Base04.rems(cx)) + .child(checkbox) + .child(git_status_icon(status, cx)); - if selected { - entry = entry.bg(cx.theme().status().info_background); - } + let id = ElementId::Name(format!("entry_{}", entry_details.display_name).into()); - entry = entry - .child( - Checkbox::new( - checkbox_id, - entry_details - .is_staged - .map_or(ToggleState::Indeterminate, ToggleState::from), - ) + div().w_full().px_0p5().child( + ListItem::new(id) + .spacing(ListItemSpacing::Sparse) + .start_slot(start_slot) + .toggle_state(selected) .disabled(!has_write_access) - .fill() - .elevation(ElevationIndex::Surface) .on_click({ - let handle = handle.clone(); - let repo_path = repo_path.clone(); - move |toggle, _window, cx| { + let handle = cx.entity().downgrade(); + move |_, window, cx| { let Some(this) = handle.upgrade() else { return; }; this.update(cx, |this, cx| { - this.visible_entries[ix].is_staged = match *toggle { - ToggleState::Selected => Some(true), - ToggleState::Unselected => Some(false), - ToggleState::Indeterminate => None, - }; - let repo_path = repo_path.clone(); - let Some(active_repository) = this.active_repository.as_ref() else { - return; - }; - let result = match toggle { - ToggleState::Selected | ToggleState::Indeterminate => { - active_repository - .stage_entries(vec![repo_path], this.err_sender.clone()) - } - ToggleState::Unselected => active_repository - .unstage_entries(vec![repo_path], this.err_sender.clone()), - }; - if let Err(e) = result { - this.show_err_toast("toggle staged error", e, cx); - } + this.selected_entry = Some(ix); + window.dispatch_action(Box::new(OpenSelected), cx); + cx.notify(); }); } - }), - ) - .when(status_style == StatusStyle::Icon, |this| { - this.child(git_status_icon(status, cx)) - }) - .child( - h_flex() - .text_color(label_color) - .when(status.is_deleted(), |this| this.line_through()) - .when_some(repo_path.parent(), |this, parent| { - let parent_str = parent.to_string_lossy(); - if !parent_str.is_empty() { - this.child( - div() - .text_color(path_color) - .child(format!("{}/", parent_str)), - ) - } else { - this - } - }) - .child(div().child(entry_details.display_name.clone())), - ) - .child(div().flex_1()) - .child(end_slot) - .on_click(move |_, window, cx| { - // TODO: add `select_entry` method then do after that - window.dispatch_action(Box::new(OpenSelected), cx); - - handle - .update(cx, |git_panel, _| { - git_panel.selected_entry = Some(ix); - }) - .ok(); - }); - - entry + }) + .child( + h_flex() + .when_some(repo_path.parent(), |this, parent| { + let parent_str = parent.to_string_lossy(); + if !parent_str.is_empty() { + this.child( + self.entry_label(format!("{}/", parent_str), path_color) + .when(status.is_deleted(), |this| this.strikethrough(true)), + ) + } else { + this + } + }) + .child( + self.entry_label(entry_details.display_name.clone(), label_color) + .when(status.is_deleted(), |this| this.strikethrough(true)), + ), + ), + ) } } @@ -1426,10 +1388,9 @@ impl Render for GitPanel { })) .size_full() .overflow_hidden() - .font_buffer(cx) .py_1() .bg(ElevationIndex::Surface.bg(cx)) - .child(self.render_panel_header(window, has_write_access, cx)) + .child(self.render_panel_header(window, cx)) .child(self.render_divider(cx)) .child(if has_entries { self.render_entries(has_write_access, cx).into_any_element() diff --git a/crates/git_ui/src/repository_selector.rs b/crates/git_ui/src/repository_selector.rs index 8c02c66218e5cd..a41c966845dda7 100644 --- a/crates/git_ui/src/repository_selector.rs +++ b/crates/git_ui/src/repository_selector.rs @@ -34,7 +34,8 @@ impl RepositorySelector { }; let picker = cx.new(|cx| { - Picker::uniform_list(delegate, window, cx).max_height(Some(rems(20.).into())) + Picker::nonsearchable_uniform_list(delegate, window, cx) + .max_height(Some(rems(20.).into())) }); let _subscriptions = if let Some(git_state) = git_state { @@ -236,18 +237,4 @@ impl PickerDelegate for RepositorySelectorDelegate { .child(Label::new(display_name)), ) } - - fn render_footer( - &self, - _window: &mut Window, - cx: &mut Context>, - ) -> Option { - // TODO: Implement footer rendering if needed - Some( - div() - .text_ui_sm(cx) - .child("Temporary location for repo selector") - .into_any_element(), - ) - } } diff --git a/crates/title_bar/Cargo.toml b/crates/title_bar/Cargo.toml index 42b05d28b391af..cc65ad3f642188 100644 --- a/crates/title_bar/Cargo.toml +++ b/crates/title_bar/Cargo.toml @@ -47,7 +47,6 @@ util.workspace = true telemetry.workspace = true workspace.workspace = true zed_actions.workspace = true -git_ui.workspace = true zed_predict_onboarding.workspace = true [target.'cfg(windows)'.dependencies] diff --git a/crates/title_bar/src/title_bar.rs b/crates/title_bar/src/title_bar.rs index 7ed4d4609a048d..2f325335b26fbe 100644 --- a/crates/title_bar/src/title_bar.rs +++ b/crates/title_bar/src/title_bar.rs @@ -15,9 +15,7 @@ use crate::platforms::{platform_linux, platform_mac, platform_windows}; use auto_update::AutoUpdateStatus; use call::ActiveCall; use client::{Client, UserStore}; -use feature_flags::{FeatureFlagAppExt, GitUiFeatureFlag, ZedPro}; -use git_ui::repository_selector::RepositorySelector; -use git_ui::repository_selector::RepositorySelectorPopoverMenu; +use feature_flags::{FeatureFlagAppExt, ZedPro}; use gpui::{ actions, div, px, Action, AnyElement, App, Context, Decorations, Element, Entity, InteractiveElement, Interactivity, IntoElement, MouseButton, ParentElement, Render, Stateful, @@ -27,7 +25,6 @@ use project::Project; use rpc::proto; use settings::Settings as _; use smallvec::SmallVec; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use theme::ActiveTheme; use ui::{ @@ -105,7 +102,6 @@ pub struct TitleBar { platform_style: PlatformStyle, content: Stateful
, children: SmallVec<[AnyElement; 2]>, - repository_selector: Entity, project: Entity, user_store: Entity, client: Arc, @@ -113,7 +109,6 @@ pub struct TitleBar { should_move: bool, application_menu: Option>, _subscriptions: Vec, - git_ui_enabled: Arc, zed_predict_banner: Entity, } @@ -191,7 +186,6 @@ impl Render for TitleBar { title_bar .children(self.render_project_host(cx)) .child(self.render_project_name(cx)) - .children(self.render_current_repository(cx)) .children(self.render_project_branch(cx)) }) }) @@ -302,14 +296,6 @@ impl TitleBar { subscriptions.push(cx.observe_window_activation(window, Self::window_activation_changed)); subscriptions.push(cx.observe(&user_store, |_, _, cx| cx.notify())); - let is_git_ui_enabled = Arc::new(AtomicBool::new(false)); - subscriptions.push(cx.observe_flag::({ - let is_git_ui_enabled = is_git_ui_enabled.clone(); - move |enabled, _cx| { - is_git_ui_enabled.store(enabled, Ordering::SeqCst); - } - })); - let zed_predict_banner = cx.new(|cx| { ZedPredictBanner::new( workspace.weak_handle(), @@ -325,14 +311,12 @@ impl TitleBar { content: div().id(id.into()), children: SmallVec::new(), application_menu, - repository_selector: cx.new(|cx| RepositorySelector::new(project.clone(), window, cx)), workspace: workspace.weak_handle(), should_move: false, project, user_store, client, _subscriptions: subscriptions, - git_ui_enabled: is_git_ui_enabled, zed_predict_banner, } } @@ -515,41 +499,6 @@ impl TitleBar { })) } - // NOTE: Not sure we want to keep this in the titlebar, but for while we are working on Git it is helpful in the short term - pub fn render_current_repository(&self, cx: &mut Context) -> Option { - if !self.git_ui_enabled.load(Ordering::SeqCst) { - return None; - } - - let active_repository = self.project.read(cx).active_repository(cx)?; - let display_name = active_repository.display_name(self.project.read(cx), cx); - - // TODO: what to render if no active repository? - Some(RepositorySelectorPopoverMenu::new( - self.repository_selector.clone(), - ButtonLike::new("active-repository") - .style(ButtonStyle::Subtle) - .child( - h_flex().w_full().gap_0p5().child( - div() - .overflow_x_hidden() - .flex_grow() - .whitespace_nowrap() - .child( - h_flex() - .gap_1() - .child( - Label::new(display_name) - .size(LabelSize::Small) - .color(Color::Muted), - ) - .into_any_element(), - ), - ), - ), - )) - } - pub fn render_project_branch(&self, cx: &mut Context) -> Option { let entry = { let mut names_and_branches =