From a701388cb7042198f863c6a81b28276ef1942390 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 30 Sep 2025 16:12:13 -0400 Subject: [PATCH] settings ui: Implement settings search (#38989) Get a basic search implementation working in the settings ui and fix nav bar toggling bugs. Search functionality works by passing in each page and its items into our fuzzy search crate and filtering out any non-matches. A page is a match if any of its items are a match and an item is a match if its title or description has a fuzzy score greater than zero. In the future, a page section header will be filtered out if none of its children has a match or it will show all its children on a match. The team still has to decide what to do in that edge case, but that's the last step until search is fully implemented for our initial launch. Finally, I found some bugs in our nav bar toggling that occurred because we weren't taking into account the index change that occurred when toggling an element with children that is above the selected nav bar entry. I added tests to cover those edge cases as well. Release Notes: - N/A --------- Co-authored-by: Ben Kunkle --- Cargo.lock | 1 + crates/settings_ui/Cargo.toml | 16 +- crates/settings_ui/src/settings_ui.rs | 529 ++++++++++++++++++++++---- 3 files changed, 471 insertions(+), 75 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6f2089b31e2910b9915f90fc1edadc2144c3daf8..e4d94986664bda1ed1549340ad475c29440a2209 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14539,6 +14539,7 @@ dependencies = [ "feature_flags", "fs", "futures 0.3.31", + "fuzzy", "gpui", "language", "menu", diff --git a/crates/settings_ui/Cargo.toml b/crates/settings_ui/Cargo.toml index 7b0c3200ac37fa27b5c376d42348239fe7c161fa..75508164382cf848e213efc5accb94e5faf392d0 100644 --- a/crates/settings_ui/Cargo.toml +++ b/crates/settings_ui/Cargo.toml @@ -16,14 +16,15 @@ default = [] test-support = [] [dependencies] -project.workspace = true -fs.workspace = true anyhow.workspace = true command_palette_hooks.workspace = true editor.workspace = true feature_flags.workspace = true +fs.workspace = true +fuzzy.workspace = true gpui.workspace = true menu.workspace = true +project.workspace = true serde.workspace = true settings.workspace = true strum.workspace = true @@ -34,15 +35,16 @@ workspace-hack.workspace = true workspace.workspace = true [dev-dependencies] -settings.workspace = true +assets.workspace = true +client.workspace = true futures.workspace = true +gpui = { workspace = true, features = ["test-support"] } language.workspace = true -assets.workspace = true +node_runtime.workspace = true paths.workspace = true -zlog.workspace = true -client.workspace = true session.workspace = true -node_runtime.workspace = true +settings.workspace = true +zlog.workspace = true [[example]] name = "ui" diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index d52e2eea8a09f99d7d57943cce84f02251a7bd5c..b2443d6d30a0f4098423d4fadfaa052a96f89f36 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -1,9 +1,10 @@ //! # settings_ui mod components; -use editor::Editor; +use editor::{Editor, EditorEvent}; use feature_flags::{FeatureFlag, FeatureFlagAppExt as _}; +use fuzzy::StringMatchCandidate; use gpui::{ - App, AppContext as _, Context, Div, Entity, Global, IntoElement, ReadGlobal as _, Render, + App, AppContext as _, Context, Div, Entity, Global, IntoElement, ReadGlobal as _, Render, Task, TitlebarOptions, UniformListScrollHandle, Window, WindowHandle, WindowOptions, actions, div, point, px, size, uniform_list, }; @@ -15,7 +16,7 @@ use std::{ collections::HashMap, ops::Range, rc::Rc, - sync::Arc, + sync::{Arc, atomic::AtomicBool}, }; use ui::{Divider, DropdownMenu, ListItem, Switch, prelude::*}; use util::{paths::PathStyle, rel_path::RelPath}; @@ -320,10 +321,12 @@ pub struct SettingsWindow { files: Vec, current_file: SettingsUiFile, pages: Vec, - search: Entity, + search_bar: Entity, + search_task: Option>, navbar_entry: usize, // Index into pages - should probably be (usize, Option) for section + page navbar_entries: Vec, list_handle: UniformListScrollHandle, + search_matches: Vec>, } #[derive(PartialEq, Debug)] @@ -338,20 +341,23 @@ struct SettingsPage { items: Vec, } -impl SettingsPage { - fn section_headers(&self) -> impl Iterator { - self.items.iter().filter_map(|item| match item { - SettingsPageItem::SectionHeader(header) => Some(*header), - _ => None, - }) - } -} - +#[derive(PartialEq)] enum SettingsPageItem { SectionHeader(&'static str), SettingItem(SettingItem), } +impl std::fmt::Debug for SettingsPageItem { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SettingsPageItem::SectionHeader(header) => write!(f, "SectionHeader({})", header), + SettingsPageItem::SettingItem(setting_item) => { + write!(f, "SettingItem({})", setting_item.title) + } + } + } +} + impl SettingsPageItem { fn render(&self, file: SettingsUiFile, window: &mut Window, cx: &mut App) -> AnyElement { match self { @@ -423,6 +429,18 @@ struct SettingItem { metadata: Option>, } +impl PartialEq for SettingItem { + fn eq(&self, other: &Self) -> bool { + self.title == other.title + && self.description == other.description + && (match (&self.metadata, &other.metadata) { + (None, None) => true, + (Some(m1), Some(m2)) => m1.placeholder == m2.placeholder, + _ => false, + }) + } +} + #[allow(unused)] #[derive(Clone, PartialEq)] enum SettingsUiFile { @@ -472,11 +490,27 @@ impl SettingsUiFile { impl SettingsWindow { pub fn new(window: &mut Window, cx: &mut Context) -> Self { let current_file = SettingsUiFile::User; - let search = cx.new(|cx| { + let search_bar = cx.new(|cx| { let mut editor = Editor::single_line(window, cx); editor.set_placeholder_text("Search settings…", window, cx); editor }); + + cx.subscribe(&search_bar, |this, _, event: &EditorEvent, cx| { + let EditorEvent::Edited { transaction_id: _ } = event else { + return; + }; + + this.update_matches(cx); + }) + .detach(); + + cx.observe_global_in::(window, move |this, _, cx| { + this.fetch_files(cx); + cx.notify(); + }) + .detach(); + let mut this = Self { files: vec![], current_file: current_file, @@ -484,61 +518,175 @@ impl SettingsWindow { navbar_entries: vec![], navbar_entry: 0, list_handle: UniformListScrollHandle::default(), - search, + search_bar, + search_task: None, + search_matches: vec![], }; - cx.observe_global_in::(window, move |this, _, cx| { - this.fetch_files(cx); - cx.notify(); - }) - .detach(); - this.fetch_files(cx); + this.fetch_files(cx); this.build_ui(cx); + this } fn toggle_navbar_entry(&mut self, ix: usize) { - if self.navbar_entries[ix].is_root { - let expanded = &mut self.page_for_navbar_index(ix).expanded; - *expanded = !*expanded; - let current_page_index = self.page_index_from_navbar_index(self.navbar_entry); - // if currently selected page is a child of the parent page we are folding, - // set the current page to the parent page - if current_page_index == ix { - self.navbar_entry = ix; + // We can only toggle root entries + if !self.navbar_entries[ix].is_root { + return; + } + + 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; + *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 { + 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(); } + + self.build_navbar(); } fn build_navbar(&mut self) { - self.navbar_entries = self - .pages - .iter() - .flat_map(|page| { - std::iter::once(NavBarEntry { - title: page.title, - is_root: true, - }) - .chain( - page.expanded - .then(|| { - page.section_headers().map(|h| NavBarEntry { - title: h, - is_root: false, - }) - }) - .into_iter() - .flatten(), - ) + 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, + }); + 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, + }); + } + } + self.navbar_entries = navbar_entries; + } + + fn update_matches(&mut self, cx: &mut Context) { + self.search_task.take(); + let query = self.search_bar.read(cx).text(cx); + if query.is_empty() { + for page in &mut self.search_matches { + page.fill(true); + } + self.build_navbar(); + cx.notify(); + return; + } + + struct ItemKey { + page_index: usize, + header_index: usize, + item_index: usize, + } + let mut key_lut: Vec = vec![]; + let mut candidates = Vec::default(); + + for (page_index, page) in self.pages.iter().enumerate() { + let mut header_index = 0; + for (item_index, item) in page.items.iter().enumerate() { + let key_index = key_lut.len(); + match item { + SettingsPageItem::SettingItem(item) => { + candidates.push(StringMatchCandidate::new(key_index, item.title)); + candidates.push(StringMatchCandidate::new(key_index, item.description)); + } + SettingsPageItem::SectionHeader(header) => { + candidates.push(StringMatchCandidate::new(key_index, header)); + header_index = item_index; + } + } + key_lut.push(ItemKey { + page_index, + header_index, + item_index, + }); + } + } + let atomic_bool = AtomicBool::new(false); + + self.search_task = Some(cx.spawn(async move |this, cx| { + let string_matches = fuzzy::match_strings( + candidates.as_slice(), + &query, + false, + false, + candidates.len(), + &atomic_bool, + cx.background_executor().clone(), + ); + let string_matches = string_matches.await; + + this.update(cx, |this, cx| { + for page in &mut this.search_matches { + page.fill(false); + } + + for string_match in string_matches { + let ItemKey { + page_index, + header_index, + item_index, + } = key_lut[string_match.candidate_id]; + let page = &mut this.search_matches[page_index]; + page[header_index] = true; + page[item_index] = true; + } + this.build_navbar(); + this.navbar_entry = 0; + cx.notify() }) - .collect(); + .ok(); + })); } fn build_ui(&mut self, cx: &mut Context) { self.pages = self.current_file.pages(); + self.search_matches = self + .pages + .iter() + .map(|page| vec![true; page.items.len()]) + .collect::>(); self.build_navbar(); + if !self.search_bar.read(cx).is_empty(cx) { + self.update_matches(cx); + } + cx.notify(); } @@ -590,7 +738,7 @@ impl SettingsWindow { .border_1() .border_color(cx.theme().colors().border) .child(Icon::new(IconName::MagnifyingGlass).color(Color::Muted)) - .child(self.search.clone()) + .child(self.search_bar.clone()) } fn render_nav(&self, window: &mut Window, cx: &mut Context) -> Div { @@ -670,24 +818,38 @@ impl SettingsWindow { ) } - fn render_page( - &self, - page: &SettingsPage, - window: &mut Window, - cx: &mut Context, - ) -> Div { + fn page_items(&self) -> impl Iterator { + let page_idx = self.current_page_index(); + + self.current_page() + .items + .iter() + .enumerate() + .filter_map(move |(item_index, item)| { + self.search_matches[page_idx][item_index].then_some(item) + }) + } + + fn render_page(&self, window: &mut Window, cx: &mut Context) -> Div { v_flex().gap_4().children( - page.items - .iter() + self.page_items() .map(|item| item.render(self.current_file.clone(), window, cx)), ) } + fn current_page_index(&self) -> usize { + self.page_index_from_navbar_index(self.navbar_entry) + } + fn current_page(&self) -> &SettingsPage { - &self.pages[self.page_index_from_navbar_index(self.navbar_entry)] + &self.pages[self.current_page_index()] } fn page_index_from_navbar_index(&self, index: usize) -> usize { + if self.navbar_entries.is_empty() { + return 0; + } + self.navbar_entries .iter() .take(index + 1) @@ -723,7 +885,7 @@ impl Render for SettingsWindow { .gap_4() .bg(cx.theme().colors().editor_background) .child(self.render_files(window, cx)) - .child(self.render_page(self.current_page(), window, cx)), + .child(self.render_page(window, cx)), ) } } @@ -841,6 +1003,7 @@ where #[cfg(test)] mod test { + use super::*; impl SettingsWindow { @@ -851,6 +1014,56 @@ mod test { fn navbar_entry(&self) -> usize { self.navbar_entry } + + fn new_builder(window: &mut Window, cx: &mut Context) -> Self { + let mut this = Self::new(window, cx); + this.navbar_entries.clear(); + this.pages.clear(); + this + } + + fn build(mut self) -> Self { + self.build_navbar(); + self + } + + fn add_page( + mut self, + title: &'static str, + build_page: impl Fn(SettingsPage) -> SettingsPage, + ) -> Self { + let page = SettingsPage { + title, + expanded: false, + items: Vec::default(), + }; + + self.pages.push(build_page(page)); + self + } + + fn search(&mut self, search_query: &str, window: &mut Window, cx: &mut Context) { + self.search_task.take(); + self.search_bar.update(cx, |editor, cx| { + editor.set_text(search_query, window, cx); + }); + self.update_matches(cx); + } + + fn assert_search_results(&self, other: &Self) { + assert_eq!(self.navbar_entries, other.navbar_entries); + assert_eq!( + self.current_page().items.iter().collect::>(), + other.page_items().collect::>() + ); + } + } + + impl SettingsPage { + fn item(mut self, item: SettingsPageItem) -> Self { + self.items.push(item); + self + } } fn register_settings(cx: &mut App) { @@ -867,13 +1080,15 @@ mod test { let mut pages: Vec = Vec::new(); let mut current_page = None; let mut selected_idx = None; + let mut ix = 0; + let mut in_closed_subentry = false; - for (ix, mut line) in input + for mut line in input .lines() .map(|line| line.trim()) .filter(|line| !line.is_empty()) - .enumerate() { + let mut is_selected = false; if line.ends_with("*") { assert!( selected_idx.is_none(), @@ -881,6 +1096,7 @@ mod test { ); selected_idx = Some(ix); line = &line[..line.len() - 1]; + is_selected = true; } if line.starts_with("v") || line.starts_with(">") { @@ -889,6 +1105,8 @@ mod test { } let expanded = line.starts_with("v"); + in_closed_subentry = !expanded; + ix += 1; current_page = Some(SettingsPage { title: line.split_once(" ").unwrap().1, @@ -896,6 +1114,12 @@ mod test { items: Vec::default(), }); } 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"); + } + let Some(current_page) = current_page.as_mut() else { panic!("Sub entries must be within a page"); }; @@ -915,14 +1139,21 @@ mod test { 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, pages, - search: cx.new(|cx| Editor::single_line(window, cx)), - navbar_entry: selected_idx.unwrap(), + search_bar: cx.new(|cx| Editor::single_line(window, cx)), + navbar_entry: selected_idx.expect("Must have a selected navbar entry"), navbar_entries: Vec::default(), list_handle: UniformListScrollHandle::default(), + search_matches, + search_task: None, }; settings_window.build_navbar(); @@ -963,7 +1194,7 @@ mod test { } check_navbar_toggle!( - basic_open, + navbar_basic_open, before: r" v General - General @@ -980,7 +1211,7 @@ mod test { ); check_navbar_toggle!( - basic_close, + navbar_basic_close, before: r" > General* - General @@ -999,7 +1230,7 @@ mod test { ); check_navbar_toggle!( - basic_second_root_entry_close, + navbar_basic_second_root_entry_close, before: r" > General - General @@ -1013,4 +1244,166 @@ mod test { > Project* " ); + + check_navbar_toggle!( + navbar_toggle_subroot, + before: r" + v General Page + - General + - Privacy + v Project + - Worktree Settings Content* + v AI + - General + > Appearance & Behavior + ", + toggle_idx: 3, + after: r" + v General Page + - General + - Privacy + > Project* + v AI + - General + > Appearance & Behavior + " + ); + + check_navbar_toggle!( + navbar_toggle_close_propagates_selected_index, + before: r" + v General Page + - General + - Privacy + v Project + - Worktree Settings Content + v AI + - General* + > Appearance & Behavior + ", + toggle_idx: 0, + after: r" + > General Page + v Project + - Worktree Settings Content + v AI + - General* + > Appearance & Behavior + " + ); + + check_navbar_toggle!( + navbar_toggle_expand_propagates_selected_index, + before: r" + > General Page + - General + - Privacy + v Project + - Worktree Settings Content + v AI + - General* + > Appearance & Behavior + ", + toggle_idx: 0, + after: r" + v General Page + - General + - Privacy + v Project + - Worktree Settings Content + v AI + - General* + > Appearance & Behavior + " + ); + + 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(); + let (actual, expected) = cx.update(|window, cx| { + register_settings(cx); + + let expected = cx.new(|cx| { + SettingsWindow::new_builder(window, cx) + .add_page("General", |page| { + page.item(SettingsPageItem::SectionHeader("General settings")) + .item(SettingsPageItem::SettingItem(SettingItem { + title: "test title", + description: "General test", + field: Box::new(SettingField { + pick: |settings_content| { + &settings_content.workspace.confirm_quit + }, + pick_mut: |settings_content| { + &mut settings_content.workspace.confirm_quit + }, + }), + metadata: None, + })) + }) + .build() + }); + + let actual = cx.new(|cx| { + SettingsWindow::new_builder(window, cx) + .add_page("General", |page| { + page.item(SettingsPageItem::SectionHeader("General settings")) + .item(SettingsPageItem::SettingItem(SettingItem { + title: "test title", + description: "General test", + field: Box::new(SettingField { + pick: |settings_content| { + &settings_content.workspace.confirm_quit + }, + pick_mut: |settings_content| { + &mut settings_content.workspace.confirm_quit + }, + }), + metadata: None, + })) + }) + .add_page("Theme", |page| { + page.item(SettingsPageItem::SectionHeader("Theme settings")) + }) + .build() + }); + + actual.update(cx, |settings, cx| settings.search("gen", window, cx)); + + (actual, expected) + }); + + cx.cx.run_until_parked(); + + cx.update(|_window, cx| { + let expected = expected.read(cx); + let actual = actual.read(cx); + expected.assert_search_results(&actual); + }) + } }