From 6b980ecad369e2ce4d47c1ab6625f886db1b738d Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Fri, 3 Oct 2025 22:29:28 -0500 Subject: [PATCH] settings_ui: Dynamic navbar filtering (#39494) Closes #ISSUE Release Notes: - N/A *or* Added/Fixed/Improved ... --- Cargo.lock | 1 + crates/settings_ui/Cargo.toml | 1 + crates/settings_ui/src/settings_ui.rs | 328 +++++++++---------- crates/ui/src/components/disclosure.rs | 12 +- crates/ui/src/components/list/list_header.rs | 3 +- crates/ui/src/components/list/list_item.rs | 5 +- crates/ui/src/components/tree_view_item.rs | 14 +- 7 files changed, 183 insertions(+), 181 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c8af92a969373c30a36ba7af19e3fc04e6675b25..547ef3e57e87638d91ac85116cb12be369a95139 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14551,6 +14551,7 @@ dependencies = [ "menu", "node_runtime", "paths", + "pretty_assertions", "project", "serde", "session", diff --git a/crates/settings_ui/Cargo.toml b/crates/settings_ui/Cargo.toml index d331954972c91f9a86ae150276bf9d9559bc8417..b6a741d4dbfe00d44ef685bfe5e226a771669382 100644 --- a/crates/settings_ui/Cargo.toml +++ b/crates/settings_ui/Cargo.toml @@ -46,6 +46,7 @@ paths.workspace = true session.workspace = true settings.workspace = true zlog.workspace = true +pretty_assertions.workspace = true [[example]] name = "ui" diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index 6b62adbf2b871832e8eacfdbe74f084e4e8c620b..959455f3b298cc493116d7de5bd857ac8bc6b12f 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -144,7 +144,6 @@ fn user_settings_data() -> Vec { vec![ SettingsPage { title: "General Page", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("General"), SettingsPageItem::SettingItem(SettingItem { @@ -303,7 +302,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Appearance & Behavior", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Theme"), // todo(settings_ui): Figure out how we want to add these @@ -663,7 +661,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Editor", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Indentation"), // todo(settings_ui): Needs numeric stepper @@ -1360,7 +1357,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Languages & Frameworks", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("General"), SettingsPageItem::SettingItem(SettingItem { @@ -1393,7 +1389,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Workbench & Window", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Workbench"), SettingsPageItem::SettingItem(SettingItem { @@ -1495,7 +1490,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Panels & Tools", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Project Panel"), SettingsPageItem::SettingItem(SettingItem { @@ -1700,7 +1694,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Version Control", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Git"), SettingsPageItem::SettingItem(SettingItem { @@ -1863,7 +1856,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "System & Network", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Network"), // todo(settings_ui): Proxy needs a default @@ -1903,7 +1895,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Diagnostics & Errors", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Display"), SettingsPageItem::SettingItem(SettingItem { @@ -2121,7 +2112,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "Collaboration", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Calls"), SettingsPageItem::SettingItem(SettingItem { @@ -2201,7 +2191,6 @@ fn user_settings_data() -> Vec { }, SettingsPage { title: "AI", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("General"), SettingsPageItem::SettingItem(SettingItem { @@ -2224,7 +2213,6 @@ fn project_settings_data() -> Vec { vec![ SettingsPage { title: "Project", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Worktree Settings Content"), SettingsPageItem::SettingItem(SettingItem { @@ -2244,7 +2232,6 @@ fn project_settings_data() -> Vec { }, SettingsPage { title: "Appearance & Behavior", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Guides"), SettingsPageItem::SettingItem(SettingItem { @@ -2316,7 +2303,6 @@ fn project_settings_data() -> Vec { }, SettingsPage { title: "Editing", - expanded: false, items: vec![ SettingsPageItem::SectionHeader("Indentation"), // todo(settings_ui): Needs numeric stepper @@ -2788,12 +2774,13 @@ struct SubPage { struct NavBarEntry { title: &'static str, is_root: bool, + expanded: bool, page_index: usize, + item_index: Option, } struct SettingsPage { title: &'static str, - expanded: bool, items: Vec, } @@ -3058,66 +3045,80 @@ impl SettingsWindow { let toggle_page_index = self.page_index_from_navbar_index(ix); let selected_page_index = self.page_index_from_navbar_index(self.navbar_entry); - let expanded = &mut self.page_for_navbar_index(ix).expanded; + let expanded = &mut self.navbar_entries[ix].expanded; *expanded = !*expanded; - let expanded = *expanded; // if currently selected page is a child of the parent page we are folding, // set the current page to the parent page - if selected_page_index == toggle_page_index { + if !*expanded && selected_page_index == toggle_page_index { self.navbar_entry = ix; - } else if selected_page_index > toggle_page_index { - let sub_items_count = self.pages[toggle_page_index] - .items - .iter() - .filter(|item| matches!(item, SettingsPageItem::SectionHeader(_))) - .count(); - if expanded { - self.navbar_entry += sub_items_count; - } else { - self.navbar_entry -= sub_items_count; - } } - - self.build_navbar(); } fn build_navbar(&mut self) { let mut navbar_entries = Vec::with_capacity(self.navbar_entries.len()); for (page_index, page) in self.pages.iter().enumerate() { - if !self.search_matches[page_index] - .iter() - .any(|is_match| *is_match) - && !self.search_matches[page_index].is_empty() - { - continue; - } navbar_entries.push(NavBarEntry { title: page.title, is_root: true, + expanded: false, page_index, + item_index: None, }); - if !page.expanded { - continue; - } for (item_index, item) in page.items.iter().enumerate() { let SettingsPageItem::SectionHeader(title) = item else { continue; }; - if !self.search_matches[page_index][item_index] { - continue; - } - navbar_entries.push(NavBarEntry { title, is_root: false, + expanded: false, page_index, + item_index: Some(item_index), }); } } self.navbar_entries = navbar_entries; } + fn visible_navbar_entries(&self) -> impl Iterator { + let mut index = 0; + let entries = &self.navbar_entries; + let search_matches = &self.search_matches; + std::iter::from_fn(move || { + while index < entries.len() { + let entry = &entries[index]; + let included_in_search = if let Some(item_index) = entry.item_index { + search_matches[entry.page_index][item_index] + } else { + search_matches[entry.page_index].iter().any(|b| *b) + || search_matches[entry.page_index].is_empty() + }; + if included_in_search { + break; + } + index += 1; + } + if index >= self.navbar_entries.len() { + return None; + } + let entry = &entries[index]; + let entry_index = index; + + index += 1; + if entry.is_root && !entry.expanded { + while index < entries.len() { + if entries[index].is_root { + break; + } + index += 1; + } + } + + return Some((entry_index, entry)); + }) + } + fn update_matches(&mut self, cx: &mut Context) { self.search_task.take(); let query = self.search_bar.read(cx).text(cx); @@ -3125,7 +3126,6 @@ impl SettingsWindow { for page in &mut self.search_matches { page.fill(true); } - self.build_navbar(); cx.notify(); return; } @@ -3169,7 +3169,7 @@ impl SettingsWindow { candidates.as_slice(), &query, false, - false, + true, candidates.len(), &atomic_bool, cx.background_executor().clone(), @@ -3191,21 +3191,29 @@ impl SettingsWindow { page[header_index] = true; page[item_index] = true; } - this.build_navbar(); - this.navbar_entry = 0; + let first_navbar_entry_index = this + .visible_navbar_entries() + .next() + .map(|e| e.0) + .unwrap_or(0); + this.navbar_entry = first_navbar_entry_index; cx.notify(); }) .ok(); })); } - fn build_ui(&mut self, cx: &mut Context) { - self.pages = self.current_file.pages(); + fn build_search_matches(&mut self) { self.search_matches = self .pages .iter() .map(|page| vec![true; page.items.len()]) .collect::>(); + } + + fn build_ui(&mut self, cx: &mut Context) { + self.pages = self.current_file.pages(); + self.build_search_matches(); self.build_navbar(); if !self.search_bar.read(cx).is_empty(cx) { @@ -3283,25 +3291,20 @@ impl SettingsWindow { "settings-ui-nav-bar", self.navbar_entries.len(), cx.processor(|this, range: Range, _, cx| { - range - .into_iter() - .map(|ix| { - let entry = &this.navbar_entries[ix]; - + this.visible_navbar_entries() + .skip(range.start.saturating_sub(1)) + .take(range.len()) + .map(|(ix, entry)| { TreeViewItem::new(("settings-ui-navbar-entry", ix), entry.title) .root_item(entry.is_root) .toggle_state(this.is_navbar_entry_selected(ix)) .when(entry.is_root, |item| { - item.toggle( - this.pages[this.page_index_from_navbar_index(ix)] - .expanded, - ) - .on_toggle( - cx.listener(move |this, _, _, cx| { + item.expanded(entry.expanded).on_toggle(cx.listener( + move |this, _, _, cx| { this.toggle_navbar_entry(ix); cx.notify(); - }), - ) + }, + )) }) .on_click(cx.listener(move |this, _, _, cx| { this.navbar_entry = ix; @@ -3423,11 +3426,6 @@ impl SettingsWindow { self.navbar_entries[index].page_index } - fn page_for_navbar_index(&mut self, index: usize) -> &mut SettingsPage { - let index = self.page_index_from_navbar_index(index); - &mut self.pages[index] - } - fn is_navbar_entry_selected(&self, ix: usize) -> bool { ix == self.navbar_entry } @@ -3626,10 +3624,6 @@ mod test { use super::*; impl SettingsWindow { - fn navbar(&self) -> &[NavBarEntry] { - self.navbar_entries.as_slice() - } - fn navbar_entry(&self) -> usize { self.navbar_entry } @@ -3642,6 +3636,7 @@ mod test { } fn build(mut self) -> Self { + self.build_search_matches(); self.build_navbar(); self } @@ -3653,7 +3648,6 @@ mod test { ) -> Self { let page = SettingsPage { title, - expanded: false, items: Vec::default(), }; @@ -3671,13 +3665,25 @@ mod test { fn assert_search_results(&self, other: &Self) { // page index could be different because of filtered out pages - assert!( - self.navbar_entries - .iter() - .zip(other.navbar_entries.iter()) - .all(|(entry, other)| { - entry.is_root == other.is_root && entry.title == other.title + #[derive(Debug, PartialEq)] + struct EntryMinimal { + is_root: bool, + title: &'static str, + } + pretty_assertions::assert_eq!( + other + .visible_navbar_entries() + .map(|(_, entry)| EntryMinimal { + is_root: entry.is_root, + title: entry.title, + }) + .collect::>(), + self.visible_navbar_entries() + .map(|(_, entry)| EntryMinimal { + is_root: entry.is_root, + title: entry.title, }) + .collect::>(), ); assert_eq!( self.current_page().items.iter().collect::>(), @@ -3719,55 +3725,50 @@ mod test { fn parse(input: &'static str, window: &mut Window, cx: &mut App) -> SettingsWindow { let mut pages: Vec = Vec::new(); - let mut current_page = None; + let mut expanded_pages = Vec::new(); let mut selected_idx = None; - let mut ix = 0; - let mut in_closed_subentry = false; + let mut index = 0; + let mut in_expanded_section = false; for mut line in input .lines() .map(|line| line.trim()) .filter(|line| !line.is_empty()) { - let mut is_selected = false; - if line.ends_with("*") { - assert!( - selected_idx.is_none(), - "Can only have one selected navbar entry at a time" - ); - selected_idx = Some(ix); - line = &line[..line.len() - 1]; - is_selected = true; + if let Some(pre) = line.strip_suffix('*') { + assert!(selected_idx.is_none(), "Only one selected entry allowed"); + selected_idx = Some(index); + line = pre; } - - if line.starts_with("v") || line.starts_with(">") { - if let Some(current_page) = current_page.take() { - pages.push(current_page); - } - - let expanded = line.starts_with("v"); - in_closed_subentry = !expanded; - ix += 1; - - current_page = Some(SettingsPage { - title: line.split_once(" ").unwrap().1, - expanded, - items: Vec::default(), + let (kind, title) = line.split_once(" ").unwrap(); + assert_eq!(kind.len(), 1); + let kind = kind.chars().next().unwrap(); + if kind == 'v' { + let page_idx = pages.len(); + expanded_pages.push(page_idx); + pages.push(SettingsPage { + title, + items: vec![], + }); + index += 1; + in_expanded_section = true; + } else if kind == '>' { + pages.push(SettingsPage { + title, + items: vec![], }); - } else if line.starts_with("-") { - if !in_closed_subentry { - ix += 1; - } else if is_selected && in_closed_subentry { - panic!("Can't select sub entry if it's parent is closed"); + index += 1; + in_expanded_section = false; + } else if kind == '-' { + pages + .last_mut() + .unwrap() + .items + .push(SettingsPageItem::SectionHeader(title)); + if selected_idx == Some(index) && !in_expanded_section { + panic!("Items in unexpanded sections cannot be selected"); } - - let Some(current_page) = current_page.as_mut() else { - panic!("Sub entries must be within a page"); - }; - - current_page.items.push(SettingsPageItem::SectionHeader( - line.split_once(" ").unwrap().1, - )); + index += 1; } else { panic!( "Entries must start with one of 'v', '>', or '-'\n line: {}", @@ -3776,15 +3777,6 @@ mod test { } } - if let Some(current_page) = current_page.take() { - pages.push(current_page); - } - - let search_matches = pages - .iter() - .map(|page| vec![true; page.items.len()]) - .collect::>(); - let mut settings_window = SettingsWindow { files: Vec::default(), current_file: crate::SettingsUiFile::User, @@ -3793,43 +3785,70 @@ mod test { navbar_entry: selected_idx.expect("Must have a selected navbar entry"), navbar_entries: Vec::default(), list_handle: UniformListScrollHandle::default(), - search_matches, + search_matches: vec![], search_task: None, sub_page_stack: vec![], }; + settings_window.build_search_matches(); settings_window.build_navbar(); + for expanded_page_index in expanded_pages { + for entry in &mut settings_window.navbar_entries { + if entry.page_index == expanded_page_index && entry.is_root { + entry.expanded = true; + } + } + } settings_window } #[track_caller] fn check_navbar_toggle( before: &'static str, - toggle_idx: usize, + toggle_page: &'static str, after: &'static str, window: &mut Window, cx: &mut App, ) { let mut settings_window = parse(before, window, cx); + let toggle_page_idx = settings_window + .pages + .iter() + .position(|page| page.title == toggle_page) + .expect("page not found"); + let toggle_idx = settings_window + .navbar_entries + .iter() + .position(|entry| entry.page_index == toggle_page_idx) + .expect("page not found"); settings_window.toggle_navbar_entry(toggle_idx); let expected_settings_window = parse(after, window, cx); - assert_eq!(settings_window.navbar(), expected_settings_window.navbar()); - assert_eq!( - settings_window.navbar_entry(), - expected_settings_window.navbar_entry() + pretty_assertions::assert_eq!( + settings_window + .visible_navbar_entries() + .map(|(_, entry)| entry) + .collect::>(), + expected_settings_window + .visible_navbar_entries() + .map(|(_, entry)| entry) + .collect::>(), + ); + pretty_assertions::assert_eq!( + settings_window.navbar_entries[settings_window.navbar_entry()], + expected_settings_window.navbar_entries[expected_settings_window.navbar_entry()], ); } macro_rules! check_navbar_toggle { - ($name:ident, before: $before:expr, toggle_idx: $toggle_idx:expr, after: $after:expr) => { + ($name:ident, before: $before:expr, toggle_page: $toggle_page:expr, after: $after:expr) => { #[gpui::test] fn $name(cx: &mut gpui::TestAppContext) { let window = cx.add_empty_window(); window.update(|window, cx| { register_settings(cx); - check_navbar_toggle($before, $toggle_idx, $after, window, cx); + check_navbar_toggle($before, $toggle_page, $after, window, cx); }); } }; @@ -3844,7 +3863,7 @@ mod test { v Project - Project Settings ", - toggle_idx: 0, + toggle_page: "General", after: r" > General* v Project @@ -3861,7 +3880,7 @@ mod test { v Project - Project Settings ", - toggle_idx: 0, + toggle_page: "General", after: r" v General* - General @@ -3880,7 +3899,7 @@ mod test { v Project - Project Settings* ", - toggle_idx: 1, + toggle_page: "Project", after: r" > General > Project* @@ -3899,7 +3918,7 @@ mod test { - General > Appearance & Behavior ", - toggle_idx: 3, + toggle_page: "Project", after: r" v General Page - General @@ -3923,7 +3942,7 @@ mod test { - General* > Appearance & Behavior ", - toggle_idx: 0, + toggle_page: "General Page", after: r" > General Page v Project @@ -3946,7 +3965,7 @@ mod test { - General* > Appearance & Behavior ", - toggle_idx: 0, + toggle_page: "General Page", after: r" v General Page - General @@ -3959,31 +3978,6 @@ mod test { " ); - check_navbar_toggle!( - navbar_toggle_sub_entry_does_nothing, - before: r" - > General Page - - General - - Privacy - v Project - - Worktree Settings Content - v AI - - General* - > Appearance & Behavior - ", - toggle_idx: 4, - after: r" - > General Page - - General - - Privacy - v Project - - Worktree Settings Content - v AI - - General* - > Appearance & Behavior - " - ); - #[gpui::test] fn test_basic_search(cx: &mut gpui::TestAppContext) { let cx = cx.add_empty_window(); diff --git a/crates/ui/src/components/disclosure.rs b/crates/ui/src/components/disclosure.rs index 4bb3419176eb074c85cc7837d23a10816ce81596..84282db2e332dc5d39cde2b3aae8d8d181a1024c 100644 --- a/crates/ui/src/components/disclosure.rs +++ b/crates/ui/src/components/disclosure.rs @@ -10,7 +10,7 @@ pub struct Disclosure { is_open: bool, selected: bool, disabled: bool, - on_toggle: Option>, + on_toggle_expanded: Option>, cursor_style: CursorStyle, opened_icon: IconName, closed_icon: IconName, @@ -24,7 +24,7 @@ impl Disclosure { is_open, selected: false, disabled: false, - on_toggle: None, + on_toggle_expanded: None, cursor_style: CursorStyle::PointingHand, opened_icon: IconName::ChevronDown, closed_icon: IconName::ChevronRight, @@ -32,11 +32,11 @@ impl Disclosure { } } - pub fn on_toggle( + pub fn on_toggle_expanded( mut self, handler: impl Into>>, ) -> Self { - self.on_toggle = handler.into(); + self.on_toggle_expanded = handler.into(); self } @@ -65,7 +65,7 @@ impl Toggleable for Disclosure { impl Clickable for Disclosure { fn on_click(mut self, handler: impl Fn(&ClickEvent, &mut Window, &mut App) + 'static) -> Self { - self.on_toggle = Some(Arc::new(handler)); + self.on_toggle_expanded = Some(Arc::new(handler)); self } @@ -99,7 +99,7 @@ impl RenderOnce for Disclosure { .when_some(self.visible_on_hover.clone(), |this, group_name| { this.visible_on_hover(group_name) }) - .when_some(self.on_toggle, move |this, on_toggle| { + .when_some(self.on_toggle_expanded, move |this, on_toggle| { this.on_click(move |event, window, cx| on_toggle(event, window, cx)) }) } diff --git a/crates/ui/src/components/list/list_header.rs b/crates/ui/src/components/list/list_header.rs index d64f5c7263a4c395a48591a9454c0b518e541a82..d59af07fa5271c070fca8433156b94301cc134aa 100644 --- a/crates/ui/src/components/list/list_header.rs +++ b/crates/ui/src/components/list/list_header.rs @@ -107,7 +107,8 @@ impl RenderOnce for ListHeader { h_flex() .gap(DynamicSpacing::Base04.rems(cx)) .children(self.toggle.map(|is_open| { - Disclosure::new("toggle", is_open).on_toggle(self.on_toggle.clone()) + Disclosure::new("toggle", is_open) + .on_toggle_expanded(self.on_toggle.clone()) })) .child( div() diff --git a/crates/ui/src/components/list/list_item.rs b/crates/ui/src/components/list/list_item.rs index 6356930aee8cc38a9d793428544b8ca33f3966ff..a58291438a1d10bb1b61149f412151375b6b0a1f 100644 --- a/crates/ui/src/components/list/list_item.rs +++ b/crates/ui/src/components/list/list_item.rs @@ -308,7 +308,10 @@ impl RenderOnce for ListItem { .when(is_open && !self.always_show_disclosure_icon, |this| { this.visible_on_hover("") }) - .child(Disclosure::new("toggle", is_open).on_toggle(self.on_toggle)) + .child( + Disclosure::new("toggle", is_open) + .on_toggle_expanded(self.on_toggle), + ) })) .child( h_flex() diff --git a/crates/ui/src/components/tree_view_item.rs b/crates/ui/src/components/tree_view_item.rs index 307fe1496b21913c5620ed7c871dd9679d422a41..c57d071075ea0c0479f7700d9c52da2692a42d8d 100644 --- a/crates/ui/src/components/tree_view_item.rs +++ b/crates/ui/src/components/tree_view_item.rs @@ -9,7 +9,7 @@ pub struct TreeViewItem { id: ElementId, group_name: Option, label: SharedString, - toggle: bool, + expanded: bool, selected: bool, disabled: bool, focused: bool, @@ -28,7 +28,7 @@ impl TreeViewItem { id: id.into(), group_name: None, label: label.into(), - toggle: false, + expanded: false, selected: false, disabled: false, focused: false, @@ -73,8 +73,8 @@ impl TreeViewItem { self } - pub fn toggle(mut self, toggle: bool) -> Self { - self.toggle = toggle; + pub fn expanded(mut self, toggle: bool) -> Self { + self.expanded = toggle; self } @@ -161,10 +161,12 @@ impl RenderOnce for TreeViewItem { }) .hover(|s| s.bg(cx.theme().colors().element_hover)) .child( - Disclosure::new("toggle", self.toggle) + Disclosure::new("toggle", self.expanded) .when_some( self.on_toggle.clone(), - |disclosure, on_toggle| disclosure.on_toggle(on_toggle), + |disclosure, on_toggle| { + disclosure.on_toggle_expanded(on_toggle) + }, ) .opened_icon(IconName::ChevronDown) .closed_icon(IconName::ChevronRight),