From dd1320e6d19864651df8e672c1b17d6f40295c18 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Wed, 12 Oct 2022 17:05:23 -0700 Subject: [PATCH 1/2] Improved settings writing to be strongly typed and based on settings file content diffs Co-Authored-By: kay@zed.dev --- crates/settings/src/settings.rs | 43 +++-- crates/settings/src/settings_file.rs | 193 +++++++------------- crates/settings/src/watched_json.rs | 105 +++++++++++ crates/theme_selector/src/theme_selector.rs | 6 +- crates/zed/src/main.rs | 9 +- 5 files changed, 206 insertions(+), 150 deletions(-) create mode 100644 crates/settings/src/watched_json.rs diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs index 2e7dc08d16..8ef798640a 100644 --- a/crates/settings/src/settings.rs +++ b/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, pub gutter_debounce: Option, } -#[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, pub hard_tabs: Option, @@ -83,14 +84,14 @@ pub struct EditorSettings { pub enable_language_server: Option, } -#[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, pub working_directory: Option, @@ -134,7 +135,7 @@ pub struct TerminalSettings { pub copy_on_select: Option, } -#[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, #[serde(default)] @@ -229,7 +230,7 @@ pub struct SettingsFileContent { pub staff_mode: Option, } -#[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, @@ -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 { diff --git a/crates/settings/src/settings_file.rs b/crates/settings/src/settings_file.rs index 6a7c96fd81..506ebc8c3d 100644 --- a/crates/settings/src/settings_file.rs +++ b/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, fs: Arc, } impl SettingsFile { - pub fn new(path: &'static Path, fs: Arc) -> Self { - SettingsFile { path, fs } - } - - pub async fn rewrite_settings_file(&self, f: F) -> anyhow::Result<()> - where - F: Fn(String) -> String, - { - let content = self.fs.load(self.path).await?; - - let new_settings = f(content); - - self.fs - .atomic_write(self.path.to_path_buf(), new_settings) - .await?; - - Ok(()) - } -} - -pub fn write_setting(key: &'static str, val: String, cx: &mut MutableAppContext) { - let settings_file = cx.global::().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); -} - -#[derive(Clone)] -pub struct WatchedJsonFile(pub watch::Receiver); - -impl WatchedJsonFile -where - T: 'static + for<'de> Deserialize<'de> + Clone + Default + Send + Sync, -{ - pub async fn new( + pub fn new( + path: &'static Path, + settings_file_content: WatchedJsonFile, fs: Arc, - executor: &executor::Background, - path: impl Into>, ) -> 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 + SettingsFile { + path, + settings_file_content, + fs, + } + } + + pub fn update(cx: &mut MutableAppContext, update: impl FnOnce(&mut SettingsFileContent)) { + let this = cx.global::(); + + let current_file_content = this.settings_file_content.current(); + let mut new_file_content = current_file_content.clone(); + + update(&mut new_file_content); + + let fs = this.fs.clone(); + let path = this.path.clone(); + + 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" + ); } + + let new_json = serde_json::to_string_pretty(new_value) + .expect("Could not serialize new json field to string"); + + diffs.push((key, new_json)); } } + + // Have diffs, rewrite the settings file now. + let mut content = fs.load(path).await?; + + for (key, new_value) in diffs { + content = write_top_level_setting(content, key, &new_value) + } + + fs.atomic_write(path.to_path_buf(), content).await?; + + Ok(()) as Result<()> }) - .detach(); - Self(rx) + .detach_and_log_err(cx); } - - ///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, path: &Path) -> Option { - 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 watch_settings_file( - defaults: Settings, - mut file: WatchedJsonFile, - theme_registry: Arc, - 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, - 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, 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(); } #[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) { diff --git a/crates/settings/src/watched_json.rs b/crates/settings/src/watched_json.rs new file mode 100644 index 0000000000..e304842aa2 --- /dev/null +++ b/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(pub watch::Receiver); + +impl WatchedJsonFile +where + T: 'static + for<'de> Deserialize<'de> + Clone + Default + Send + Sync, +{ + pub async fn new( + fs: Arc, + executor: &executor::Background, + path: impl Into>, + ) -> 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, path: &Path) -> Option { + 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, + theme_registry: Arc, + 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, + 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, 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(); +} diff --git a/crates/theme_selector/src/theme_selector.rs b/crates/theme_selector/src/theme_selector.rs index f3ca38b78b..729cdad739 100644 --- a/crates/theme_selector/src/theme_selector.rs +++ b/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::().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); } diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 1c6a818ef3..a921bc2680 100644 --- a/crates/zed/src/main.rs +++ b/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); From e73270085bd8b748cbb8f46898201fc7608b73ce Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Wed, 12 Oct 2022 17:11:47 -0700 Subject: [PATCH 2/2] Fixed settings --- crates/settings/src/settings.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs index 8ef798640a..88ce173a17 100644 --- a/crates/settings/src/settings.rs +++ b/crates/settings/src/settings.rs @@ -634,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) } @@ -654,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) } @@ -670,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) } @@ -680,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) } @@ -690,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) } @@ -712,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) }