Merge pull request #1743 from zed-industries/new-settings-writing

Mikayla Maki created

Improved settings writing to be strongly typed and based on settings file content diffs

Change summary

crates/settings/src/settings.rs             |  61 ++++---
crates/settings/src/settings_file.rs        | 181 +++++++---------------
crates/settings/src/watched_json.rs         | 105 +++++++++++++
crates/theme_selector/src/theme_selector.rs |   6 
crates/zed/src/main.rs                      |   9 
5 files changed, 212 insertions(+), 150 deletions(-)

Detailed changes

crates/settings/src/settings.rs 🔗

@@ -1,5 +1,6 @@
 mod keymap_file;
 pub mod settings_file;
+pub mod watched_json;
 
 use anyhow::Result;
 use gpui::{
@@ -11,7 +12,7 @@ use schemars::{
     schema::{InstanceType, ObjectValidation, Schema, SchemaObject, SingleOrVec},
     JsonSchema,
 };
-use serde::{de::DeserializeOwned, Deserialize};
+use serde::{de::DeserializeOwned, Deserialize, Serialize};
 use serde_json::Value;
 use std::{collections::HashMap, fmt::Write as _, num::NonZeroU32, str, sync::Arc};
 use theme::{Theme, ThemeRegistry};
@@ -45,7 +46,7 @@ pub struct Settings {
     pub staff_mode: bool,
 }
 
-#[derive(Copy, Clone, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Copy, Clone, Debug, Default, Serialize, Deserialize, JsonSchema)]
 pub struct FeatureFlags {
     pub experimental_themes: bool,
 }
@@ -56,13 +57,13 @@ impl FeatureFlags {
     }
 }
 
-#[derive(Copy, Clone, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Copy, Clone, Debug, Default, Serialize, Deserialize, JsonSchema)]
 pub struct GitSettings {
     pub git_gutter: Option<GitGutter>,
     pub gutter_debounce: Option<u64>,
 }
 
-#[derive(Clone, Copy, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Clone, Copy, Debug, Default, Serialize, Deserialize, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum GitGutter {
     #[default]
@@ -72,7 +73,7 @@ pub enum GitGutter {
 
 pub struct GitGutterConfig {}
 
-#[derive(Clone, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)]
 pub struct EditorSettings {
     pub tab_size: Option<NonZeroU32>,
     pub hard_tabs: Option<bool>,
@@ -83,14 +84,14 @@ pub struct EditorSettings {
     pub enable_language_server: Option<bool>,
 }
 
-#[derive(Copy, Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum SoftWrap {
     None,
     EditorWidth,
     PreferredLineLength,
 }
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum FormatOnSave {
     On,
@@ -102,7 +103,7 @@ pub enum FormatOnSave {
     },
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum Formatter {
     LanguageServer,
@@ -112,7 +113,7 @@ pub enum Formatter {
     },
 }
 
-#[derive(Copy, Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum Autosave {
     Off,
@@ -121,7 +122,7 @@ pub enum Autosave {
     OnWindowChange,
 }
 
-#[derive(Clone, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)]
 pub struct TerminalSettings {
     pub shell: Option<Shell>,
     pub working_directory: Option<WorkingDirectory>,
@@ -134,7 +135,7 @@ pub struct TerminalSettings {
     pub copy_on_select: Option<bool>,
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum TerminalBlink {
     Off,
@@ -148,7 +149,7 @@ impl Default for TerminalBlink {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum Shell {
     System,
@@ -162,7 +163,7 @@ impl Default for Shell {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum AlternateScroll {
     On,
@@ -175,7 +176,7 @@ impl Default for AlternateScroll {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum WorkingDirectory {
     CurrentProjectDirectory,
@@ -184,7 +185,7 @@ pub enum WorkingDirectory {
     Always { directory: String },
 }
 
-#[derive(PartialEq, Eq, Debug, Default, Copy, Clone, Hash, Deserialize, JsonSchema)]
+#[derive(PartialEq, Eq, Debug, Default, Copy, Clone, Hash, Serialize, Deserialize, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum DockAnchor {
     #[default]
@@ -193,7 +194,7 @@ pub enum DockAnchor {
     Expanded,
 }
 
-#[derive(Clone, Debug, Default, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)]
 pub struct SettingsFileContent {
     pub experiments: Option<FeatureFlags>,
     #[serde(default)]
@@ -229,7 +230,7 @@ pub struct SettingsFileContent {
     pub staff_mode: Option<bool>,
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub struct LspSettings {
     pub initialization_options: Option<Value>,
@@ -503,6 +504,8 @@ pub fn settings_file_json_schema(
     serde_json::to_value(root_schema).unwrap()
 }
 
+/// Expects the key to be unquoted, and the value to be valid JSON
+/// (e.g. values should be unquoted for numbers and bools, quoted for strings)
 pub fn write_top_level_setting(
     mut settings_content: String,
     top_level_key: &str,
@@ -553,7 +556,7 @@ pub fn write_top_level_setting(
             settings_content.clear();
             write!(
                 settings_content,
-                "{{\n    \"{}\": \"{new_val}\"\n}}\n",
+                "{{\n    \"{}\": {new_val}\n}}\n",
                 top_level_key
             )
             .unwrap();
@@ -561,7 +564,7 @@ pub fn write_top_level_setting(
 
         (_, Some(existing_value_range)) => {
             // Existing theme key, overwrite
-            settings_content.replace_range(existing_value_range, &format!("\"{new_val}\""));
+            settings_content.replace_range(existing_value_range, &new_val);
         }
 
         (Some(first_key_start), None) => {
@@ -581,7 +584,7 @@ pub fn write_top_level_setting(
                 }
             }
 
-            let content = format!(r#""{top_level_key}": "{new_val}","#);
+            let content = format!(r#""{top_level_key}": {new_val},"#);
             settings_content.insert_str(first_key_start, &content);
 
             if row > 0 {
@@ -631,7 +634,8 @@ mod tests {
         "#
         .unindent();
 
-        let settings_after_theme = write_top_level_setting(settings, "theme", "summerfruit-light");
+        let settings_after_theme =
+            write_top_level_setting(settings, "theme", "\"summerfruit-light\"");
 
         assert_eq!(settings_after_theme, new_settings)
     }
@@ -651,7 +655,8 @@ mod tests {
         "#
         .unindent();
 
-        let settings_after_theme = write_top_level_setting(settings, "theme", "summerfruit-light");
+        let settings_after_theme =
+            write_top_level_setting(settings, "theme", "\"summerfruit-light\"");
 
         assert_eq!(settings_after_theme, new_settings)
     }
@@ -667,7 +672,8 @@ mod tests {
         "#
         .unindent();
 
-        let settings_after_theme = write_top_level_setting(settings, "theme", "summerfruit-light");
+        let settings_after_theme =
+            write_top_level_setting(settings, "theme", "\"summerfruit-light\"");
 
         assert_eq!(settings_after_theme, new_settings)
     }
@@ -677,7 +683,8 @@ mod tests {
         let settings = r#"{ "a": "", "ok": true }"#.to_string();
         let new_settings = r#"{ "theme": "summerfruit-light", "a": "", "ok": true }"#;
 
-        let settings_after_theme = write_top_level_setting(settings, "theme", "summerfruit-light");
+        let settings_after_theme =
+            write_top_level_setting(settings, "theme", "\"summerfruit-light\"");
 
         assert_eq!(settings_after_theme, new_settings)
     }
@@ -687,7 +694,8 @@ mod tests {
         let settings = r#"          { "a": "", "ok": true }"#.to_string();
         let new_settings = r#"          { "theme": "summerfruit-light", "a": "", "ok": true }"#;
 
-        let settings_after_theme = write_top_level_setting(settings, "theme", "summerfruit-light");
+        let settings_after_theme =
+            write_top_level_setting(settings, "theme", "\"summerfruit-light\"");
 
         assert_eq!(settings_after_theme, new_settings)
     }
@@ -709,7 +717,8 @@ mod tests {
         "#
         .unindent();
 
-        let settings_after_theme = write_top_level_setting(settings, "theme", "summerfruit-light");
+        let settings_after_theme =
+            write_top_level_setting(settings, "theme", "\"summerfruit-light\"");
 
         assert_eq!(settings_after_theme, new_settings)
     }

crates/settings/src/settings_file.rs 🔗

@@ -1,153 +1,96 @@
+use crate::{watched_json::WatchedJsonFile, write_top_level_setting, SettingsFileContent};
+use anyhow::Result;
 use fs::Fs;
-use futures::StreamExt;
-use gpui::{executor, MutableAppContext};
-use postage::sink::Sink as _;
-use postage::{prelude::Stream, watch};
-use serde::Deserialize;
-
-use std::{path::Path, sync::Arc, time::Duration};
-use theme::ThemeRegistry;
-use util::ResultExt;
-
-use crate::{
-    parse_json_with_comments, write_top_level_setting, KeymapFileContent, Settings,
-    SettingsFileContent,
-};
+use gpui::MutableAppContext;
+use serde_json::Value;
+use std::{path::Path, sync::Arc};
 
 // TODO: Switch SettingsFile to open a worktree and buffer for synchronization
 //       And instant updates in the Zed editor
 #[derive(Clone)]
 pub struct SettingsFile {
     path: &'static Path,
+    settings_file_content: WatchedJsonFile<SettingsFileContent>,
     fs: Arc<dyn Fs>,
 }
 
 impl SettingsFile {
-    pub fn new(path: &'static Path, fs: Arc<dyn Fs>) -> Self {
-        SettingsFile { path, fs }
+    pub fn new(
+        path: &'static Path,
+        settings_file_content: WatchedJsonFile<SettingsFileContent>,
+        fs: Arc<dyn Fs>,
+    ) -> Self {
+        SettingsFile {
+            path,
+            settings_file_content,
+            fs,
+        }
     }
 
-    pub async fn rewrite_settings_file<F>(&self, f: F) -> anyhow::Result<()>
-    where
-        F: Fn(String) -> String,
-    {
-        let content = self.fs.load(self.path).await?;
-
-        let new_settings = f(content);
+    pub fn update(cx: &mut MutableAppContext, update: impl FnOnce(&mut SettingsFileContent)) {
+        let this = cx.global::<SettingsFile>();
 
-        self.fs
-            .atomic_write(self.path.to_path_buf(), new_settings)
-            .await?;
-
-        Ok(())
-    }
-}
+        let current_file_content = this.settings_file_content.current();
+        let mut new_file_content = current_file_content.clone();
 
-pub fn write_setting(key: &'static str, val: String, cx: &mut MutableAppContext) {
-    let settings_file = cx.global::<SettingsFile>().clone();
-    cx.background()
-        .spawn(async move {
-            settings_file
-                .rewrite_settings_file(|settings| write_top_level_setting(settings, key, &val))
-                .await
-        })
-        .detach_and_log_err(cx);
-}
+        update(&mut new_file_content);
 
-#[derive(Clone)]
-pub struct WatchedJsonFile<T>(pub watch::Receiver<T>);
+        let fs = this.fs.clone();
+        let path = this.path.clone();
 
-impl<T> WatchedJsonFile<T>
-where
-    T: 'static + for<'de> Deserialize<'de> + Clone + Default + Send + Sync,
-{
-    pub async fn new(
-        fs: Arc<dyn Fs>,
-        executor: &executor::Background,
-        path: impl Into<Arc<Path>>,
-    ) -> Self {
-        let path = path.into();
-        let settings = Self::load(fs.clone(), &path).await.unwrap_or_default();
-        let mut events = fs.watch(&path, Duration::from_millis(500)).await;
-        let (mut tx, rx) = watch::channel_with(settings);
-        executor
+        cx.background()
             .spawn(async move {
-                while events.next().await.is_some() {
-                    if let Some(settings) = Self::load(fs.clone(), &path).await {
-                        if tx.send(settings).await.is_err() {
-                            break;
+                // Unwrap safety: These values are all guarnteed to be well formed, and we know
+                // that they will deserialize to our settings object. All of the following unwraps
+                // are therefore safe.
+                let tmp = serde_json::to_value(current_file_content).unwrap();
+                let old_json = tmp.as_object().unwrap();
+
+                let new_tmp = serde_json::to_value(new_file_content).unwrap();
+                let new_json = new_tmp.as_object().unwrap();
+
+                // Find changed fields
+                let mut diffs = vec![];
+                for (key, old_value) in old_json.iter() {
+                    let new_value = new_json.get(key).unwrap();
+                    if old_value != new_value {
+                        if matches!(
+                            new_value,
+                            &Value::Null | &Value::Object(_) | &Value::Array(_)
+                        ) {
+                            unimplemented!(
+                                "We only support updating basic values at the top level"
+                            );
                         }
-                    }
-                }
-            })
-            .detach();
-        Self(rx)
-    }
 
-    ///Loads the given watched JSON file. In the special case that the file is
-    ///empty (ignoring whitespace) or is not a file, this will return T::default()
-    async fn load(fs: Arc<dyn Fs>, path: &Path) -> Option<T> {
-        if !fs.is_file(path).await {
-            return Some(T::default());
-        }
+                        let new_json = serde_json::to_string_pretty(new_value)
+                            .expect("Could not serialize new json field to string");
 
-        fs.load(path).await.log_err().and_then(|data| {
-            if data.trim().is_empty() {
-                Some(T::default())
-            } else {
-                parse_json_with_comments(&data).log_err()
-            }
-        })
-    }
-}
+                        diffs.push((key, new_json));
+                    }
+                }
 
-pub fn watch_settings_file(
-    defaults: Settings,
-    mut file: WatchedJsonFile<SettingsFileContent>,
-    theme_registry: Arc<ThemeRegistry>,
-    cx: &mut MutableAppContext,
-) {
-    settings_updated(&defaults, file.0.borrow().clone(), &theme_registry, cx);
-    cx.spawn(|mut cx| async move {
-        while let Some(content) = file.0.recv().await {
-            cx.update(|cx| settings_updated(&defaults, content, &theme_registry, cx));
-        }
-    })
-    .detach();
-}
+                // Have diffs, rewrite the settings file now.
+                let mut content = fs.load(path).await?;
 
-pub fn keymap_updated(content: KeymapFileContent, cx: &mut MutableAppContext) {
-    cx.clear_bindings();
-    KeymapFileContent::load_defaults(cx);
-    content.add_to_cx(cx).log_err();
-}
+                for (key, new_value) in diffs {
+                    content = write_top_level_setting(content, key, &new_value)
+                }
 
-pub fn settings_updated(
-    defaults: &Settings,
-    content: SettingsFileContent,
-    theme_registry: &Arc<ThemeRegistry>,
-    cx: &mut MutableAppContext,
-) {
-    let mut settings = defaults.clone();
-    settings.set_user_settings(content, theme_registry, cx.font_cache());
-    cx.set_global(settings);
-    cx.refresh_windows();
-}
+                fs.atomic_write(path.to_path_buf(), content).await?;
 
-pub fn watch_keymap_file(mut file: WatchedJsonFile<KeymapFileContent>, cx: &mut MutableAppContext) {
-    cx.spawn(|mut cx| async move {
-        while let Some(content) = file.0.recv().await {
-            cx.update(|cx| keymap_updated(content, cx));
-        }
-    })
-    .detach();
+                Ok(()) as Result<()>
+            })
+            .detach_and_log_err(cx);
+    }
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{EditorSettings, SoftWrap};
+    use crate::{watched_json::watch_settings_file, EditorSettings, Settings, SoftWrap};
     use fs::FakeFs;
+    use theme::ThemeRegistry;
 
     #[gpui::test]
     async fn test_watch_settings_files(cx: &mut gpui::TestAppContext) {

crates/settings/src/watched_json.rs 🔗

@@ -0,0 +1,105 @@
+use fs::Fs;
+use futures::StreamExt;
+use gpui::{executor, MutableAppContext};
+use postage::sink::Sink as _;
+use postage::{prelude::Stream, watch};
+use serde::Deserialize;
+
+use std::{path::Path, sync::Arc, time::Duration};
+use theme::ThemeRegistry;
+use util::ResultExt;
+
+use crate::{parse_json_with_comments, KeymapFileContent, Settings, SettingsFileContent};
+
+#[derive(Clone)]
+pub struct WatchedJsonFile<T>(pub watch::Receiver<T>);
+
+impl<T> WatchedJsonFile<T>
+where
+    T: 'static + for<'de> Deserialize<'de> + Clone + Default + Send + Sync,
+{
+    pub async fn new(
+        fs: Arc<dyn Fs>,
+        executor: &executor::Background,
+        path: impl Into<Arc<Path>>,
+    ) -> Self {
+        let path = path.into();
+        let settings = Self::load(fs.clone(), &path).await.unwrap_or_default();
+        let mut events = fs.watch(&path, Duration::from_millis(500)).await;
+        let (mut tx, rx) = watch::channel_with(settings);
+        executor
+            .spawn(async move {
+                while events.next().await.is_some() {
+                    if let Some(settings) = Self::load(fs.clone(), &path).await {
+                        if tx.send(settings).await.is_err() {
+                            break;
+                        }
+                    }
+                }
+            })
+            .detach();
+        Self(rx)
+    }
+
+    ///Loads the given watched JSON file. In the special case that the file is
+    ///empty (ignoring whitespace) or is not a file, this will return T::default()
+    async fn load(fs: Arc<dyn Fs>, path: &Path) -> Option<T> {
+        if !fs.is_file(path).await {
+            return Some(T::default());
+        }
+
+        fs.load(path).await.log_err().and_then(|data| {
+            if data.trim().is_empty() {
+                Some(T::default())
+            } else {
+                parse_json_with_comments(&data).log_err()
+            }
+        })
+    }
+
+    pub fn current(&self) -> T {
+        self.0.borrow().clone()
+    }
+}
+
+pub fn watch_settings_file(
+    defaults: Settings,
+    mut file: WatchedJsonFile<SettingsFileContent>,
+    theme_registry: Arc<ThemeRegistry>,
+    cx: &mut MutableAppContext,
+) {
+    settings_updated(&defaults, file.0.borrow().clone(), &theme_registry, cx);
+    cx.spawn(|mut cx| async move {
+        while let Some(content) = file.0.recv().await {
+            cx.update(|cx| settings_updated(&defaults, content, &theme_registry, cx));
+        }
+    })
+    .detach();
+}
+
+pub fn keymap_updated(content: KeymapFileContent, cx: &mut MutableAppContext) {
+    cx.clear_bindings();
+    KeymapFileContent::load_defaults(cx);
+    content.add_to_cx(cx).log_err();
+}
+
+pub fn settings_updated(
+    defaults: &Settings,
+    content: SettingsFileContent,
+    theme_registry: &Arc<ThemeRegistry>,
+    cx: &mut MutableAppContext,
+) {
+    let mut settings = defaults.clone();
+    settings.set_user_settings(content, theme_registry, cx.font_cache());
+    cx.set_global(settings);
+    cx.refresh_windows();
+}
+
+pub fn watch_keymap_file(mut file: WatchedJsonFile<KeymapFileContent>, cx: &mut MutableAppContext) {
+    cx.spawn(|mut cx| async move {
+        while let Some(content) = file.0.recv().await {
+            cx.update(|cx| keymap_updated(content, cx));
+        }
+    })
+    .detach();
+}

crates/theme_selector/src/theme_selector.rs 🔗

@@ -4,7 +4,7 @@ use gpui::{
     MutableAppContext, RenderContext, View, ViewContext, ViewHandle,
 };
 use picker::{Picker, PickerDelegate};
-use settings::Settings;
+use settings::{settings_file::SettingsFile, Settings};
 use std::sync::Arc;
 use theme::{Theme, ThemeMeta, ThemeRegistry};
 use workspace::{AppState, Workspace};
@@ -155,7 +155,9 @@ impl PickerDelegate for ThemeSelector {
         self.selection_completed = true;
 
         let theme_name = cx.global::<Settings>().theme.meta.name.clone();
-        settings::settings_file::write_setting("theme", theme_name, cx);
+        SettingsFile::update(cx, |settings_content| {
+            settings_content.theme = Some(theme_name);
+        });
 
         cx.emit(Event::Dismissed);
     }

crates/zed/src/main.rs 🔗

@@ -35,7 +35,7 @@ use std::{env, ffi::OsStr, panic, path::PathBuf, sync::Arc, thread, time::Durati
 use terminal::terminal_container_view::{get_working_directory, TerminalContainer};
 
 use fs::RealFs;
-use settings::settings_file::{watch_keymap_file, watch_settings_file, WatchedJsonFile};
+use settings::watched_json::{watch_keymap_file, watch_settings_file, WatchedJsonFile};
 use theme::ThemeRegistry;
 use util::{ResultExt, TryFutureExt};
 use workspace::{self, AppState, ItemHandle, NewFile, OpenPaths, Workspace};
@@ -65,7 +65,6 @@ fn main() {
     let themes = ThemeRegistry::new(Assets, app.font_cache());
     let default_settings = Settings::defaults(Assets, &app.font_cache(), &themes);
 
-    let settings_file = SettingsFile::new(&*zed::paths::SETTINGS, fs.clone());
     let config_files = load_config_files(&app, fs.clone());
 
     let login_shell_env_loaded = if stdout_is_a_pty() {
@@ -101,7 +100,11 @@ fn main() {
         let (settings_file_content, keymap_file) = cx.background().block(config_files).unwrap();
 
         //Setup settings global before binding actions
-        cx.set_global(settings_file);
+        cx.set_global(SettingsFile::new(
+            &*zed::paths::SETTINGS,
+            settings_file_content.clone(),
+            fs.clone(),
+        ));
         watch_settings_file(default_settings, settings_file_content, themes.clone(), cx);
         watch_keymap_file(keymap_file, cx);