From 1e583611fa972ef192b19f5ab0a14bf40e2d70e3 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Tue, 20 Jan 2026 01:27:42 +0100 Subject: [PATCH] settings_ui: Prevent panic when trying to configure edit prediction providers for language (#47162) Closes #46502 The issue here was that we were not looking into the sub page stack when looking headers up, resulting in an out of bounds index. This PR fixes this. Due to me also fixing another small bug in the UI (and adding proper support for the breadcrumbs), I had to move quite some stuff around here to get this to work. Namely, I made the `sub_page_stack` a field on the `SettingsWindow` and now only store the `active_language` in a global to ensure that we store scroll positions properly for all sub pages. For that to work with the edit prediction provider page, I had to remove the struct there and could just move that into a method, which was a nice side effect there I suppose. Release Notes: - Fixed a crash that could occur when trying to edit edit prediction providers in the settings UI. --- Cargo.lock | 1 + crates/settings_ui/Cargo.toml | 1 + crates/settings_ui/src/page_data.rs | 66 ++-- crates/settings_ui/src/pages.rs | 2 +- .../pages/edit_prediction_provider_setup.rs | 161 ++++----- crates/settings_ui/src/settings_ui.rs | 335 ++++++++++-------- 6 files changed, 288 insertions(+), 278 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5d88e5df82961ceedef8238b739cfe48a6cf30fc..5624e0cb269bb9f5522bc6f7d6ec6f527ae7d6eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14930,6 +14930,7 @@ dependencies = [ "fuzzy", "gpui", "heck 0.5.0", + "itertools 0.14.0", "language", "language_models", "log", diff --git a/crates/settings_ui/Cargo.toml b/crates/settings_ui/Cargo.toml index 256ec2de557e903405d1c3431ef44e98d757d3c6..9abf125509be5d0867715e7a15e5c98d42b6503f 100644 --- a/crates/settings_ui/Cargo.toml +++ b/crates/settings_ui/Cargo.toml @@ -27,6 +27,7 @@ fs.workspace = true fuzzy.workspace = true gpui.workspace = true heck.workspace = true +itertools.workspace = true log.workspace = true menu.workspace = true paths.workspace = true diff --git a/crates/settings_ui/src/page_data.rs b/crates/settings_ui/src/page_data.rs index 372ed1fae34412f1453e7cc6c4056b24a6c1603f..8b12b0da39ff212162c13c3a0701fa8ccce58ac8 100644 --- a/crates/settings_ui/src/page_data.rs +++ b/crates/settings_ui/src/page_data.rs @@ -1,12 +1,13 @@ -use gpui::{Action as _, App}; +use gpui::{Action as _, App, SharedString}; use settings::{LanguageSettingsContent, SettingsContent}; use std::sync::Arc; use strum::IntoDiscriminant as _; -use ui::{IntoElement, SharedString}; +use ui::IntoElement; use crate::{ ActionLink, DynamicItem, PROJECT, SettingField, SettingItem, SettingsFieldMetadata, - SettingsPage, SettingsPageItem, SubPageLink, USER, all_language_names, sub_page_stack, + SettingsPage, SettingsPageItem, SubPageLink, USER, active_language, all_language_names, + pages::render_edit_prediction_setup_page, }; const DEFAULT_STRING: String = String::new(); @@ -2696,24 +2697,30 @@ fn languages_and_tools_page(cx: &App) -> SettingsPage { fn languages_list_section(cx: &App) -> Box<[SettingsPageItem]> { // todo(settings_ui): Refresh on extension (un)/installed // Note that `crates/json_schema_store` solves the same problem, there is probably a way to unify the two - std::iter::once(SettingsPageItem::SectionHeader(LANGUAGES_SECTION_HEADER)) + std::iter::once(SettingsPageItem::SectionHeader("Languages")) .chain(all_language_names(cx).into_iter().map(|language_name| { let link = format!("languages.{language_name}"); SettingsPageItem::SubPageLink(SubPageLink { title: language_name, + r#type: crate::SubPageType::Language, description: None, json_path: Some(link.leak()), in_json: true, files: USER | PROJECT, - render: Arc::new(|this, window, cx| { + render: |this, scroll_handle, window, cx| { let items: Box<[SettingsPageItem]> = concat_sections!( language_settings_data(), non_editor_language_settings_data(), edit_prediction_language_settings_section() ); - this.render_sub_page_items(items.iter().enumerate(), None, window, cx) - .into_any_element() - }), + this.render_sub_page_items( + items.iter().enumerate(), + scroll_handle, + window, + cx, + ) + .into_any_element() + }, }) })) .collect() @@ -6911,29 +6918,21 @@ fn network_page() -> SettingsPage { } } -const LANGUAGES_SECTION_HEADER: &'static str = "Languages"; - -fn current_language() -> Option { - sub_page_stack().iter().find_map(|page| { - (page.section_header == LANGUAGES_SECTION_HEADER).then(|| page.link.title.clone()) - }) -} - fn language_settings_field( settings_content: &SettingsContent, - get: fn(&LanguageSettingsContent) -> Option<&T>, + get_language_setting_field: fn(&LanguageSettingsContent) -> Option<&T>, ) -> Option<&T> { let all_languages = &settings_content.project.all_languages; - if let Some(current_language_name) = current_language() { - if let Some(current_language) = all_languages.languages.0.get(¤t_language_name) { - let value = get(current_language); - if value.is_some() { - return value; - } - } - } - let default_value = get(&all_languages.defaults); - return default_value; + + active_language() + .and_then(|current_language_name| { + all_languages + .languages + .0 + .get(current_language_name.as_ref()) + }) + .and_then(get_language_setting_field) + .or_else(|| get_language_setting_field(&all_languages.defaults)) } fn language_settings_field_mut( @@ -6942,7 +6941,7 @@ fn language_settings_field_mut( write: fn(&mut LanguageSettingsContent, Option), ) { let all_languages = &mut settings_content.project.all_languages; - let language_content = if let Some(current_language) = current_language() { + let language_content = if let Some(current_language) = active_language() { all_languages .languages .0 @@ -8121,7 +8120,7 @@ fn language_settings_data() -> Box<[SettingsPageItem]> { ] } - let is_global = current_language().is_none(); + let is_global = active_language().is_none(); let lsp_document_colors_item = [SettingsPageItem::SettingItem(SettingItem { title: "LSP Document Colors", @@ -8463,17 +8462,12 @@ fn edit_prediction_language_settings_section() -> [SettingsPageItem; 4] { SettingsPageItem::SectionHeader("Edit Predictions"), SettingsPageItem::SubPageLink(SubPageLink { title: "Configure Providers".into(), + r#type: Default::default(), json_path: Some("edit_predictions.providers"), description: Some("Set up different edit prediction providers in complement to Zed's built-in Zeta model.".into()), in_json: false, files: USER, - render: Arc::new(|_, window, cx| { - let settings_window = cx.entity(); - let page = window.use_state(cx, |_, _| { - crate::pages::EditPredictionSetupPage::new(settings_window) - }); - page.into_any_element() - }), + render: render_edit_prediction_setup_page }), SettingsPageItem::SettingItem(SettingItem { title: "Show Edit Predictions", diff --git a/crates/settings_ui/src/pages.rs b/crates/settings_ui/src/pages.rs index 2b2c4818c1322216707f38bf93cefffeb14add03..cda63c6f0db5297333d2c371ffe75905d89a59fb 100644 --- a/crates/settings_ui/src/pages.rs +++ b/crates/settings_ui/src/pages.rs @@ -1,2 +1,2 @@ mod edit_prediction_provider_setup; -pub use edit_prediction_provider_setup::EditPredictionSetupPage; +pub(crate) use edit_prediction_provider_setup::render_edit_prediction_setup_page; diff --git a/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs b/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs index fb8f967613fa195080f62c5ab2ce76a43f3d1e22..48db109aa82dff86fba2aefa22c3259b0030713e 100644 --- a/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs +++ b/crates/settings_ui/src/pages/edit_prediction_provider_setup.rs @@ -1,104 +1,89 @@ use edit_prediction::{ - ApiKeyState, Zeta2FeatureFlag, + ApiKeyState, MercuryFeatureFlag, SweepFeatureFlag, mercury::{MERCURY_CREDENTIALS_URL, mercury_api_token}, sweep_ai::{SWEEP_CREDENTIALS_URL, sweep_api_token}, }; use feature_flags::FeatureFlagAppExt as _; use gpui::{Entity, ScrollHandle, prelude::*}; use language_models::provider::mistral::{CODESTRAL_API_URL, codestral_api_key}; -use ui::{ButtonLink, ConfiguredApiCard, WithScrollbar, prelude::*}; +use ui::{ButtonLink, ConfiguredApiCard, prelude::*}; use crate::{ SettingField, SettingItem, SettingsFieldMetadata, SettingsPageItem, SettingsWindow, USER, components::{SettingsInputField, SettingsSectionHeader}, }; -pub struct EditPredictionSetupPage { - settings_window: Entity, - scroll_handle: ScrollHandle, -} - -impl EditPredictionSetupPage { - pub fn new(settings_window: Entity) -> Self { - Self { - settings_window, - scroll_handle: ScrollHandle::new(), - } - } -} - -impl Render for EditPredictionSetupPage { - fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { - let settings_window = self.settings_window.clone(); - - let providers = [ - Some(render_github_copilot_provider(window, cx).into_any_element()), - cx.has_flag::().then(|| { - render_api_key_provider( - IconName::Inception, - "Mercury", - "https://platform.inceptionlabs.ai/dashboard/api-keys".into(), - mercury_api_token(cx), - |_cx| MERCURY_CREDENTIALS_URL, - None, - window, - cx, - ) - .into_any_element() - }), - cx.has_flag::().then(|| { - render_api_key_provider( - IconName::SweepAi, - "Sweep", - "https://app.sweep.dev/".into(), - sweep_api_token(cx), - |_cx| SWEEP_CREDENTIALS_URL, - None, - window, - cx, - ) - .into_any_element() - }), - Some( - render_api_key_provider( - IconName::AiMistral, - "Codestral", - "https://console.mistral.ai/codestral".into(), - codestral_api_key(cx), - |cx| language_models::MistralLanguageModelProvider::api_url(cx), - Some(settings_window.update(cx, |settings_window, cx| { - let codestral_settings = codestral_settings(); - settings_window - .render_sub_page_items_section( - codestral_settings.iter().enumerate(), - None, - window, - cx, - ) - .into_any_element() - })), - window, - cx, - ) - .into_any_element(), - ), - ]; - - div() - .size_full() - .vertical_scrollbar_for(&self.scroll_handle, window, cx) - .child( - v_flex() - .id("ep-setup-page") - .min_w_0() - .size_full() - .px_8() - .pb_16() - .overflow_y_scroll() - .track_scroll(&self.scroll_handle) - .children(providers.into_iter().flatten()), +pub(crate) fn render_edit_prediction_setup_page( + settings_window: &SettingsWindow, + scroll_handle: &ScrollHandle, + window: &mut Window, + cx: &mut Context, +) -> AnyElement { + let providers = [ + Some(render_github_copilot_provider(window, cx).into_any_element()), + cx.has_flag::().then(|| { + render_api_key_provider( + IconName::Inception, + "Mercury", + "https://platform.inceptionlabs.ai/dashboard/api-keys".into(), + mercury_api_token(cx), + |_cx| MERCURY_CREDENTIALS_URL, + None, + window, + cx, + ) + .into_any_element() + }), + cx.has_flag::().then(|| { + render_api_key_provider( + IconName::SweepAi, + "Sweep", + "https://app.sweep.dev/".into(), + sweep_api_token(cx), + |_cx| SWEEP_CREDENTIALS_URL, + None, + window, + cx, + ) + .into_any_element() + }), + Some( + render_api_key_provider( + IconName::AiMistral, + "Codestral", + "https://console.mistral.ai/codestral".into(), + codestral_api_key(cx), + |cx| language_models::MistralLanguageModelProvider::api_url(cx), + Some( + settings_window + .render_sub_page_items_section( + codestral_settings().iter().enumerate(), + window, + cx, + ) + .into_any_element(), + ), + window, + cx, ) - } + .into_any_element(), + ), + ]; + + div() + .size_full() + .child( + v_flex() + .id("ep-setup-page") + .min_w_0() + .size_full() + .px_8() + .pb_16() + .overflow_y_scroll() + .track_scroll(&scroll_handle) + .children(providers.into_iter().flatten()), + ) + .into_any_element() } fn render_api_key_provider( @@ -109,7 +94,7 @@ fn render_api_key_provider( current_url: fn(&mut App) -> SharedString, additional_fields: Option, window: &mut Window, - cx: &mut Context, + cx: &mut Context, ) -> impl IntoElement { let weak_page = cx.weak_entity(); _ = window.use_keyed_state(title, cx, |_, cx| { diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index 50bb6534cc15e926a5592acfb89000507e1f46d5..95aeaeb9fbf3a0c0a03838ada0e4b4e8598571a1 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -30,7 +30,8 @@ use theme::ThemeSettings; use title_bar::platform_title_bar::PlatformTitleBar; use ui::{ Banner, ContextMenu, Divider, DropdownMenu, DropdownStyle, IconButtonShape, KeyBinding, - KeybindingHint, PopoverMenu, Switch, Tooltip, TreeViewItem, WithScrollbar, prelude::*, + KeybindingHint, PopoverMenu, Scrollbars, Switch, Tooltip, TreeViewItem, WithScrollbar, + prelude::*, }; use ui_input::{NumberField, NumberFieldMode, NumberFieldType}; use util::{ResultExt as _, paths::PathStyle, rel_path::RelPath}; @@ -653,18 +654,18 @@ pub fn open_settings_editor( /// /// Global so that `pick` and `write` callbacks can access it /// and use it to dynamically render sub pages (e.g. for language settings) -static SUB_PAGE_STACK: LazyLock>> = LazyLock::new(|| RwLock::new(Vec::new())); +static ACTIVE_LANGUAGE: LazyLock>> = + LazyLock::new(|| RwLock::new(Option::None)); -fn sub_page_stack() -> std::sync::RwLockReadGuard<'static, Vec> { - SUB_PAGE_STACK +fn active_language() -> Option { + ACTIVE_LANGUAGE .read() - .expect("SUB_PAGE_STACK is never poisoned") + .ok() + .and_then(|language| language.clone()) } -fn sub_page_stack_mut() -> std::sync::RwLockWriteGuard<'static, Vec> { - SUB_PAGE_STACK - .write() - .expect("SUB_PAGE_STACK is never poisoned") +fn active_language_mut() -> Option>> { + ACTIVE_LANGUAGE.write().ok() } pub struct SettingsWindow { @@ -674,6 +675,7 @@ pub struct SettingsWindow { worktree_root_dirs: HashMap, current_file: SettingsUiFile, pages: Vec, + sub_page_stack: Vec, search_bar: Entity, search_task: Option>, /// Index into navbar_entries @@ -687,7 +689,6 @@ pub struct SettingsWindow { filter_table: Vec>, has_query: bool, content_handles: Vec>>, - sub_page_scroll_handle: ScrollHandle, focus_handle: FocusHandle, navbar_focus_handle: Entity, content_focus_handle: Entity, @@ -712,7 +713,37 @@ struct SearchKeyLUTEntry { struct SubPage { link: SubPageLink, - section_header: &'static str, + section_header: SharedString, + scroll_handle: ScrollHandle, +} + +impl SubPage { + fn new(link: SubPageLink, section_header: SharedString) -> Self { + if link.r#type == SubPageType::Language + && let Some(mut active_language_global) = active_language_mut() + { + active_language_global.replace(link.title.clone()); + } + + SubPage { + link, + section_header, + scroll_handle: ScrollHandle::new(), + } + } +} + +impl Drop for SubPage { + fn drop(&mut self) { + if self.link.r#type == SubPageType::Language + && let Some(mut active_language_global) = active_language_mut() + && active_language_global + .as_ref() + .is_some_and(|language_name| language_name == &self.link.title) + { + active_language_global.take(); + } + } } #[derive(Debug)] @@ -760,6 +791,13 @@ impl std::fmt::Debug for SettingsPageItem { } impl SettingsPageItem { + fn header_text(&self) -> Option<&'static str> { + match self { + SettingsPageItem::SectionHeader(header) => Some(header), + _ => None, + } + } + fn render( &self, settings_window: &SettingsWindow, @@ -896,19 +934,22 @@ impl SettingsPageItem { .on_click({ let sub_page_link = sub_page_link.clone(); cx.listener(move |this, _, window, cx| { - let mut section_index = item_index; - let current_page = this.current_page(); - - while !matches!( - current_page.items[section_index], - SettingsPageItem::SectionHeader(_) - ) { - section_index -= 1; - } - - let SettingsPageItem::SectionHeader(header) = - current_page.items[section_index] - else { + let header_text = this + .sub_page_stack + .last() + .map(|sub_page| sub_page.link.title.clone()) + .or_else(|| { + this.current_page() + .items + .iter() + .take(item_index) + .rev() + .find_map(|item| { + item.header_text().map(SharedString::new_static) + }) + }); + + let Some(header) = header_text else { unreachable!( "All items always have a section header above them" ) @@ -941,7 +982,7 @@ impl SettingsPageItem { render_setting_item_inner(discriminant_setting_item, true, false, cx); let has_sub_fields = - rendered_ok && discriminant.map(|d| !fields[d].is_empty()).unwrap_or(false); + rendered_ok && discriminant.is_some_and(|d| !fields[d].is_empty()); let mut content = v_flex() .id("dynamic-item") @@ -1104,7 +1145,7 @@ fn render_settings_item( ), ) .child(control) - .when(sub_page_stack().is_empty(), |this| { + .when(settings_window.sub_page_stack.is_empty(), |this| { this.child(render_settings_item_link( setting_item.description, setting_item.field.json_path(), @@ -1241,9 +1282,17 @@ impl PartialEq for SettingItem { } } +#[derive(Clone, PartialEq, Default)] +enum SubPageType { + Language, + #[default] + Other, +} + #[derive(Clone)] struct SubPageLink { title: SharedString, + r#type: SubPageType, description: Option, /// See [`SettingField.json_path`] json_path: Option<&'static str>, @@ -1251,12 +1300,8 @@ struct SubPageLink { /// Removes the "Edit in settings.json" button from the page. in_json: bool, files: FileMask, - render: Arc< - dyn Fn(&mut SettingsWindow, &mut Window, &mut Context) -> AnyElement - + 'static - + Send - + Sync, - >, + render: + fn(&SettingsWindow, &ScrollHandle, &mut Window, &mut Context) -> AnyElement, } impl PartialEq for SubPageLink { @@ -1518,6 +1563,7 @@ impl SettingsWindow { current_file: current_file, pages: vec![], + sub_page_stack: vec![], navbar_entries: vec![], navbar_entry: 0, navbar_scroll_handle: UniformListScrollHandle::default(), @@ -1526,7 +1572,6 @@ impl SettingsWindow { filter_table: vec![], has_query: false, content_handles: vec![], - sub_page_scroll_handle: ScrollHandle::new(), focus_handle: cx.focus_handle(), navbar_focus_handle: NonFocusableHandle::new( NAVBAR_CONTAINER_TAB_INDEX, @@ -2015,7 +2060,7 @@ impl SettingsWindow { self.setup_navbar_focus_subscriptions(window, cx); self.build_content_handles(window, cx); } - sub_page_stack_mut().clear(); + self.sub_page_stack.clear(); // PERF: doesn't have to be rebuilt, can just be filled with true. pages is constant once it is built self.build_filter_table(); self.reset_list_state(); @@ -2131,7 +2176,7 @@ impl SettingsWindow { self.reset_list_state(); } - sub_page_stack_mut().clear(); + self.sub_page_stack.clear(); } fn open_first_nav_page(&mut self) { @@ -2626,8 +2671,10 @@ impl SettingsWindow { if self.navbar_entries[navbar_entry_index].is_root || !self.is_nav_entry_visible(navbar_entry_index) { - self.sub_page_scroll_handle - .set_offset(point(px(0.), px(0.))); + if let Some(scroll_handle) = self.current_sub_page_scroll_handle() { + scroll_handle.set_offset(point(px(0.), px(0.))); + } + if focus_content { let Some(first_item_index) = self.visible_page_items().next().map(|(index, _)| index) @@ -2698,8 +2745,10 @@ impl SettingsWindow { .position(|(index, _)| index == content_item_index) .unwrap_or(0); if index == 0 { - self.sub_page_scroll_handle - .set_offset(point(px(0.), px(0.))); + if let Some(scroll_handle) = self.current_sub_page_scroll_handle() { + scroll_handle.set_offset(point(px(0.), px(0.))); + } + self.list_state.scroll_to(gpui::ListOffset { item_ix: 0, offset_in_item: px(0.), @@ -2747,6 +2796,10 @@ impl SettingsWindow { cx.notify(); } + fn current_sub_page_scroll_handle(&self) -> Option<&ScrollHandle> { + self.sub_page_stack.last().map(|page| &page.scroll_handle) + } + fn visible_page_items(&self) -> impl Iterator { let page_idx = self.current_page_index(); @@ -2754,33 +2807,32 @@ impl SettingsWindow { .items .iter() .enumerate() - .filter_map(move |(item_index, item)| { - self.filter_table[page_idx][item_index].then_some((item_index, item)) - }) + .filter(move |&(item_index, _)| self.filter_table[page_idx][item_index]) } fn render_sub_page_breadcrumbs(&self) -> impl IntoElement { - let mut items = vec![]; - items.push(self.current_page().title.into()); - items.extend( - sub_page_stack() - .iter() - .flat_map(|page| [page.section_header.into(), page.link.title.clone()]), - ); - - let last = items.pop().unwrap(); - h_flex() - .gap_1() - .children( - items - .into_iter() - .flat_map(|item| [item, "/".into()]) - .map(|item| Label::new(item).color(Color::Muted)), + h_flex().gap_1().children( + itertools::intersperse( + std::iter::once(self.current_page().title.into()).chain( + self.sub_page_stack + .iter() + .enumerate() + .flat_map(|(index, page)| { + (index == 0) + .then(|| page.section_header.clone()) + .into_iter() + .chain(std::iter::once(page.link.title.clone())) + }), + ), + "/".into(), ) - .child(Label::new(last)) + .map(|item| Label::new(item).color(Color::Muted)), + ) } - fn render_empty_state(&self, search_query: SharedString) -> impl IntoElement { + fn render_no_results(&self, cx: &App) -> impl IntoElement { + let search_query = self.search_bar.read(cx).text(cx); + v_flex() .size_full() .items_center() @@ -2788,28 +2840,25 @@ impl SettingsWindow { .gap_1() .child(Label::new("No Results")) .child( - Label::new(search_query) + Label::new(format!("No settings match \"{}\"", search_query)) .size(LabelSize::Small) .color(Color::Muted), ) } - fn render_page_items( + fn render_current_page_items( &mut self, - page_index: usize, _window: &mut Window, cx: &mut Context, ) -> impl IntoElement { + let current_page_index = self.current_page_index(); let mut page_content = v_flex().id("settings-ui-page").size_full(); let has_active_search = !self.search_bar.read(cx).is_empty(cx); let has_no_results = self.visible_page_items().next().is_none() && has_active_search; if has_no_results { - let search_query = self.search_bar.read(cx).text(cx); - page_content = page_content.child( - self.render_empty_state(format!("No settings match \"{}\"", search_query).into()), - ) + page_content = page_content.child(self.render_no_results(cx)) } else { let last_non_header_index = self .visible_page_items() @@ -2830,7 +2879,7 @@ impl SettingsWindow { if index == 0 { return div() .px_8() - .when(sub_page_stack().is_empty(), |this| { + .when(this.sub_page_stack.is_empty(), |this| { this.when_some(root_nav_label, |this, title| { this.child( Label::new(title).size(LabelSize::Large).mt_2().mb_3(), @@ -2852,8 +2901,9 @@ impl SettingsWindow { let is_last = Some(actual_item_index) == last_non_header_index; - let item_focus_handle = - this.content_handles[page_index][actual_item_index].focus_handle(cx); + let item_focus_handle = this.content_handles[current_page_index] + [actual_item_index] + .focus_handle(cx); v_flex() .id(("settings-page-item", actual_item_index)) @@ -2879,7 +2929,7 @@ impl SettingsWindow { fn render_sub_page_items<'a, Items>( &self, items: Items, - page_index: Option, + scroll_handle: &ScrollHandle, window: &mut Window, cx: &mut Context, ) -> impl IntoElement @@ -2890,14 +2940,13 @@ impl SettingsWindow { .id("settings-ui-page") .size_full() .overflow_y_scroll() - .track_scroll(&self.sub_page_scroll_handle); - self.render_sub_page_items_in(page_content, items, page_index, window, cx) + .track_scroll(scroll_handle); + self.render_sub_page_items_in(page_content, items, window, cx) } fn render_sub_page_items_section<'a, Items>( &self, items: Items, - page_index: Option, window: &mut Window, cx: &mut Context, ) -> impl IntoElement @@ -2905,14 +2954,13 @@ impl SettingsWindow { Items: Iterator, { let page_content = v_flex().id("settings-ui-sub-page-section").size_full(); - self.render_sub_page_items_in(page_content, items, page_index, window, cx) + self.render_sub_page_items_in(page_content, items, window, cx) } fn render_sub_page_items_in<'a, Items>( &self, - mut page_content: Stateful
, + page_content: Stateful
, items: Items, - page_index: Option, window: &mut Window, cx: &mut Context, ) -> impl IntoElement @@ -2921,16 +2969,12 @@ impl SettingsWindow { { let items: Vec<_> = items.collect(); let items_len = items.len(); - let mut section_header = None; let has_active_search = !self.search_bar.read(cx).is_empty(cx); let has_no_results = items_len == 0 && has_active_search; if has_no_results { - let search_query = self.search_bar.read(cx).text(cx); - page_content = page_content.child( - self.render_empty_state(format!("No settings match \"{}\"", search_query).into()), - ) + page_content.child(self.render_no_results(cx)) } else { let last_non_header_index = items .iter() @@ -2945,35 +2989,25 @@ impl SettingsWindow { .find(|entry| entry.is_root && entry.page_index == self.current_page_index()) .map(|entry| entry.title); - page_content = page_content - .when(sub_page_stack().is_empty(), |this| { + page_content + .when(self.sub_page_stack.is_empty(), |this| { this.when_some(root_nav_label, |this, title| { this.child(Label::new(title).size(LabelSize::Large).mt_2().mb_3()) }) }) .children(items.clone().into_iter().enumerate().map( |(index, (actual_item_index, item))| { - let no_bottom_border = items - .get(index + 1) - .map(|(_, next_item)| { + let no_bottom_border = + items.get(index + 1).is_some_and(|(_, next_item)| { matches!(next_item, SettingsPageItem::SectionHeader(_)) - }) - .unwrap_or(false); + }); + let is_last = Some(index) == last_non_header_index; - if let SettingsPageItem::SectionHeader(header) = item { - section_header = Some(*header); - } v_flex() .w_full() .min_w_0() .id(("settings-page-item", actual_item_index)) - .when_some(page_index, |element, page_index| { - element.track_focus( - &self.content_handles[page_index][actual_item_index] - .focus_handle(cx), - ) - }) .child(item.render( self, actual_item_index, @@ -2984,7 +3018,6 @@ impl SettingsWindow { }, )) } - page_content } fn render_page( @@ -2995,13 +3028,7 @@ impl SettingsWindow { let page_header; let page_content; - if sub_page_stack().is_empty() { - page_header = self.render_files_header(window, cx).into_any_element(); - - page_content = self - .render_page_items(self.current_page_index(), window, cx) - .into_any_element(); - } else { + if let Some(current_sub_page) = self.sub_page_stack.last() { page_header = h_flex() .w_full() .justify_between() @@ -3019,32 +3046,36 @@ impl SettingsWindow { ) .child(self.render_sub_page_breadcrumbs()), ) - .when( - sub_page_stack() - .last() - .is_none_or(|sub_page| sub_page.link.in_json), - |this| { - this.child( - Button::new("open-in-settings-file", "Edit in settings.json") - .tab_index(0_isize) - .style(ButtonStyle::OutlinedGhost) - .tooltip(Tooltip::for_action_title_in( - "Edit in settings.json", - &OpenCurrentFile, - &self.focus_handle, - )) - .on_click(cx.listener(|this, _, window, cx| { - this.open_current_settings_file(window, cx); - })), - ) - }, - ) + .when(current_sub_page.link.in_json, |this| { + this.child( + Button::new("open-in-settings-file", "Edit in settings.json") + .tab_index(0_isize) + .style(ButtonStyle::OutlinedGhost) + .tooltip(Tooltip::for_action_title_in( + "Edit in settings.json", + &OpenCurrentFile, + &self.focus_handle, + )) + .on_click(cx.listener(|this, _, window, cx| { + this.open_current_settings_file(window, cx); + })), + ) + }) .into_any_element(); - let active_page_render_fn = sub_page_stack().last().unwrap().link.render.clone(); - page_content = (active_page_render_fn)(self, window, cx); + let active_page_render_fn = ¤t_sub_page.link.render; + page_content = + (active_page_render_fn)(self, ¤t_sub_page.scroll_handle, window, cx); + } else { + page_header = self.render_files_header(window, cx).into_any_element(); + + page_content = self + .render_current_page_items(window, cx) + .into_any_element(); } + let current_sub_page = self.sub_page_stack.last(); + let mut warning_banner = gpui::Empty.into_any_element(); if let Some(error) = SettingsStore::global(cx).error_for_file(self.current_file.to_settings()) @@ -3114,10 +3145,10 @@ impl SettingsWindow { .into_any_element() } - return v_flex() + v_flex() .id("settings-ui-page") .on_action(cx.listener(|this, _: &menu::SelectNext, window, cx| { - if !sub_page_stack().is_empty() { + if !this.sub_page_stack.is_empty() { window.focus_next(cx); return; } @@ -3149,7 +3180,7 @@ impl SettingsWindow { window.focus_next(cx); })) .on_action(cx.listener(|this, _: &menu::SelectPrevious, window, cx| { - if !sub_page_stack().is_empty() { + if !this.sub_page_stack.is_empty() { window.focus_prev(cx); return; } @@ -3181,11 +3212,17 @@ impl SettingsWindow { } window.focus_prev(cx); })) - .when(sub_page_stack().is_empty(), |this| { + .when(current_sub_page.is_none(), |this| { this.vertical_scrollbar_for(&self.list_state, window, cx) }) - .when(!sub_page_stack().is_empty(), |this| { - this.vertical_scrollbar_for(&self.sub_page_scroll_handle, window, cx) + .when_some(current_sub_page, |this, current_sub_page| { + this.custom_scrollbars( + Scrollbars::new(ui::ScrollAxes::Vertical) + .tracked_scroll_handle(¤t_sub_page.scroll_handle) + .id((current_sub_page.link.title.clone(), 42)), + window, + cx, + ) }) .track_focus(&self.content_focus_handle.focus_handle(cx)) .pt_6() @@ -3206,7 +3243,7 @@ impl SettingsWindow { .tab_group() .tab_index(CONTENT_GROUP_TAB_INDEX) .child(page_content), - ); + ) } /// This function will create a new settings file if one doesn't exist @@ -3350,19 +3387,15 @@ impl SettingsWindow { } fn current_page_index(&self) -> usize { - self.page_index_from_navbar_index(self.navbar_entry) - } - - fn current_page(&self) -> &SettingsPage { - &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[index].page_index + self.navbar_entries[self.navbar_entry].page_index + } + + fn current_page(&self) -> &SettingsPage { + &self.pages[self.current_page_index()] } fn is_navbar_entry_selected(&self, ix: usize) -> bool { @@ -3372,22 +3405,18 @@ impl SettingsWindow { fn push_sub_page( &mut self, sub_page_link: SubPageLink, - section_header: &'static str, + section_header: SharedString, window: &mut Window, cx: &mut Context, ) { - sub_page_stack_mut().push(SubPage { - link: sub_page_link, - section_header, - }); - self.sub_page_scroll_handle - .set_offset(point(px(0.), px(0.))); + self.sub_page_stack + .push(SubPage::new(sub_page_link, section_header)); self.content_focus_handle.focus_handle(cx).focus(window, cx); cx.notify(); } fn pop_sub_page(&mut self, window: &mut Window, cx: &mut Context) { - sub_page_stack_mut().pop(); + self.sub_page_stack.pop(); self.content_focus_handle.focus_handle(cx).focus(window, cx); cx.notify(); } @@ -4043,10 +4072,10 @@ pub mod test { navbar_scroll_handle: UniformListScrollHandle::default(), navbar_focus_subscriptions: vec![], filter_table: vec![], + sub_page_stack: vec![], has_query: false, content_handles: vec![], search_task: None, - sub_page_scroll_handle: ScrollHandle::new(), focus_handle: cx.focus_handle(), navbar_focus_handle: NonFocusableHandle::new( NAVBAR_CONTAINER_TAB_INDEX,