settings_ui: Add broken file warning banner (#40823)

Ben Kunkle created

Closes #ISSUE

Release Notes:

- settings_ui: Added a warning banner when the settings file you are
actively editing is in a broken or invalid state.

Change summary

crates/settings/src/settings_store.rs | 122 ++++++++++++++++++++++++++--
crates/settings_ui/src/settings_ui.rs |  33 +++++++
2 files changed, 141 insertions(+), 14 deletions(-)

Detailed changes

crates/settings/src/settings_store.rs 🔗

@@ -148,9 +148,10 @@ pub struct SettingsStore {
     _setting_file_updates: Task<()>,
     setting_file_updates_tx:
         mpsc::UnboundedSender<Box<dyn FnOnce(AsyncApp) -> LocalBoxFuture<'static, Result<()>>>>,
+    file_errors: BTreeMap<SettingsFile, String>,
 }
 
-#[derive(Clone, PartialEq, Debug)]
+#[derive(Clone, PartialEq, Eq, Debug)]
 pub enum SettingsFile {
     User,
     Server,
@@ -159,6 +160,34 @@ pub enum SettingsFile {
     Project((WorktreeId, Arc<RelPath>)),
 }
 
+impl PartialOrd for SettingsFile {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+/// Sorted in order of precedence
+impl Ord for SettingsFile {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        use SettingsFile::*;
+        use std::cmp::Ordering;
+        match (self, other) {
+            (User, User) => Ordering::Equal,
+            (Server, Server) => Ordering::Equal,
+            (Default, Default) => Ordering::Equal,
+            (Project((id1, rel_path1)), Project((id2, rel_path2))) => id1
+                .cmp(id2)
+                .then_with(|| rel_path1.cmp(rel_path2).reverse()),
+            (Project(_), _) => Ordering::Less,
+            (_, Project(_)) => Ordering::Greater,
+            (Server, _) => Ordering::Less,
+            (_, Server) => Ordering::Greater,
+            (User, _) => Ordering::Less,
+            (_, User) => Ordering::Greater,
+        }
+    }
+}
+
 #[derive(Clone)]
 pub struct Editorconfig {
     pub is_root: bool,
@@ -228,6 +257,7 @@ impl SettingsStore {
                     (setting_file_update)(cx.clone()).await.log_err();
                 }
             }),
+            file_errors: BTreeMap::default(),
         }
     }
 
@@ -586,6 +616,24 @@ impl SettingsStore {
 
         (SettingsFile::Default, None)
     }
+
+    fn handle_potential_file_error<R>(
+        &mut self,
+        file: SettingsFile,
+        result: Result<R>,
+    ) -> Result<R> {
+        if let Err(err) = result.as_ref() {
+            let message = err.to_string();
+            self.file_errors.insert(file, message);
+        } else {
+            self.file_errors.remove(&file);
+        }
+        return result;
+    }
+
+    pub fn error_for_file(&self, file: SettingsFile) -> Option<String> {
+        self.file_errors.get(&file).cloned()
+    }
 }
 
 impl SettingsStore {
@@ -658,7 +706,10 @@ impl SettingsStore {
         let settings: UserSettingsContent = if user_settings_content.is_empty() {
             parse_json_with_comments("{}")?
         } else {
-            parse_json_with_comments(user_settings_content)?
+            self.handle_potential_file_error(
+                SettingsFile::User,
+                parse_json_with_comments(user_settings_content),
+            )?
         };
 
         self.user_settings = Some(settings);
@@ -691,7 +742,10 @@ impl SettingsStore {
         let settings: Option<SettingsContent> = if server_settings_content.is_empty() {
             None
         } else {
-            parse_json_with_comments(server_settings_content)?
+            self.handle_potential_file_error(
+                SettingsFile::Server,
+                parse_json_with_comments(server_settings_content),
+            )?
         };
 
         // Rewrite the server settings into a content type
@@ -740,20 +794,24 @@ impl SettingsStore {
                 zed_settings_changed = self
                     .local_settings
                     .remove(&(root_id, directory_path.clone()))
-                    .is_some()
+                    .is_some();
+                self.file_errors
+                    .remove(&SettingsFile::Project((root_id, directory_path.clone())));
             }
             (LocalSettingsKind::Editorconfig, None) => {
                 self.raw_editorconfig_settings
                     .remove(&(root_id, directory_path.clone()));
             }
             (LocalSettingsKind::Settings, Some(settings_contents)) => {
-                let new_settings = parse_json_with_comments::<ProjectSettingsContent>(
-                    settings_contents,
-                )
-                .map_err(|e| InvalidSettingsError::LocalSettings {
-                    path: directory_path.join(local_settings_file_relative_path()),
-                    message: e.to_string(),
-                })?;
+                let new_settings = self
+                    .handle_potential_file_error(
+                        SettingsFile::Project((root_id, directory_path.clone())),
+                        parse_json_with_comments::<ProjectSettingsContent>(settings_contents),
+                    )
+                    .map_err(|e| InvalidSettingsError::LocalSettings {
+                        path: directory_path.join(local_settings_file_relative_path()),
+                        message: e.to_string(),
+                    })?;
                 match self.local_settings.entry((root_id, directory_path.clone())) {
                     btree_map::Entry::Vacant(v) => {
                         v.insert(SettingsContent {
@@ -931,6 +989,7 @@ impl SettingsStore {
             .to_value()
     }
 
+    // todo -> this function never fails, and should not return a result
     fn recompute_values(
         &mut self,
         changed_local_path: Option<(WorktreeId, &RelPath)>,
@@ -2032,4 +2091,45 @@ mod tests {
         let overrides = store.get_overrides_for_field(SettingsFile::Project(wt0_child1), get);
         assert_eq!(overrides, vec![]);
     }
+
+    #[test]
+    fn test_file_ord() {
+        let wt0_root =
+            SettingsFile::Project((WorktreeId::from_usize(0), RelPath::empty().into_arc()));
+        let wt0_child1 =
+            SettingsFile::Project((WorktreeId::from_usize(0), rel_path("child1").into_arc()));
+        let wt0_child2 =
+            SettingsFile::Project((WorktreeId::from_usize(0), rel_path("child2").into_arc()));
+
+        let wt1_root =
+            SettingsFile::Project((WorktreeId::from_usize(1), RelPath::empty().into_arc()));
+        let wt1_subdir =
+            SettingsFile::Project((WorktreeId::from_usize(1), rel_path("subdir").into_arc()));
+
+        let mut files = vec![
+            &wt1_root,
+            &SettingsFile::Default,
+            &wt0_root,
+            &wt1_subdir,
+            &wt0_child2,
+            &SettingsFile::Server,
+            &wt0_child1,
+            &SettingsFile::User,
+        ];
+
+        files.sort();
+        pretty_assertions::assert_eq!(
+            files,
+            vec![
+                &wt0_child2,
+                &wt0_child1,
+                &wt0_root,
+                &wt1_subdir,
+                &wt1_root,
+                &SettingsFile::Server,
+                &SettingsFile::User,
+                &SettingsFile::Default,
+            ]
+        )
+    }
 }

crates/settings_ui/src/settings_ui.rs 🔗

@@ -27,9 +27,9 @@ use std::{
 };
 use title_bar::platform_title_bar::PlatformTitleBar;
 use ui::{
-    ContextMenu, Divider, DividerColor, DropdownMenu, DropdownStyle, IconButtonShape, KeyBinding,
-    KeybindingHint, PopoverMenu, Switch, SwitchColor, Tooltip, TreeViewItem, WithScrollbar,
-    prelude::*,
+    Banner, ContextMenu, Divider, DividerColor, DropdownMenu, DropdownStyle, IconButtonShape,
+    KeyBinding, KeybindingHint, PopoverMenu, Switch, SwitchColor, Tooltip, TreeViewItem,
+    WithScrollbar, prelude::*,
 };
 use ui_input::{NumberField, NumberFieldType};
 use util::{ResultExt as _, paths::PathStyle, rel_path::RelPath};
@@ -2436,6 +2436,32 @@ impl SettingsWindow {
             page_content = (active_page_render_fn)(self, window, cx);
         }
 
+        let mut warning_banner = gpui::Empty.into_any_element();
+        if let Some(error) =
+            SettingsStore::global(cx).error_for_file(self.current_file.to_settings())
+        {
+            warning_banner = v_flex()
+                .pb_4()
+                .child(
+                    Banner::new()
+                        .severity(Severity::Warning)
+                        .child(
+                            Label::new("Your Settings File is in an Invalid State. Setting Values May Be Incorrect, and Changes May Be Lost")
+                                .size(LabelSize::Large),
+                        )
+                        .child(Label::new(error).size(LabelSize::Small).color(Color::Muted))
+                        .action_slot(
+                            Button::new("fix-in-json", "Fix in settings.json")
+                                .tab_index(0_isize)
+                                .style(ButtonStyle::OutlinedGhost)
+                                .on_click(cx.listener(|this, _, _, cx| {
+                                    this.open_current_settings_file(cx);
+                                })),
+                        ),
+                )
+                .into_any_element()
+        }
+
         return v_flex()
             .id("Settings-ui-page")
             .on_action(cx.listener(|this, _: &menu::SelectNext, window, cx| {
@@ -2497,6 +2523,7 @@ impl SettingsWindow {
                 }
                 window.focus_prev();
             }))
+            .child(warning_banner)
             .child(page_header)
             .when(sub_page_stack().is_empty(), |this| {
                 this.vertical_scrollbar_for(self.list_state.clone(), window, cx)