diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index f99e63036..7e3a860a3 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -133,22 +133,46 @@ impl Session { } /// Write the saved preset `preset` to the store, which held it as `old_name` when that is set - /// (a rename). Without a store, nothing happens. + /// (a rename). A rename writes the new file first and removes the old one only when that write + /// succeeded: after a failed write the store still holds the preset under its old name. When + /// the two names differ only in case, the old file is written and then renamed to the new name; + /// when that rename fails, the old file gets its content back. Without a store, nothing happens. pub(crate) fn store_pdf_preset(&self, old_name: Option<&str>, preset: &PdfPreset) { let Some(dir) = &self.pdf_preset_dir else { return }; let mut clean = preset.clone(); clean.settings.clear_passwords(); let file = format!("{}.{PRESET_FORMAT}", file_stem(&clean.name)); - if let Some(old) = old_name.map(|o| format!("{}.{PRESET_FORMAT}", file_stem(o))) - && old != file - { - take(dir, &old); - } - match serde_json::to_vec_pretty(&PresetFile::new(vec![clean])) { - Ok(bytes) => { + let bytes = match serde_json::to_vec_pretty(&PresetFile::new(vec![clean])) { + Ok(bytes) => bytes, + Err(e) => { + log::warn!("can't save the PDF preset `{}`: {e}", preset.name); + return; + } + }; + match old_name.map(|o| format!("{}.{PRESET_FORMAT}", file_stem(o))).filter(|old| *old != file) { + // Names equal in any case get file names that differ only in case. A file system that + // ignores case holds them as one file, which removing the old name would delete. + Some(old) if old.to_lowercase() == file.to_lowercase() => match read_capped(&dir.join(&old), MAX_PRESET_FILE) { + Ok(before) => { + if put(dir, &old, &bytes) + && let Err(e) = std::fs::rename(dir.join(&old), dir.join(&file)) + { + log::warn!("can't rename {} to {file}: {e}", dir.join(&old).display()); + put(dir, &old, &before); + } + } + Err(_) => { + put(dir, &file, &bytes); + } + }, + Some(old) => { + if put(dir, &file, &bytes) { + take(dir, &old); + } + } + None => { put(dir, &file, &bytes); } - Err(e) => log::warn!("can't save the PDF preset `{}`: {e}", preset.name), } } diff --git a/crates/engine/src/tests_store.rs b/crates/engine/src/tests_store.rs index f630d78eb..37eafe6e4 100644 --- a/crates/engine/src/tests_store.rs +++ b/crates/engine/src/tests_store.rs @@ -40,6 +40,82 @@ fn saved_presets_come_back_in_the_next_session_and_renames_and_deletes_follow() assert!(!dir.join(format!("{}.vcpdfpresets", file_stem("Old"))).exists()); } +#[test] +fn a_rename_that_cannot_be_written_keeps_the_preset_under_its_old_name() { + let dir = fresh_dir("rename-fails").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap(); + // The test puts a folder where the new name's file goes, so the write fails as it would on a full disk. + std::fs::create_dir(dir.join(format!("{}.vcpdfpresets", file_stem("Screen")))).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "Screen"})).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["Web"]); + assert_eq!(b.prefs.pdf_presets[0].description, "for screens"); + assert!(dir.join(format!("{}.vcpdfpresets", file_stem("Web"))).is_file()); +} + +#[test] +fn a_rename_that_only_changes_case_keeps_one_file() { + let dir = fresh_dir("rename-case").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web"})).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB", "description": "renamed"})).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["WEB"]); + assert_eq!(b.prefs.pdf_presets[0].description, "renamed"); + assert_eq!(file_names(&dir), [format!("{}.vcpdfpresets", file_stem("WEB"))]); +} + +fn file_names(dir: &std::path::Path) -> Vec { + std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect() +} + +/// Windows does not allow replacing a read-only file or writing into it, so the write fails. +#[cfg(windows)] +#[test] +fn a_rename_that_only_changes_case_and_cannot_be_written_keeps_the_old_file() { + let dir = fresh_dir("rename-case-fails").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap(); + let old = dir.join(format!("{}.vcpdfpresets", file_stem("Web"))); + let writable = std::fs::metadata(&old).unwrap().permissions(); + let mut read_only = writable.clone(); + read_only.set_readonly(true); + std::fs::set_permissions(&old, read_only).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB"})).unwrap(); + std::fs::set_permissions(&old, writable).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["Web"]); + assert_eq!(b.prefs.pdf_presets[0].description, "for screens"); + assert_eq!(file_names(&dir), [format!("{}.vcpdfpresets", file_stem("Web"))]); +} + +/// Where file names keep case apart, a folder named like the new file makes the rename fail. +#[cfg(not(any(windows, target_vendor = "apple")))] +#[test] +fn a_rename_that_only_changes_case_and_cannot_be_moved_keeps_the_old_preset() { + let dir = fresh_dir("rename-case-blocked").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap(); + std::fs::create_dir(dir.join(format!("{}.vcpdfpresets", file_stem("WEB")))).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB", "description": "renamed"})).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["Web"]); + assert_eq!(b.prefs.pdf_presets[0].description, "for screens"); +} + #[test] fn a_folder_that_does_not_exist_yet_takes_the_presets_the_preferences_held() { let dir = fresh_dir("seed").join("PDF Presets");