From f501cae73bd141eb499934befb993a906cc5c390 Mon Sep 17 00:00:00 2001 From: aiongg Date: Tue, 20 Oct 2026 15:49:51 +0000 Subject: [PATCH] keymap: settle the public API before release Outcome, ConflictMode, ProfileError and Options are non_exhaustive, with Options::with for options built in one expression. Profiles keeps its fields private behind active() and sets(), lists the sets loading dropped in problems(), and stores a format version (a store without one reads as version 1). --- crates/keymap/README.md | 7 +- crates/keymap/src/lib.rs | 2 +- crates/keymap/src/profiles.rs | 200 +++++++++++++++++++++++------ crates/keymap/src/ui/mod.rs | 15 ++- crates/keymap/src/ui/state.rs | 30 ++--- crates/keymap/tests/profiles.rs | 54 ++++++-- crates/keymap/tests/settings.rs | 10 +- crates/keymap/tests/ui_dialog.rs | 14 +- crates/keymap/tests/ui_snapshot.rs | 2 +- 9 files changed, 249 insertions(+), 85 deletions(-) diff --git a/crates/keymap/README.md b/crates/keymap/README.md index f5ff51eb1..ec3b55a99 100644 --- a/crates/keymap/README.md +++ b/crates/keymap/README.md @@ -163,7 +163,8 @@ let keymap = Keymap::new(catalog, Scopes::new(), Platform::current()).with_overr store.update(|p| p.shortcut_sets = sets.clone()); ``` -- `names`, `get`, `active_name`, `active_overrides` (hand them to `Keymap::set_overrides`). +- `names`, `get`, `active_name`, `active_overrides` (hand them to `Keymap::set_overrides`); + `sets` (the user's sets) and `active` (the stored name, which `active_name` resolves to a set). - `select` switches sets; `duplicate(from, to)` copies a built-in or user set; `save_as`, `rename` and `delete` work on user sets only. - `store(overrides, builtins, copy_name)` saves the editor's result to the active set. Editing a @@ -173,6 +174,10 @@ store.update(|p| p.shortcut_sets = sets.clone()); - `export(name)` writes one set as JSON (`{"format":"craft-keymap","version":1,"name":…,"overrides":…}`); `import(text)` reads it into a new set. A file from a newer version is refused (`ProfileError::NewerVersion`). +- Loading never fails. `problems()` lists what loading dropped: sets with a bad or repeated name, + overrides that are neither a list nor a map, sets past `MAX_SETS`, and a `version` newer than + this crate writes. The app shows them before the next save writes the sets without them. + Stored `Profiles` carry `version = 1`; a file without it reads as version 1. - `import_plan(&plan, &keymap, name)` applies an import plan (see Importing a shortcut set) to a copy of the keymap and keeps the result as a new set, so another application's set becomes one the user can switch to and away from. diff --git a/crates/keymap/src/lib.rs b/crates/keymap/src/lib.rs index 513df9552..35c7a35f7 100644 --- a/crates/keymap/src/lib.rs +++ b/crates/keymap/src/lib.rs @@ -157,5 +157,5 @@ pub use keymap::{ AssignError, Assigned, Bindable, Binding, BindingError, Conflict, ConflictKind, DropReason, Holder, Keymap, Kind, Lookup, MAX_BINDING_TEXT, MAX_BINDINGS, MAX_COMMANDS, MAX_CONFLICTS, Overrides, Policy, Problem, TypingRule, }; -pub use profiles::{Builtins, MAX_EXPORT_BYTES, MAX_SET_NAME, MAX_SETS, ProfileError, Profiles}; +pub use profiles::{Builtins, MAX_EXPORT_BYTES, MAX_SET_NAME, MAX_SETS, ProfileError, Profiles, SetProblem}; pub use scope::{Active, MAX_SCOPE_NAME, Scope, ScopeKind, Scopes}; diff --git a/crates/keymap/src/profiles.rs b/crates/keymap/src/profiles.rs index baa4b4b9f..df9634db6 100644 --- a/crates/keymap/src/profiles.rs +++ b/crates/keymap/src/profiles.rs @@ -19,6 +19,7 @@ use std::collections::BTreeMap; use std::fmt; use serde::de::{self, IgnoredAny, MapAccess, SeqAccess, Visitor}; +use serde::ser::SerializeStruct; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde_json::{Value, json}; @@ -34,6 +35,10 @@ pub const MAX_EXPORT_BYTES: usize = 4 << 20; const FORMAT: &str = "craft-keymap"; const VERSION: u64 = 1; +/// The version stored [`Profiles`] carry; stored profiles without one read as version 1. +const STORE_VERSION: u64 = 1; +// Load notes are kept for `Profiles::problems`; a hostile file must not grow them without bound. +const MAX_NOTES: usize = 64; const IMPORTED: &str = "Imported"; /// The app's built-in sets: name and overrides against the catalog defaults. @@ -42,6 +47,7 @@ pub type Builtins<'a> = [(&'a str, Overrides)]; /// Why a set operation was refused. #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] +#[non_exhaustive] pub enum ProfileError { /// Empty, over [`MAX_SET_NAME`] chars, or with control characters. BadName, @@ -80,23 +86,51 @@ impl fmt::Display for ProfileError { impl std::error::Error for ProfileError {} +/// Something loading dropped from stored [`Profiles`]; [`Profiles::problems`] lists them. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +#[non_exhaustive] +pub enum SetProblem { + /// A set without a valid name: none, empty, over [`MAX_SET_NAME`] chars, with control + /// characters or with spaces around it. `name` is what was stored, cut to [`MAX_SET_NAME`] + /// chars. + BadName { name: String }, + /// A second set of this name; the first one stays. + Duplicate { name: String }, + /// Overrides that are neither a list nor a map. + BadOverrides { name: String }, + /// Sets past [`MAX_SETS`]. + TooMany { ignored: usize }, + /// Stored by a newer craft-keymap; what this version reads is loaded. + NewerVersion(u64), +} + /// The user's sets and which set is active. The built-in sets are passed to each call. /// -/// An `active` name that matches no set means the first built-in set (or no overrides when the app +/// An active name that matches no set means the first built-in set (or no overrides when the app /// has none). /// /// A user set may carry a built-in's name, for instance after an update adds that built-in. Until /// [`Profiles::reconcile`] renames it, the user set is the one that name reads and writes. Call /// `reconcile` after loading; every method that changes the profiles calls it first. -#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] -#[serde(default)] +/// +/// Stored as `{ version, active, sets }`. Loading never fails: what it drops is listed by +/// [`Profiles::problems`], and the next save writes the profiles without it. +#[derive(Clone, Debug, Default)] pub struct Profiles { - #[serde(deserialize_with = "lenient_name")] - pub active: String, - #[serde(serialize_with = "sets_as_list", deserialize_with = "capped_sets")] - pub sets: BTreeMap, + active: String, + sets: BTreeMap, + notes: Vec, } +impl PartialEq for Profiles { + fn eq(&self, other: &Profiles) -> bool { + self.active == other.active && self.sets == other.sets + } +} + +impl Eq for Profiles {} + static NO_OVERRIDES: Overrides = Overrides::new(); /// A name as given, trimmed; `None` when empty, too long or with control characters. @@ -107,6 +141,22 @@ pub fn set_name(name: &str) -> Option { } impl Profiles { + /// The stored active name, which may match no set; [`active_name`](Self::active_name) is the + /// set in use. + pub fn active(&self) -> &str { + &self.active + } + + /// The user's sets. + pub fn sets(&self) -> &BTreeMap { + &self.sets + } + + /// What loading dropped. + pub fn problems(&self) -> &[SetProblem] { + &self.notes + } + /// Built-in names first, then the user's sets. pub fn names(&self, builtins: &Builtins) -> Vec { builtins.iter().map(|(n, _)| n.to_string()).chain(self.sets.keys().cloned()).collect() @@ -345,25 +395,105 @@ impl Profiles { } } +impl Serialize for Profiles { + fn serialize(&self, serializer: S) -> Result { + let mut s = serializer.serialize_struct("Profiles", 3)?; + s.serialize_field("version", &STORE_VERSION)?; + s.serialize_field("active", &self.active)?; + s.serialize_field("sets", &SetList(&self.sets))?; + s.end() + } +} + +impl<'de> Deserialize<'de> for Profiles { + fn deserialize>(deserializer: D) -> Result { + #[derive(Deserialize, Default)] + #[serde(default)] + struct Stored { + #[serde(deserialize_with = "lenient_version")] + version: Option, + #[serde(deserialize_with = "lenient_name")] + active: String, + #[serde(deserialize_with = "capped_sets")] + sets: Loaded, + } + let Stored { version, active, sets: Loaded { sets, mut notes, .. } } = Stored::deserialize(deserializer)?; + if let Some(v) = version.filter(|v| *v > STORE_VERSION) { + notes.insert(0, SetProblem::NewerVersion(v)); + notes.truncate(MAX_NOTES); + } + Ok(Profiles { active, sets, notes }) + } +} + fn lenient_name<'de, D: Deserializer<'de>>(deserializer: D) -> Result { let v = Value::deserialize(deserializer)?; Ok(v.as_str().and_then(set_name).unwrap_or_default()) } +fn lenient_version<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { + Ok(Value::deserialize(deserializer)?.as_u64()) +} + // An array of `{ name, overrides }` rather than a table, and each set's overrides in their array // form, so that craft-settings (which replaces arrays whole) drops a deleted set or entry from the // file. -fn sets_as_list(sets: &BTreeMap, serializer: S) -> Result { - #[derive(Serialize)] - struct Set<'a> { - name: &'a str, - #[serde(serialize_with = "crate::keymap::overrides_list::serialize")] - overrides: &'a Overrides, +struct SetList<'a>(&'a BTreeMap); + +impl Serialize for SetList<'_> { + fn serialize(&self, serializer: S) -> Result { + #[derive(Serialize)] + struct Set<'a> { + name: &'a str, + #[serde(serialize_with = "crate::keymap::overrides_list::serialize")] + overrides: &'a Overrides, + } + serializer.collect_seq(self.0.iter().map(|(name, overrides)| Set { name, overrides })) } - serializer.collect_seq(sets.iter().map(|(name, overrides)| Set { name, overrides })) } -fn capped_sets<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { +/// The sets that loaded, and notes on the ones that did not. +#[derive(Default)] +struct Loaded { + sets: BTreeMap, + notes: Vec, + ignored: usize, +} + +impl Loaded { + fn note(&mut self, problem: SetProblem) { + if self.notes.len() < MAX_NOTES { + self.notes.push(problem); + } + } + + /// Keeps a set with a valid name, stored as written, that is new and fits under [`MAX_SETS`]; + /// notes why not otherwise. + fn add(&mut self, name: &str, overrides: Option) { + let shown = || name.chars().take(MAX_SET_NAME).collect::(); + if !set_name(name).is_some_and(|n| n == name) { + self.note(SetProblem::BadName { name: shown() }); + } else if self.sets.contains_key(name) { + self.note(SetProblem::Duplicate { name: shown() }); + } else if self.sets.len() >= MAX_SETS { + self.ignored += 1; + } else if let Some(o) = overrides { + self.sets.insert(name.to_string(), o); + } else { + self.note(SetProblem::BadOverrides { name: shown() }); + } + } + + fn done(mut self) -> Loaded { + if self.ignored > 0 { + let ignored = std::mem::take(&mut self.ignored); + self.note(SetProblem::TooMany { ignored }); + } + self + } +} + +fn capped_sets<'de, D: Deserializer<'de>>(deserializer: D) -> Result { deserializer.deserialize_any(SetsVisitor) } @@ -373,13 +503,13 @@ struct SetsVisitor; macro_rules! no_sets { ($($name:ident: $t:ty),*) => { $(fn $name(self, _: $t) -> Result { - Ok(BTreeMap::new()) + Ok(Loaded::default()) })* }; } impl<'de> Visitor<'de> for SetsVisitor { - type Value = BTreeMap; + type Value = Loaded; fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { f.write_str("a list of `{ name, overrides }` items or a map of set names to overrides") @@ -388,11 +518,11 @@ impl<'de> Visitor<'de> for SetsVisitor { no_sets!(visit_bool: bool, visit_i64: i64, visit_u64: u64, visit_i128: i128, visit_u128: u128, visit_f64: f64, visit_str: &str, visit_bytes: &[u8], visit_char: char); fn visit_unit(self) -> Result { - Ok(BTreeMap::new()) + Ok(Loaded::default()) } fn visit_none(self) -> Result { - Ok(BTreeMap::new()) + Ok(Loaded::default()) } fn visit_some>(self, deserializer: D) -> Result { @@ -400,39 +530,29 @@ impl<'de> Visitor<'de> for SetsVisitor { } fn visit_seq>(self, mut access: A) -> Result { - let mut out = BTreeMap::new(); + let mut out = Loaded::default(); while let Some(item) = access.next_element::()? { - let Some(name) = item.get("name").and_then(Value::as_str) else { continue }; - if !admits(&out, name) { - continue; - } - if let Some(o) = item.get("overrides").and_then(overrides_of) { - out.insert(name.to_string(), o); - } + let name = item.get("name").and_then(Value::as_str).unwrap_or_default(); + out.add(name, item.get("overrides").and_then(overrides_of)); } - Ok(out) + Ok(out.done()) } fn visit_map>(self, mut access: A) -> Result { - let mut out = BTreeMap::new(); + let mut out = Loaded::default(); while let Some(key) = access.next_key::()? { - if admits(&out, &key) { - if let Some(o) = overrides_of(&access.next_value::()?) { - out.insert(key, o); - } - } else { + if out.sets.len() >= MAX_SETS && !out.sets.contains_key(&key) { access.next_value::()?; + out.ignored += 1; + } else { + let overrides = overrides_of(&access.next_value::()?); + out.add(&key, overrides); } } - Ok(out) + Ok(out.done()) } } -/// A valid name, stored as written, with room under [`MAX_SETS`]. -fn admits(sets: &BTreeMap, name: &str) -> bool { - set_name(name).is_some_and(|n| n == name) && (sets.len() < MAX_SETS || sets.contains_key(name)) -} - /// A set's overrides: a list or a map, anything else is no set. fn overrides_of(v: &Value) -> Option { match v { diff --git a/crates/keymap/src/ui/mod.rs b/crates/keymap/src/ui/mod.rs index cf73737ff..d0da34243 100644 --- a/crates/keymap/src/ui/mod.rs +++ b/crates/keymap/src/ui/mod.rs @@ -31,6 +31,7 @@ use state::Env; /// What the dialog reports for a frame. #[derive(Clone, Debug)] +#[non_exhaustive] pub enum Outcome { /// Closed with OK: save `Profiles` (with sets) or `keymap.overrides()`. Ok, @@ -55,6 +56,7 @@ pub enum Outcome { /// What happens when a recorded chord is held by other commands. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[non_exhaustive] pub enum ConflictMode { /// Ask with the conflict bar: Take It, Take It and Go to Conflict, Keep Both, Cancel. #[default] @@ -97,7 +99,9 @@ pub type Accept = Box Result<(), String>>; /// Called for every control once per frame with (automation id, rect, label). pub type OnWidget = Box; -/// How the dialog looks and behaves. [`Options::default`] gives the defaults noted on each field. +/// How the dialog looks and behaves. [`Options::default`] gives the defaults noted on each field; +/// set fields on it, or with [`Options::with`]. +#[non_exhaustive] pub struct Options { /// "Keyboard Shortcuts". pub title: &'static str, @@ -160,6 +164,15 @@ impl Default for Options { } } +impl Options { + /// `self` with `f` applied, for options built in one expression: + /// `Options::default().with(|o| o.keyboard = true)`. + pub fn with(mut self, f: impl FnOnce(&mut Options)) -> Options { + f(&mut self); + self + } +} + /// The format id of craft-keymap's own JSON in [`Outcome::Import`]. pub const JSON_FORMAT: &str = "craft-keymap"; diff --git a/crates/keymap/src/ui/state.rs b/crates/keymap/src/ui/state.rs index 701ba5466..83678353b 100644 --- a/crates/keymap/src/ui/state.rs +++ b/crates/keymap/src/ui/state.rs @@ -1354,21 +1354,21 @@ mod tests { st.new_set(&mut fx.env(), "Mine", None); record(&mut st, &mut fx, add("object.front"), &["F2"]); st.save_set(&mut fx.env()); - let kept = fx.p.sets["Mine"].clone(); + let kept = fx.p.sets()["Mine"].clone(); assert!(!kept.is_empty()); st.switch_set(&mut fx.env(), "Fixture Defaults"); for name in ["Mine", " mine ", "fixture defaults"] { for based_on in [None, Some("Fixture Defaults")] { st.new_set(&mut fx.env(), name, based_on); assert!(st.message.as_ref().is_some_and(|m| m.refused), "{name:?} from {based_on:?}"); - assert_eq!((fx.p.sets.len(), fx.p.active.as_str()), (1, "Fixture Defaults")); - assert_eq!(fx.p.sets["Mine"], kept); + assert_eq!((fx.p.sets().len(), fx.p.active()), (1, "Fixture Defaults")); + assert_eq!(fx.p.sets()["Mine"], kept); } } st.new_set(&mut fx.env(), "Other", None); st.rename_set(&mut fx.env(), "MINE"); assert!(st.message.as_ref().is_some_and(|m| m.refused)); - assert_eq!(fx.p.sets["Mine"], kept); + assert_eq!(fx.p.sets()["Mine"], kept); } #[test] @@ -1379,11 +1379,11 @@ mod tests { let mut st = open(&mut fx); record(&mut st, &mut fx, add("object.front"), &["F2"]); st.new_set(&mut fx.env(), "Other", None); - assert_eq!(fx.p.active, "Other"); + assert_eq!(fx.p.active(), "Other"); st.ask_delete_set(&fx.env()); assert_eq!(st.prompt, Some(Prompt::DeleteSet("Other".into()))); st.answer(&mut fx.env(), Answer::Yes); - assert!(!fx.p.sets.contains_key("Other")); + assert!(!fx.p.sets().contains_key("Other")); st.cancel(&mut fx.env()); assert_eq!((fx.km.overrides(), &fx.p), (&km0, &p0)); } @@ -1397,13 +1397,13 @@ mod tests { st.ask_delete_set(&fx.env()); assert!(st.modal()); st.switch_set(&mut fx.env(), "Mine"); - assert_eq!((st.prompt.clone(), fx.p.active.as_str()), (None, "Mine"), "switching closes the prompt"); + assert_eq!((st.prompt.clone(), fx.p.active()), (None, "Mine"), "switching closes the prompt"); record(&mut st, &mut fx, add("object.front"), &["F2"]); st.prompt = Some(Prompt::DeleteSet("Other".into())); st.answer(&mut fx.env(), Answer::Yes); - assert!(!fx.p.sets.contains_key("Other")); - assert_eq!((fx.p.active.as_str(), fx.keys("object.front")), ("Mine", vec!["F2".to_string()]), "the active set keeps its edits"); + assert!(!fx.p.sets().contains_key("Other")); + assert_eq!((fx.p.active(), fx.keys("object.front")), ("Mine", vec!["F2".to_string()]), "the active set keeps its edits"); assert!(st.can_undo()); } @@ -1419,9 +1419,9 @@ mod tests { st.ask_delete_set(&fx.env()); st.answer(&mut fx.env(), Answer::Yes); assert!(st.message.is_none(), "{:?}", st.message); - assert!(fx.p.sets.is_empty() && fx.km.overrides().is_empty() && !st.can_undo()); + assert!(fx.p.sets().is_empty() && fx.km.overrides().is_empty() && !st.can_undo()); st.ok(&mut fx.env()); - assert!(fx.p.sets.is_empty(), "OK stores no copy of the deleted set"); + assert!(fx.p.sets().is_empty(), "OK stores no copy of the deleted set"); } #[test] @@ -1432,7 +1432,7 @@ mod tests { assert!(DialogState::is_modified(&fx.env())); st.ok(&mut fx.env()); assert!(matches!(st.finish(), Some(Outcome::Ok))); - assert_eq!(fx.p.active, "Custom"); + assert_eq!(fx.p.active(), "Custom"); assert_eq!(fx.p.get("Custom", &fx.b).and_then(|o| o.get("object.front")), Some(&["F2".to_string()][..])); } @@ -1445,7 +1445,7 @@ mod tests { st.switch_set(&mut fx.env(), "Fixture Defaults"); assert!(matches!(&st.prompt, Some(Prompt::SaveChanges { set, .. }) if set == "Mine")); st.answer(&mut fx.env(), Answer::No); - assert_eq!(fx.p.active, "Fixture Defaults"); + assert_eq!(fx.p.active(), "Fixture Defaults"); assert!(fx.km.overrides().is_empty() && !st.can_undo(), "the set loads with a fresh undo history"); assert!(fx.p.get("Mine", &fx.b).is_some_and(Overrides::is_empty), "Don't Save keeps the stored set"); } @@ -1466,7 +1466,7 @@ mod tests { let options = ImportOptions { contexts: vec![("Default".into(), ContextRule::Home)], ..ImportOptions::default() }; st.review_import(&mut fx.env(), set, options); st.import_reviewed(&mut fx.env()); - assert_eq!(fx.p.active, "Theirs"); + assert_eq!(fx.p.active(), "Theirs"); assert_eq!(fx.p.get("Theirs", &fx.b).and_then(|o| o.get("object.front")), None); assert!(fx.p.get("Mine", &fx.b).is_some_and(Overrides::is_empty)); } @@ -1550,7 +1550,7 @@ mod tests { assert_eq!(r.map(|r| (r.name.as_str(), r.plan.commands.len(), r.plan.report.unmatched.len())), Some(("Theirs", 1, 1))); assert!(fx.km.overrides().is_empty(), "review changes nothing"); st.import_reviewed(&mut fx.env()); - assert_eq!(fx.p.active, "Theirs"); + assert_eq!(fx.p.active(), "Theirs"); assert_eq!(fx.keys("file.save"), ["Cmd+Alt+S"]); assert!(st.review.as_ref().is_some_and(|r| r.done.is_some())); st.escape(&mut fx.env()); diff --git a/crates/keymap/tests/profiles.rs b/crates/keymap/tests/profiles.rs index 348420df0..05183b11a 100644 --- a/crates/keymap/tests/profiles.rs +++ b/crates/keymap/tests/profiles.rs @@ -1,6 +1,6 @@ use craft_keymap::{ Bindable, Binding, CommandRef, ContextRule, Flavor, ForeignEntry, ForeignKey, ForeignSet, ImportOptions, Keymap, MAX_SETS, Overrides, Platform, - Policy, ProfileError, Profiles, Scope, Scopes, Start, plan, + Policy, ProfileError, SetProblem, Profiles, Scope, Scopes, Start, plan, }; use serde_json::json; @@ -46,7 +46,7 @@ fn profiles_copy_built_ins_on_edit_and_round_trip_as_json() { assert_eq!(p.delete("Classic", &builtins), Err(ProfileError::BuiltIn)); assert_eq!(p.rename("Custom 3", "classic", &builtins), Err(ProfileError::NameTaken)); assert_eq!(p.rename("Custom 3", "Mine", &builtins), Ok("Mine".into())); - assert_eq!(p.active, "Mine"); + assert_eq!(p.active(), "Mine"); assert_eq!(p.save_as(&"x".repeat(65), Overrides::new(), &builtins), Err(ProfileError::BadName)); p.delete("Mine", &builtins).unwrap(); assert_eq!(p.active_name(&builtins), "Craft"); @@ -59,10 +59,10 @@ fn profiles_copy_built_ins_on_edit_and_round_trip_as_json() { let text = serde_json::to_string(&p).unwrap(); assert_eq!(serde_json::from_str::(&text).unwrap(), p); let junk: Profiles = serde_json::from_str(r#"{"active": 5, "sets": {"": {}, "ok": {"a": ["A"]}, "bad": 3}}"#).unwrap(); - assert_eq!(junk.sets.keys().collect::>(), ["ok"]); + assert_eq!(junk.sets().keys().collect::>(), ["ok"]); let junk: Profiles = serde_json::from_str(r#"{"sets": [{"name": "ok", "overrides": [{"command": "a", "keys": ["A"]}, 4]}, {"name": ""}, 7]}"#).unwrap(); - assert_eq!(junk.sets["ok"].get("a"), Some(&["A".to_string()][..])); + assert_eq!(junk.sets()["ok"].get("a"), Some(&["A".to_string()][..])); assert!(text.contains(r#""overrides":[{"command":"edit.copy","keys":[]}]"#), "{text}"); } @@ -71,7 +71,7 @@ fn rename_keeps_a_set_whose_name_differs_only_in_case() { let builtins = [("Craft", Overrides::new())]; let mut p: Profiles = serde_json::from_str(r#"{"sets": {"Mine": {"a": "A"}, "mine": {"b": "B"}}}"#).unwrap(); assert_eq!(p.rename("Mine", "mine", &builtins), Err(ProfileError::NameTaken)); - assert_eq!(p.sets["mine"].get("b"), Some(&["B".to_string()][..])); + assert_eq!(p.sets()["mine"].get("b"), Some(&["B".to_string()][..])); assert_eq!(p.rename("Mine", "MINE", &builtins), Err(ProfileError::NameTaken)); p.delete("mine", &builtins).unwrap(); assert_eq!(p.rename("Mine", "MINE", &builtins), Ok("MINE".into())); @@ -81,8 +81,8 @@ fn rename_keeps_a_set_whose_name_differs_only_in_case() { fn sets_that_are_not_a_list_or_map_load_as_none() { for text in [r#"{"active": "Mine", "sets": 5}"#, r#"{"active": "Mine", "sets": null}"#, r#"{"active": "Mine", "sets": "x"}"#] { let p: Profiles = serde_json::from_str(text).unwrap_or_else(|e| panic!("{text}: {e}")); - assert!(p.sets.is_empty(), "{text}"); - assert_eq!(p.active, "Mine", "{text}"); + assert!(p.sets().is_empty(), "{text}"); + assert_eq!(p.active(), "Mine", "{text}"); } } @@ -107,9 +107,7 @@ fn a_user_set_named_like_a_new_built_in_keeps_its_edits() { assert_eq!(p.get("Classic", &builtins), Some(&classic)); assert_eq!(builtins[1].1, classic); - let mut q = Profiles::default(); - q.sets.insert("CRAFT".into(), mine.clone()); - q.active = "CRAFT".into(); + let mut q: Profiles = serde_json::from_value(json!({"active": "CRAFT", "sets": [{"name": "CRAFT", "overrides": {"edit.copy": "Cmd+K"}}]})).unwrap(); assert!(q.reconcile(&builtins)); assert_eq!(q.active_name(&builtins), "CRAFT 2"); assert_eq!(q.active_overrides(&builtins), &mine); @@ -128,7 +126,7 @@ fn an_exported_set_imports_as_a_new_set_with_the_same_keymap() { let text = p.export("Mine", &builtins).unwrap(); assert_eq!(p.import(&text, &builtins), Ok("Mine 2".into())); - assert_eq!(p.active, "Mine 2"); + assert_eq!(p.active(), "Mine 2"); let mut again = keymap(); again.set_overrides(p.active_overrides(&builtins).clone()); @@ -146,14 +144,14 @@ fn duplicate_copies_built_in_and_user_sets() { let mut p = Profiles::default(); assert_eq!(p.duplicate("Classic", "Classic", &builtins), Ok("Classic 2".into())); - assert_eq!(p.active, "Classic 2"); + assert_eq!(p.active(), "Classic 2"); assert_eq!(p.active_overrides(&builtins), &classic); assert_eq!(p.duplicate("Classic 2", " Mine ", &builtins), Ok("Mine".into())); assert_eq!(p.duplicate("Mine", "mine", &builtins), Ok("mine 2".into()), "names compare without case"); assert_eq!(p.duplicate("Nope", "X", &builtins), Err(ProfileError::NotFound)); assert_eq!(p.duplicate("Craft", "", &builtins), Err(ProfileError::BadName)); - while p.sets.len() < MAX_SETS { + while p.sets().len() < MAX_SETS { p.duplicate("Craft", "Spare", &builtins).unwrap(); } assert_eq!(p.duplicate("Craft", "Spare", &builtins), Err(ProfileError::TooMany)); @@ -173,7 +171,7 @@ fn a_foreign_set_imports_as_a_new_set_and_leaves_the_keymap_alone() { assert_eq!(name, "Other App"); assert_eq!(report.changed.len(), 1); assert!(km.overrides().is_empty()); - assert_eq!(p.active, "Other App"); + assert_eq!(p.active(), "Other App"); let mut imported = keymap(); imported.set_overrides(p.active_overrides(&builtins).clone()); assert_eq!(imported.bindings(&"file.save".into()), [b("Cmd+Shift+S")]); @@ -181,3 +179,31 @@ fn a_foreign_set_imports_as_a_new_set_and_leaves_the_keymap_alone() { let (name, _) = p.import_plan(&planned, &km, Some("Craft"), &builtins).unwrap(); assert_eq!(name, "Craft 2", "a built-in's name is not reused"); } + +#[test] +fn loading_lists_the_sets_it_drops_and_the_store_has_a_version() { + let text = r#"{"version": 2, "active": "ok", "sets": [ + {"name": "ok", "overrides": {}}, {"name": " pad ", "overrides": {}}, {"name": "ok", "overrides": {"a": "A"}}, {"name": "bad", "overrides": 3} + ]}"#; + let p: Profiles = serde_json::from_str(text).unwrap(); + assert_eq!(p.sets().keys().collect::>(), ["ok"]); + assert!(p.sets()["ok"].is_empty(), "the first of two sets with one name stays"); + let name = |n: &str| n.to_string(); + assert_eq!( + p.problems(), + [ + SetProblem::NewerVersion(2), + SetProblem::BadName { name: name(" pad ") }, + SetProblem::Duplicate { name: name("ok") }, + SetProblem::BadOverrides { name: name("bad") } + ] + ); + + let many: Vec<_> = (0..MAX_SETS + 3).map(|i| json!({"name": format!("s{i}"), "overrides": {}})).collect(); + let p: Profiles = serde_json::from_value(json!({ "sets": many })).unwrap(); + assert_eq!((p.sets().len(), p.problems()), (MAX_SETS, &[SetProblem::TooMany { ignored: 3 }][..])); + + assert_eq!(serde_json::to_value(&p).unwrap()["version"], 1); + let unversioned: Profiles = serde_json::from_str(r#"{"active": "x", "sets": []}"#).unwrap(); + assert!(unversioned.problems().is_empty()); +} diff --git a/crates/keymap/tests/settings.rs b/crates/keymap/tests/settings.rs index ac76296e5..2361693e9 100644 --- a/crates/keymap/tests/settings.rs +++ b/crates/keymap/tests/settings.rs @@ -77,7 +77,7 @@ fn overrides_round_trip_through_craft_settings() { let problems = km.problems(); assert!(problems.contains(&Problem::Dropped { key: "edit.cut".into(), reason: DropReason::NotAList }), "{problems:?}"); assert!(problems.iter().any(|p| matches!(p, Problem::BadBinding { text, .. } if text == "Cmd+Nope")), "{problems:?}"); - assert_eq!(store.get().profiles.sets.len(), 2); + assert_eq!(store.get().profiles.sets().len(), 2); store.update(|p| p.general.language = "de".into()); store.flush().unwrap(); @@ -101,8 +101,8 @@ fn overrides_round_trip_through_craft_settings() { assert!(!line.contains("window.panel") && !text.contains("Spare"), "{text}"); let reloaded = open(tmp.path()); assert!(keymap(reloaded.get().shortcuts.clone()).bindings(&layers).is_empty()); - assert_eq!(reloaded.get().profiles.sets.keys().collect::>(), ["Mine"]); - assert_eq!(reloaded.get().profiles.sets["Mine"].get("text.bold"), Some(&["Text: Cmd+B".to_string()][..])); + assert_eq!(reloaded.get().profiles.sets().keys().collect::>(), ["Mine"]); + assert_eq!(reloaded.get().profiles.sets()["Mine"].get("text.bold"), Some(&["Text: Cmd+B".to_string()][..])); } #[test] @@ -140,7 +140,7 @@ fn a_sets_value_that_is_not_a_list_keeps_the_rest_of_the_profiles() { let text = format!("schema_version = 1\n\n[profiles]\nactive = \"Mine\"\nsets = {value}\n"); std::fs::write(tmp.path().join("config.toml"), text).unwrap(); let store = open(tmp.path()); - assert_eq!(store.get().profiles.active, "Mine", "{value}"); - assert!(store.get().profiles.sets.is_empty(), "{value}"); + assert_eq!(store.get().profiles.active(), "Mine", "{value}"); + assert!(store.get().profiles.sets().is_empty(), "{value}"); } } diff --git a/crates/keymap/tests/ui_dialog.rs b/crates/keymap/tests/ui_dialog.rs index 702ab84f6..52d6ee78d 100644 --- a/crates/keymap/tests/ui_dialog.rs +++ b/crates/keymap/tests/ui_dialog.rs @@ -648,17 +648,17 @@ fn cancel_restores_keymap_and_sets() { let mut h = with_sets(p); click(&mut h, "Set:"); click_last(&mut h, "Mine"); - assert_eq!(profiles(&h).active, "Mine"); + assert_eq!(profiles(&h).active(), "Mine"); click(&mut h, "Save: ⌘S"); press(&mut h, Modifiers::NONE, EguiKey::F2); assert!(has(&h, "(modified)")); new_set(&mut h, "Other"); - assert_eq!(profiles(&h).active, "Other"); + assert_eq!(profiles(&h).active(), "Other"); assert_eq!(h.state().bindings("file.save"), ["F2"], "based on the current shortcuts"); click(&mut h, "Delete"); assert!(has(&h, "Delete the set “Other”?")); click_last(&mut h, "Delete"); - assert!(!profiles(&h).sets.contains_key("Other")); + assert!(!profiles(&h).sets().contains_key("Other")); click(&mut h, "Cancel"); assert!(h.state().canceled()); assert_eq!(profiles(&h), &before); @@ -692,8 +692,8 @@ fn ok_stores_edits_of_a_builtin_as_a_copy() { click(&mut h, "OK"); assert!(h.state().outcomes.iter().any(|o| matches!(o, Outcome::Ok))); let p = profiles(&h); - assert_eq!(p.active, "Custom"); - assert_eq!(p.sets.get("Custom").and_then(|o| o.get("file.save")), Some(&["F2".to_string()][..])); + assert_eq!(p.active(), "Custom"); + assert_eq!(p.sets().get("Custom").and_then(|o| o.get("file.save")), Some(&["F2".to_string()][..])); } #[test] @@ -717,7 +717,7 @@ fn set_export_import_round_trip() { let (d, state) = dialog(fresh.state_mut()); assert_eq!(d.import_json(state, &json), Ok("Name 2".into())); fresh.run(); - assert_eq!(profiles(&fresh).active, "Name 2"); + assert_eq!(profiles(&fresh).active(), "Name 2"); assert_eq!(fresh.state().km.overrides(), &edited); assert_eq!(fresh.get_by_label("Set:").accesskit_node().value().as_deref(), Some("Name 2"), "the picker shows it"); } @@ -756,7 +756,7 @@ fn foreign_import_review_lists_skipped_and_unreadable() { assert!(h.query_by_label_contains("numeric keypad key read as main key").is_some()); assert!(h.state().km.overrides().is_empty(), "the review changes nothing"); click(&mut h, "Import as New Set"); - assert_eq!(profiles(&h).active, "Theirs"); + assert_eq!(profiles(&h).active(), "Theirs"); assert_eq!(h.state().bindings("file.save"), ["Cmd+Alt+S"]); assert!(has(&h, "Imported as “Theirs”")); click(&mut h, "Done"); diff --git a/crates/keymap/tests/ui_snapshot.rs b/crates/keymap/tests/ui_snapshot.rs index a6176af78..9b90383a9 100644 --- a/crates/keymap/tests/ui_snapshot.rs +++ b/crates/keymap/tests/ui_snapshot.rs @@ -230,7 +230,7 @@ fn dialog_screenshots() { } (p, builtins) }); - let options = Options { keyboard: case.keyboard, ..Options::default() }; + let options = Options::default().with(|o| o.keyboard = case.keyboard); let km = if case.long { with_long_label(fixture(case.platform)) } else { fixture(case.platform) }; let text: Box = match case.words { Words::English => Box::new(English),