From 791ba9ce4c2bbb5ffee06a8a8cbb12652f307744 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Tue, 7 Oct 2025 15:27:42 -0500 Subject: [PATCH] settings_ui: Soft fail on no default & fix language default loading (#39709) Closes #ISSUE Release Notes: - N/A *or* Added/Fixed/Improved ... --- crates/settings/src/settings_store.rs | 75 ++++++++--------------- crates/settings_ui/src/page_data.rs | 16 ++--- crates/settings_ui/src/settings_ui.rs | 86 +++++++++++++-------------- 3 files changed, 76 insertions(+), 101 deletions(-) diff --git a/crates/settings/src/settings_store.rs b/crates/settings/src/settings_store.rs index ab65a43c011696f54e9c4875fe68d70897866046..2a94ecb8cdbb77cbd55b0ab5a2bc1474052f2b6f 100644 --- a/crates/settings/src/settings_store.rs +++ b/crates/settings/src/settings_store.rs @@ -534,12 +534,15 @@ impl SettingsStore { overrides } + /// Checks the given file, and files that the passed file overrides for the given field. + /// Returns the first file found that contains the value. + /// The value will only be None if no file contains the value. + /// I.e. if no file contains the value, returns `(File::Default, None)` pub fn get_value_from_file( &self, target_file: SettingsFile, pick: fn(&SettingsContent) -> &Option, - type_name: &'static str, - ) -> (SettingsFile, &T) { + ) -> (SettingsFile, Option<&T>) { // TODO: Add a metadata field for overriding the "overrides" tag, for contextually different settings // e.g. disable AI isn't overridden, or a vec that gets extended instead or some such @@ -565,11 +568,11 @@ impl SettingsStore { continue; }; if let Some(value) = pick(content).as_ref() { - return (file, value); + return (file, Some(value)); } } - unreachable!("{type_name}: doesn't have a default value"); + (SettingsFile::Default, None) } } @@ -1715,25 +1718,17 @@ mod tests { let default_value = get(&store.default_settings).unwrap(); assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local.clone()), - get, - "preferred line length" - ), - (SettingsFile::User, &0) + store.get_value_from_file(SettingsFile::Local(local.clone()), get), + (SettingsFile::User, Some(&0)) ); assert_eq!( - store.get_value_from_file(SettingsFile::User, get, "preferred line length"), - (SettingsFile::User, &0) + store.get_value_from_file(SettingsFile::User, get), + (SettingsFile::User, Some(&0)) ); store.set_user_settings(r#"{}"#, cx).unwrap(); assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local.clone()), - get, - "preferred line length" - ), - (SettingsFile::Default, &default_value) + store.get_value_from_file(SettingsFile::Local(local.clone()), get), + (SettingsFile::Default, Some(&default_value)) ); store .set_local_settings( @@ -1745,16 +1740,12 @@ mod tests { ) .unwrap(); assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local.clone()), - get, - "preferred line length" - ), - (SettingsFile::Local(local), &80) + store.get_value_from_file(SettingsFile::Local(local.clone()), get), + (SettingsFile::Local(local), Some(&80)) ); assert_eq!( - store.get_value_from_file(SettingsFile::User, get, "preferred line length"), - (SettingsFile::Default, &default_value) + store.get_value_from_file(SettingsFile::User, get), + (SettingsFile::Default, Some(&default_value)) ); } @@ -1830,20 +1821,12 @@ mod tests { // each local child should only inherit from it's parent assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local_2_child), - get, - "preferred_line_length" - ), - (SettingsFile::Local(local_2), &2) + store.get_value_from_file(SettingsFile::Local(local_2_child), get), + (SettingsFile::Local(local_2), Some(&2)) ); assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local_1_child.clone()), - get, - "preferred_line_length" - ), - (SettingsFile::Local(local_1.clone()), &1) + store.get_value_from_file(SettingsFile::Local(local_1_child.clone()), get), + (SettingsFile::Local(local_1.clone()), Some(&1)) ); // adjacent children should be treated as siblings not inherit from each other @@ -1868,12 +1851,8 @@ mod tests { .unwrap(); assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local_1_adjacent_child.clone()), - get, - "preferred_line_length" - ), - (SettingsFile::Local(local_1.clone()), &1) + store.get_value_from_file(SettingsFile::Local(local_1_adjacent_child.clone()), get), + (SettingsFile::Local(local_1.clone()), Some(&1)) ); store .set_local_settings( @@ -1894,12 +1873,8 @@ mod tests { ) .unwrap(); assert_eq!( - store.get_value_from_file( - SettingsFile::Local(local_1_child), - get, - "preferred_line_length" - ), - (SettingsFile::Local(local_1), &1) + store.get_value_from_file(SettingsFile::Local(local_1_child), get), + (SettingsFile::Local(local_1), Some(&1)) ); } diff --git a/crates/settings_ui/src/page_data.rs b/crates/settings_ui/src/page_data.rs index 00fdbfb035656c265992cb2d1a734bcc49a01bfb..8d8a52585a0dc5324be5d41df44495c8ecafaca6 100644 --- a/crates/settings_ui/src/page_data.rs +++ b/crates/settings_ui/src/page_data.rs @@ -4314,14 +4314,16 @@ fn language_settings_data() -> Vec { get: fn(&LanguageSettingsContent) -> &Option, ) -> &Option { let all_languages = &settings_content.project.all_languages; - let mut language_content = None; - if let Some(current_language) = current_language() { - language_content = all_languages.languages.0.get(¤t_language); + 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 value = language_content - .map(get) - .unwrap_or_else(|| get(&all_languages.defaults)); - return value; + let default_value = get(&all_languages.defaults); + return default_value; } fn language_settings_field_mut( diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index 6bb35d53a28852eb0aa6b4e3bc41b3020bddab0d..90a5ea532391cb51e43b2f1bc61aca327cfbc90c 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -85,7 +85,8 @@ trait AnySettingField { fn as_any(&self) -> &dyn Any; fn type_name(&self) -> &'static str; fn type_id(&self) -> TypeId; - fn file_set_in(&self, file: SettingsUiFile, cx: &App) -> settings::SettingsFile; + // Returns the file this value was set in and true, or File::Default and false to indicate it was not found in any file (missing default) + fn file_set_in(&self, file: SettingsUiFile, cx: &App) -> (settings::SettingsFile, bool); } impl AnySettingField for SettingField { @@ -101,17 +102,15 @@ impl AnySettingField for SettingField { TypeId::of::() } - fn file_set_in(&self, file: SettingsUiFile, cx: &App) -> settings::SettingsFile { + fn file_set_in(&self, file: SettingsUiFile, cx: &App) -> (settings::SettingsFile, bool) { if AnySettingField::type_id(self) == TypeId::of::() { - return file.to_settings(); + return (file.to_settings(), true); } - let (file, _) = cx.global::().get_value_from_file( - file.to_settings(), - self.pick, - self.type_name(), - ); - return file; + let (file, value) = cx + .global::() + .get_value_from_file(file.to_settings(), self.pick); + return (file, value.is_some()); } } @@ -548,8 +547,8 @@ impl SettingsPageItem { .into_any_element(), SettingsPageItem::SettingItem(setting_item) => { let renderer = cx.default_global::().clone(); - let file_set_in = - SettingsUiFile::from_settings(setting_item.field.file_set_in(file.clone(), cx)); + let (found_in_file, found) = setting_item.field.file_set_in(file.clone(), cx); + let file_set_in = SettingsUiFile::from_settings(found_in_file); h_flex() .id(setting_item.title) @@ -595,13 +594,24 @@ impl SettingsPageItem { .color(Color::Muted), ), ) - .child(renderer.render( - setting_item.field.as_ref(), - file, - setting_item.metadata.as_deref(), - window, - cx, - )) + .child(if cfg!(debug_assertions) && !found { + Button::new("no-default-field", "NO DEFAULT") + .size(ButtonSize::Medium) + .icon(IconName::XCircle) + .icon_position(IconPosition::Start) + .icon_color(Color::Error) + .icon_size(IconSize::Small) + .style(ButtonStyle::Outlined) + .into_any_element() + } else { + renderer.render( + setting_item.field.as_ref(), + file, + setting_item.metadata.as_deref(), + window, + cx, + ) + }) .into_any_element() } SettingsPageItem::SubPageLink(sub_page_link) => h_flex() @@ -1472,17 +1482,14 @@ fn render_text_field + Into + AsRef + Clone>( metadata: Option<&SettingsFieldMetadata>, cx: &mut App, ) -> AnyElement { - let (_, initial_text) = SettingsStore::global(cx).get_value_from_file( - file.to_settings(), - field.pick, - field.type_name(), - ); - let initial_text = Some(initial_text.clone()).filter(|s| !s.as_ref().is_empty()); + let (_, initial_text) = + SettingsStore::global(cx).get_value_from_file(file.to_settings(), field.pick); + let initial_text = initial_text.filter(|s| !s.as_ref().is_empty()); SettingsEditor::new() .tab_index(0) .when_some(initial_text, |editor, text| { - editor.with_initial_text(text.into()) + editor.with_initial_text(text.as_ref().to_string()) }) .when_some( metadata.and_then(|metadata| metadata.placeholder), @@ -1504,13 +1511,9 @@ fn render_toggle_button + From + Copy>( file: SettingsUiFile, cx: &mut App, ) -> AnyElement { - let (_, &value) = SettingsStore::global(cx).get_value_from_file( - file.to_settings(), - field.pick, - field.type_name(), - ); + let (_, value) = SettingsStore::global(cx).get_value_from_file(file.to_settings(), field.pick); - let toggle_state = if value.into() { + let toggle_state = if value.copied().map_or(false, Into::into) { ToggleState::Selected } else { ToggleState::Unselected @@ -1539,9 +1542,10 @@ fn render_font_picker( cx: &mut App, ) -> AnyElement { let current_value = SettingsStore::global(cx) - .get_value_from_file(file.to_settings(), field.pick, field.type_name()) + .get_value_from_file(file.to_settings(), field.pick) .1 - .clone(); + .cloned() + .unwrap_or_else(|| SharedString::default().into()); let font_picker = cx.new(|cx| { ui_input::font_picker( @@ -1596,12 +1600,8 @@ fn render_numeric_stepper( window: &mut Window, cx: &mut App, ) -> AnyElement { - let (_, &value) = SettingsStore::global(cx).get_value_from_file( - file.to_settings(), - field.pick, - field.type_name(), - ); - + let (_, value) = SettingsStore::global(cx).get_value_from_file(file.to_settings(), field.pick); + let value = value.copied().unwrap_or_else(T::min_value); NumericStepper::new("numeric_stepper", value, window, cx) .on_change({ move |value, _window, cx| { @@ -1629,11 +1629,9 @@ where let variants = || -> &'static [T] { ::VARIANTS }; let labels = || -> &'static [&'static str] { ::VARIANTS }; - let (_, ¤t_value) = SettingsStore::global(cx).get_value_from_file( - file.to_settings(), - field.pick, - field.type_name(), - ); + let (_, current_value) = + SettingsStore::global(cx).get_value_from_file(file.to_settings(), field.pick); + let current_value = current_value.copied().unwrap_or(variants()[0]); let current_value_label = labels()[variants().iter().position(|v| *v == current_value).unwrap()];