From 94a4c0c352337567ffa74620789e76c55325e48b Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 30 Sep 2025 17:25:49 -0400 Subject: [PATCH] settings ui: Fix bug with navbar index to page index translation (#39245) This happened when search results completely filtered out a page above the selected page index. The old index was calculated based on the nav bar entry's position and the count of root entries above it, this was wrong because root entries could be filtered out with a search. Now the page index is saved when building the navbar Release Notes: - N/A --- crates/settings_ui/src/settings_ui.rs | 127 +++++++++++++++++++------- 1 file changed, 93 insertions(+), 34 deletions(-) diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index b2443d6d30a0f4098423d4fadfaa052a96f89f36..f0f57e65c5f1ff9b5b1832c380c7e900f3233bff 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -333,6 +333,7 @@ pub struct SettingsWindow { struct NavBarEntry { title: &'static str, is_root: bool, + page_index: usize, } struct SettingsPage { @@ -574,6 +575,7 @@ impl SettingsWindow { navbar_entries.push(NavBarEntry { title: page.title, is_root: true, + page_index, }); if !page.expanded { continue; @@ -590,6 +592,7 @@ impl SettingsWindow { navbar_entries.push(NavBarEntry { title, is_root: false, + page_index, }); } } @@ -668,7 +671,7 @@ impl SettingsWindow { } this.build_navbar(); this.navbar_entry = 0; - cx.notify() + cx.notify(); }) .ok(); })); @@ -850,12 +853,7 @@ impl SettingsWindow { return 0; } - self.navbar_entries - .iter() - .take(index + 1) - .map(|entry| entry.is_root as usize) - .sum::() - - 1 + self.navbar_entries[index].page_index } fn page_for_navbar_index(&mut self, index: usize) -> &mut SettingsPage { @@ -1051,7 +1049,15 @@ mod test { } fn assert_search_results(&self, other: &Self) { - assert_eq!(self.navbar_entries, other.navbar_entries); + // 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 + }) + ); assert_eq!( self.current_page().items.iter().collect::>(), other.page_items().collect::>() @@ -1066,6 +1072,20 @@ mod test { } } + impl SettingsPageItem { + fn basic_item(title: &'static str, description: &'static str) -> Self { + SettingsPageItem::SettingItem(SettingItem { + title, + description, + field: Box::new(SettingField { + pick: |settings_content| &settings_content.auto_update, + pick_mut: |settings_content| &mut settings_content.auto_update, + }), + metadata: None, + }) + } + } + fn register_settings(cx: &mut App) { settings::init(cx); theme::init(theme::LoadThemes::JustBase, cx); @@ -1352,19 +1372,7 @@ mod test { 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, - })) + .item(SettingsPageItem::basic_item("test title", "General test")) }) .build() }); @@ -1373,19 +1381,7 @@ mod test { 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, - })) + .item(SettingsPageItem::basic_item("test title", "General test")) }) .add_page("Theme", |page| { page.item(SettingsPageItem::SectionHeader("Theme settings")) @@ -1406,4 +1402,67 @@ mod test { expected.assert_search_results(&actual); }) } + + #[gpui::test] + fn test_search_render_page_with_filtered_out_navbar_entries(cx: &mut gpui::TestAppContext) { + let cx = cx.add_empty_window(); + let (actual, expected) = cx.update(|window, cx| { + register_settings(cx); + + let actual = cx.new(|cx| { + SettingsWindow::new_builder(window, cx) + .add_page("General", |page| { + page.item(SettingsPageItem::SectionHeader("General settings")) + .item(SettingsPageItem::basic_item( + "Confirm Quit", + "Whether to confirm before quitting Zed", + )) + .item(SettingsPageItem::basic_item( + "Auto Update", + "Automatically update Zed", + )) + }) + .add_page("AI", |page| { + page.item(SettingsPageItem::basic_item( + "Disable AI", + "Whether to disable all AI features in Zed", + )) + }) + .add_page("Appearance & Behavior", |page| { + page.item(SettingsPageItem::SectionHeader("Cursor")).item( + SettingsPageItem::basic_item( + "Cursor Shape", + "Cursor shape for the editor", + ), + ) + }) + .build() + }); + + let expected = cx.new(|cx| { + SettingsWindow::new_builder(window, cx) + .add_page("Appearance & Behavior", |page| { + page.item(SettingsPageItem::SectionHeader("Cursor")).item( + SettingsPageItem::basic_item( + "Cursor Shape", + "Cursor shape for the editor", + ), + ) + }) + .build() + }); + + actual.update(cx, |settings, cx| settings.search("cursor", 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); + }) + } }