From 054029120441556d7fdaecbbd40e08049108a669 Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Tue, 30 Jul 2024 13:09:50 +0200 Subject: [PATCH] settings: Introduce PRESERVED_KEYS to write default values (#15474) This adds the optional `PRESERVED_KEYS` constant to the `Settings` trait, which allows users of the trait to specify which keys should be written to the settings file, even if their current value matches the default value. That's useful for tagged settings that have, for example, a `"version"` field that should always be present in the user settings file, so we can then reparse the user settings based on the version. Co-Authored-By: Thorsten Release Notes: - N/A --------- Co-authored-by: Thorsten --- Cargo.lock | 1 + crates/assistant/Cargo.toml | 1 + crates/assistant/src/assistant_settings.rs | 169 +++++++++------------ crates/settings/src/settings_store.rs | 20 ++- 4 files changed, 90 insertions(+), 101 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ecf93e2c0536bee05e23226c2dd21f28eebf5c26..14d247645e9dcd1beafce69e5f3ddec9cab9abfe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -439,6 +439,7 @@ dependencies = [ "semantic_index", "serde", "serde_json", + "serde_json_lenient", "settings", "similar", "smol", diff --git a/crates/assistant/Cargo.toml b/crates/assistant/Cargo.toml index 6cf00085cfdb84d7d9d1c58f062a674244967ab2..29e460b8a6c22af7f4b69d52bf1175400bcbccde 100644 --- a/crates/assistant/Cargo.toml +++ b/crates/assistant/Cargo.toml @@ -85,5 +85,6 @@ language = { workspace = true, features = ["test-support"] } log.workspace = true project = { workspace = true, features = ["test-support"] } rand.workspace = true +serde_json_lenient.workspace = true text = { workspace = true, features = ["test-support"] } unindent.workspace = true diff --git a/crates/assistant/src/assistant_settings.rs b/crates/assistant/src/assistant_settings.rs index 3242096d814de79ea8abb8fb0086388403acc45d..a438e842d626aa7b9617e55ae420ae26b6afcd8a 100644 --- a/crates/assistant/src/assistant_settings.rs +++ b/crates/assistant/src/assistant_settings.rs @@ -456,6 +456,8 @@ pub struct LegacyAssistantSettingsContent { impl Settings for AssistantSettings { const KEY: Option<&'static str> = Some("assistant"); + const PRESERVED_KEYS: Option<&'static [&'static str]> = Some(&["version"]); + type FileContent = AssistantSettingsContent; fn load( @@ -497,103 +499,70 @@ fn merge(target: &mut T, value: Option) { } } -// #[cfg(test)] -// mod tests { -// use gpui::{AppContext, UpdateGlobal}; -// use settings::SettingsStore; - -// use super::*; - -// #[gpui::test] -// fn test_deserialize_assistant_settings(cx: &mut AppContext) { -// let store = settings::SettingsStore::test(cx); -// cx.set_global(store); - -// // Settings default to gpt-4-turbo. -// AssistantSettings::register(cx); -// assert_eq!( -// AssistantSettings::get_global(cx).provider, -// AssistantProvider::OpenAi { -// model: OpenAiModel::FourOmni, -// api_url: open_ai::OPEN_AI_API_URL.into(), -// low_speed_timeout_in_seconds: None, -// available_models: Default::default(), -// } -// ); - -// // Ensure backward-compatibility. -// SettingsStore::update_global(cx, |store, cx| { -// store -// .set_user_settings( -// r#"{ -// "assistant": { -// "openai_api_url": "test-url", -// } -// }"#, -// cx, -// ) -// .unwrap(); -// }); -// assert_eq!( -// AssistantSettings::get_global(cx).provider, -// AssistantProvider::OpenAi { -// model: OpenAiModel::FourOmni, -// api_url: "test-url".into(), -// low_speed_timeout_in_seconds: None, -// available_models: Default::default(), -// } -// ); -// SettingsStore::update_global(cx, |store, cx| { -// store -// .set_user_settings( -// r#"{ -// "assistant": { -// "default_open_ai_model": "gpt-4-0613" -// } -// }"#, -// cx, -// ) -// .unwrap(); -// }); -// assert_eq!( -// AssistantSettings::get_global(cx).provider, -// AssistantProvider::OpenAi { -// model: OpenAiModel::Four, -// api_url: open_ai::OPEN_AI_API_URL.into(), -// low_speed_timeout_in_seconds: None, -// available_models: Default::default(), -// } -// ); - -// // The new version supports setting a custom model when using zed.dev. -// SettingsStore::update_global(cx, |store, cx| { -// store -// .set_user_settings( -// r#"{ -// "assistant": { -// "version": "1", -// "provider": { -// "name": "zed.dev", -// "default_model": { -// "custom": { -// "name": "custom-provider" -// } -// } -// } -// } -// }"#, -// cx, -// ) -// .unwrap(); -// }); -// assert_eq!( -// AssistantSettings::get_global(cx).provider, -// AssistantProvider::ZedDotDev { -// model: CloudModel::Custom { -// name: "custom-provider".into(), -// max_tokens: None -// } -// } -// ); -// } -// } +#[cfg(test)] +mod tests { + use gpui::{ReadGlobal, TestAppContext}; + + use super::*; + + #[gpui::test] + async fn test_deserialize_assistant_settings_with_version(cx: &mut TestAppContext) { + let fs = fs::FakeFs::new(cx.executor().clone()); + fs.create_dir(paths::settings_file().parent().unwrap()) + .await + .unwrap(); + + cx.update(|cx| { + let test_settings = settings::SettingsStore::test(cx); + cx.set_global(test_settings); + AssistantSettings::register(cx); + }); + + cx.update(|cx| { + assert!(!AssistantSettings::get_global(cx).using_outdated_settings_version); + assert_eq!( + AssistantSettings::get_global(cx).default_model, + AssistantDefaultModel { + provider: "openai".into(), + model: "gpt-4o".into(), + } + ); + }); + + cx.update(|cx| { + settings::SettingsStore::global(cx).update_settings_file::( + fs.clone(), + |settings, _| { + *settings = AssistantSettingsContent::Versioned( + VersionedAssistantSettingsContent::V2(AssistantSettingsContentV2 { + default_model: Some(AssistantDefaultModel { + provider: "test-provider".into(), + model: "gpt-99".into(), + }), + enabled: None, + button: None, + dock: None, + default_width: None, + default_height: None, + }), + ) + }, + ); + }); + + cx.run_until_parked(); + + let raw_settings_value = fs.load(paths::settings_file()).await.unwrap(); + assert!(raw_settings_value.contains(r#""version": "2""#)); + + #[derive(Debug, Deserialize)] + struct AssistantSettingsTest { + assistant: AssistantSettingsContent, + } + + let assistant_settings: AssistantSettingsTest = + serde_json_lenient::from_str(&raw_settings_value).unwrap(); + + assert!(!assistant_settings.assistant.is_version_outdated()); + } +} diff --git a/crates/settings/src/settings_store.rs b/crates/settings/src/settings_store.rs index 2833d31f677f9ce579f209dded430adce1861b3c..daad11c1ca35af36feba889184a729b29d37f4aa 100644 --- a/crates/settings/src/settings_store.rs +++ b/crates/settings/src/settings_store.rs @@ -28,6 +28,14 @@ pub trait Settings: 'static + Send + Sync { /// from the root object. const KEY: Option<&'static str>; + /// The name of the keys in the [`FileContent`] that should always be written to + /// a settings file, even if their value matches the default value. + /// + /// This is useful for tagged [`FileContent`]s where the tag is a "version" field + /// that should always be persisted, even if the current user settings match the + /// current version of the settings. + const PRESERVED_KEYS: Option<&'static [&'static str]> = None; + /// The type that is stored in an individual JSON file. type FileContent: Clone + Default + Serialize + DeserializeOwned + JsonSchema; @@ -407,6 +415,8 @@ impl SettingsStore { ) -> Vec<(Range, String)> { let setting_type_id = TypeId::of::(); + let preserved_keys = T::PRESERVED_KEYS.unwrap_or_default(); + let setting = self .setting_values .get(&setting_type_id) @@ -436,6 +446,7 @@ impl SettingsStore { tab_size, &old_value, &new_value, + preserved_keys, &mut edits, ); edits @@ -874,6 +885,7 @@ fn update_value_in_json_text<'a>( tab_size: usize, old_value: &'a serde_json::Value, new_value: &'a serde_json::Value, + preserved_keys: &[&str], edits: &mut Vec<(Range, String)>, ) { // If the old and new values are both objects, then compare them key by key, @@ -891,6 +903,7 @@ fn update_value_in_json_text<'a>( tab_size, old_sub_value, new_sub_value, + preserved_keys, edits, ); key_path.pop(); @@ -904,12 +917,17 @@ fn update_value_in_json_text<'a>( tab_size, &serde_json::Value::Null, new_sub_value, + preserved_keys, edits, ); } key_path.pop(); } - } else if old_value != new_value { + } else if key_path + .last() + .map_or(false, |key| preserved_keys.contains(&key)) + || old_value != new_value + { let mut new_value = new_value.clone(); if let Some(new_object) = new_value.as_object_mut() { new_object.retain(|_, v| !v.is_null());