settings_ui: Soft fail on no default & fix language default loading (#39709)

Ben Kunkle created 4 weeks ago

Closes #ISSUE

Release Notes:

- N/A *or* Added/Fixed/Improved ...

Change summary

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(-)

Detailed changes

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<T>(
         &self,
         target_file: SettingsFile,
         pick: fn(&SettingsContent) -> &Option<T>,
-        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))
         );
     }
 

crates/settings_ui/src/page_data.rs

@@ -4314,14 +4314,16 @@ fn language_settings_data() -> Vec<SettingsPageItem> {
         get: fn(&LanguageSettingsContent) -> &Option<T>,
     ) -> &Option<T> {
         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(&current_language);
+        if let Some(current_language_name) = current_language() {
+            if let Some(current_language) = all_languages.languages.0.get(&current_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<T>(

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<T> AnySettingField for SettingField<T> {
@@ -101,17 +102,15 @@ impl<T> AnySettingField for SettingField<T> {
         TypeId::of::<T>()
     }
 
-    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::<UnimplementedSettingField>() {
-            return file.to_settings();
+            return (file.to_settings(), true);
         }
 
-        let (file, _) = cx.global::<SettingsStore>().get_value_from_file(
-            file.to_settings(),
-            self.pick,
-            self.type_name(),
-        );
-        return file;
+        let (file, value) = cx
+            .global::<SettingsStore>()
+            .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::<SettingFieldRenderer>().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<T: From<String> + Into<String> + AsRef<str> + 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<B: Into<bool> + From<bool> + 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<T: NumericStepperType + Send + Sync>(
     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] { <T as strum::VariantArray>::VARIANTS };
     let labels = || -> &'static [&'static str] { <T as strum::VariantNames>::VARIANTS };
 
-    let (_, &current_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()];