diff --git a/crates/engine/src/preset_store.rs b/crates/engine/src/preset_store.rs index 8982cc9e..02138b63 100644 --- a/crates/engine/src/preset_store.rs +++ b/crates/engine/src/preset_store.rs @@ -538,11 +538,15 @@ pub struct PresetStore { synced_rev: Option, /// Group file name → its last written content. groups: HashMap, + /// Group files that failed to load: left on disk untouched, so a sync never writes them. + skipped: HashSet, /// Tip hash → file size, for every tip file the store owns. tips: HashMap, index: Option, /// [`crate::actions_cmds::ActionState::rev`] last written (or loaded). actions_rev: u64, + /// `actions.json` exists but failed to load: left on disk untouched, so it is never written. + actions_skipped: bool, warnings: Vec, /// Full tips loaded for brushes in use (behind a lock so `&Session` paths can load). cache: Mutex, @@ -597,6 +601,7 @@ pub fn open(backend: Box) -> Opened { // Parse the group files. let mut parsed: Vec<(String, u64, GroupFile)> = Vec::new(); + let mut skipped = HashSet::new(); let mut failed = false; for (name, size) in group_files { let r = backend.read(&name, MAX_GROUP_BYTES).and_then(|b| serde_json::from_slice::(&b).map_err(|e| e.to_string())).and_then(|g| { @@ -615,6 +620,7 @@ pub fn open(backend: Box) -> Opened { Err(e) => { failed = true; warnings.push(format!("brush presets: {name} skipped: {e}")); + skipped.insert(name); } } } @@ -632,14 +638,18 @@ pub fn open(backend: Box) -> Opened { } let (tiles, regenerated) = load_tiles(backend.as_ref(), &wanted, &tip_sizes, &info, &mut warnings); - let actions = load_actions(backend.as_ref(), &mut warnings); + let loaded_actions = load_actions(backend.as_ref(), &mut warnings); + let actions_skipped = loaded_actions.is_none(); + let actions = loaded_actions.unwrap_or_default(); let mut store = PresetStore { backend, synced_rev: None, groups: HashMap::new(), + skipped: HashSet::new(), tips: HashMap::new(), index: None, actions_rev: 0, + actions_skipped, warnings: Vec::new(), cache: Mutex::new(TipCache { limit: TIP_CACHE_BYTES, ..Default::default() }), }; @@ -668,6 +678,7 @@ pub fn open(backend: Box) -> Opened { // Left on disk untouched (a sync never deletes a file it did not load). failed = true; warnings.push(format!("brush presets: {file} skipped: {missing} preset(s) have missing or unreadable tips")); + skipped.insert(file); continue; } for h in &hashes { @@ -678,6 +689,7 @@ pub fn open(backend: Box) -> Opened { presets.extend(items.iter().cloned()); store.groups.insert(file, Synced { presets: items, tips: hashes, bytes: size, stale }); } + store.skipped = skipped; // Tip files nothing references are garbage (e.g. left by a crash), unless a group failed to // load: its tips must survive so a fixed file still works. if !failed { @@ -692,24 +704,25 @@ pub fn open(backend: Box) -> Opened { } /// `actions.json`: a missing file is an empty list. Anything else unreadable is a warning. -fn load_actions(backend: &dyn PresetBackend, warnings: &mut Vec) -> Vec { +/// The saved actions; `None` when `actions.json` exists but could not be loaded. +fn load_actions(backend: &dyn PresetBackend, warnings: &mut Vec) -> Option> { let bytes = match backend.read(ACTIONS_FILE, MAX_ACTIONS_BYTES) { Ok(b) => b, - Err(e) if missing_file(&e) => return Vec::new(), + Err(e) if missing_file(&e) => return Some(Vec::new()), Err(e) => { warnings.push(format!("actions: {ACTIONS_FILE} skipped: {e}")); - return Vec::new(); + return None; } }; match serde_json::from_slice::(&bytes) { - Ok(file) if file.version == 1 => file.actions, + Ok(file) if file.version == 1 => Some(file.actions), Ok(file) => { warnings.push(format!("actions: {ACTIONS_FILE} skipped: unsupported version {}", file.version)); - Vec::new() + None } Err(e) => { warnings.push(format!("actions: {ACTIONS_FILE} skipped: {e}")); - Vec::new() + None } } } @@ -873,6 +886,13 @@ impl PresetStore { if unchanged { continue; } + if self.skipped.contains(&file) { + self.warnings.push(format!( + "Couldn't save brush presets `{}`: {file} could not be read, so it was left unchanged", + if group.is_empty() { "(ungrouped)" } else { group } + )); + continue; + } let mut items: Vec<&mut BrushPreset> = presets.iter_mut().filter(|p| !p.builtin && p.group == *group).collect(); if let Err(e) = self.write_group(&file, group, &mut items) { self.warnings.push(format!("Couldn't save brush presets `{}`: {e}", if group.is_empty() { "(ungrouped)" } else { group })); @@ -1013,6 +1033,11 @@ impl PresetStore { if self.actions_rev == rev { return; } + if self.actions_skipped { + self.warnings.push(format!("Couldn't save actions: {ACTIONS_FILE} could not be read, so it was left unchanged")); + self.actions_rev = rev; + return; + } let file = crate::actions_cmds::ActionsFile { version: 1, actions: list.to_vec() }; let bytes = match serde_json::to_vec_pretty(&file) { Ok(b) => b, diff --git a/crates/engine/src/preset_store/tests.rs b/crates/engine/src/preset_store/tests.rs index b6017781..b7f17586 100644 --- a/crates/engine/src/preset_store/tests.rs +++ b/crates/engine/src/preset_store/tests.rs @@ -623,3 +623,33 @@ fn imported_abr_folders_persist_and_old_stores_load_flat() { assert_eq!(photocraft_paint::presets::find(&d.tools.presets, "Deep").unwrap().folder.len(), crate::brush_preset_cmds::MAX_FOLDER_DEPTH); assert!(photocraft_paint::presets::find(&d.tools.presets, "Blank").unwrap().folder.is_empty()); } + +#[test] +fn a_group_file_that_failed_to_load_is_not_overwritten() { + // e.g. written by a newer PhotoCraft: skipped on load, so a save must not replace it. + let mem = MemBackend::default(); + let file = group_file_name(""); + let newer = br#"{"format":"photocraft-brush-group","version":2,"group":"","presets":[]}"#.to_vec(); + mem.files.lock().unwrap().insert(file.clone(), newer.clone()); + let mut s = Session::new(); + s.attach_preset_store(open(Box::new(mem.clone()))); + s.tools.brush.tip = TipShape::Sampled(tip8(4, 4, 0)); + s.execute("brush.presets.save", json!({"name": "New"})).unwrap(); + assert_eq!(mem.files.lock().unwrap().get(&file), Some(&newer), "a file the store could not read was overwritten"); + assert!(!s.preset_store.as_mut().unwrap().take_warnings().is_empty()); +} + +#[test] +fn an_actions_file_that_failed_to_load_is_not_overwritten() { + // e.g. written by a newer PhotoCraft: skipped on load, so recording an action must not replace it. + let mem = MemBackend::default(); + let newer = br#"{"version":2,"actions":[{"name":"Keep","steps":[]}]}"#.to_vec(); + mem.files.lock().unwrap().insert(ACTIONS_FILE.into(), newer.clone()); + let mut s = Session::new(); + s.attach_preset_store(open(Box::new(mem.clone()))); + s.execute("actions.record", json!({"name": "New"})).unwrap(); + s.execute("actions.stop", json!({})).unwrap(); + s.sync_preset_store(); + assert_eq!(mem.files.lock().unwrap().get(ACTIONS_FILE), Some(&newer), "an actions file the store could not read was overwritten"); + assert!(!s.preset_store.as_mut().unwrap().take_warnings().is_empty()); +}