From 284f569264e886cbd5447a520a7d018043f3cdfe Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Sun, 25 Oct 2026 08:18:23 -0600 Subject: [PATCH] fix(engine): defined and imported patterns survive a restart (#9317) (#9318) * fix(engine): defined and imported patterns survive a restart (#9317) The pattern library lived only in memory: patterns defined with Edit > Define Pattern or imported from a .pat file were gone after a restart, while their Patterns panel folders (saved with the preferences) still named them. Photoshop keeps them. The preset store now writes the user patterns (the library minus the built-ins, in order) to patterns.pat, Photoshop's own pattern file format, whenever their ids, names, sizes or order change, and loads them when the store attaches. Like actions.json, a file that fails to load is warned about and never overwritten. The web store accepts the new record. Co-Authored-By: Claude Opus 5.5 * Patterns: persist same-id pixel replacements, retry a failed removal CodeRabbit (discussion_r4369506324, r4369506326): - an imported .pat whose id is already in the library replaces its pixels, which the id/name/size key didn't show, so the old pixels came back after a restart; PatternLibrary::pixels_rev is bumped on such a replacement and is part of the key; - a failed patterns.pat removal recorded the empty library as saved and was never retried, so the deleted pattern came back; it now stays pending, as a failed write does. Tests: a_same_id_pattern_replacement_survives_a_restart, a_failed_patterns_removal_is_retried. Co-Authored-By: Claude Opus 5.5 * Patterns: persist replaced built-ins and ASL replacements CodeRabbit (discussion_r4369536978, r4369536981): - a .pat import that replaced a built-in pattern by id was filtered out as a built-in and never saved; only built-ins exactly as shipped (is_pristine_builtin) are skipped now, and on load a saved built-in takes the shipped one's place unless it changed before attachment; - style.presets.importAsl replaced patterns by id without bumping pixels_rev; both imports now go through PatternLibrary::upsert. Tests: an_imported_builtin_replacement_survives_a_restart, an_asl_pattern_replacement_bumps_pixels_rev. Co-Authored-By: Claude Opus 5.5 * .pat: refuse names and counts the reader won't load, on write too CodeRabbit (review 5641404908, outside-diff, two findings): now that the preset store writes the user patterns, a file its own loader rejects or truncates loses patterns at the next start. - a name of 65,536+ UTF-16 units (with its NUL) was written although the reader refuses it; write_pattern refuses it now; - more than 100,000 patterns were written, and the reader kept the first 100,000 without a word; both now refuse counts over MAX_PAT_PATTERNS. Tests: a_name_the_reader_refuses_is_refused_on_write, more_patterns_than_a_pat_file_holds_are_refused. Co-Authored-By: Claude Opus 5.5 * Patterns: an early same-id import with other pixels is saved CodeRabbit (review 5641454449, outside-diff, preset_store.rs:1191-1202): a pattern imported before the store finished loading, with the id, name and size of a saved one but other pixels, was appended (no pixels_rev bump), won the merge, and then matched the loaded file's key, so it was never written and a restart brought the old pixels back. The merge now bumps pixels_rev when the copy it keeps differs from the saved one. Test: an_early_same_id_import_wins_and_is_saved. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- apps/photocraft-web/src/preset_bridge.rs | 5 +- crates/engine/src/pattern_cmds.rs | 29 +++- crates/engine/src/preset_store.rs | 121 +++++++++++++++- crates/engine/src/preset_store/tests.rs | 167 +++++++++++++++++++++++ crates/engine/src/style_asl_cmds.rs | 23 +++- crates/psd/src/patterns.rs | 39 +++++- 6 files changed, 368 insertions(+), 16 deletions(-) diff --git a/apps/photocraft-web/src/preset_bridge.rs b/apps/photocraft-web/src/preset_bridge.rs index a7445bbc14..a39fdfb4b4 100644 --- a/apps/photocraft-web/src/preset_bridge.rs +++ b/apps/photocraft-web/src/preset_bridge.rs @@ -9,8 +9,8 @@ use photocraft_engine::{ }; pub type Batch = BTreeMap>>; -/// Three sampled images per preset (tip, dual tip, texture), plus group/index/action records. -pub const MAX_RECORDS: usize = MAX_GROUPS + MAX_PRESETS_PER_GROUP * 3 + 2; +/// Three sampled images per preset (tip, dual tip, texture), plus group/index/action/pattern records. +pub const MAX_RECORDS: usize = MAX_GROUPS + MAX_PRESETS_PER_GROUP * 3 + 3; #[derive(Default)] struct State { @@ -45,6 +45,7 @@ pub fn record_limit(name: &str) -> Result { match name { INDEX_FILE => Ok(MAX_INDEX_BYTES), ACTIONS_FILE => Ok(MAX_ACTIONS_BYTES), + PATTERNS_FILE => Ok(MAX_PATTERNS_BYTES), _ if name.starts_with("tips/") && leaf.ends_with(".pctip") => Ok(MAX_TIP_BYTES), _ if !name.contains('/') && name.ends_with(".pcbrushes") => Ok(MAX_GROUP_BYTES), _ => Err("unknown preset record type".into()), diff --git a/crates/engine/src/pattern_cmds.rs b/crates/engine/src/pattern_cmds.rs index 95163d39f5..5aab2887f0 100644 --- a/crates/engine/src/pattern_cmds.rs +++ b/crates/engine/src/pattern_cmds.rs @@ -22,14 +22,36 @@ use crate::{EngineError, Result, Session}; #[derive(Clone, Debug)] pub struct PatternLibrary { pub items: Vec, + /// Bumped when a pattern's pixels are replaced under the same id (an imported `.pat` whose + /// id is already in the library), which its id, name and size don't show. + pub pixels_rev: u64, } impl Default for PatternLibrary { fn default() -> Self { - PatternLibrary { items: builtin() } + PatternLibrary { items: builtin(), pixels_rev: 0 } } } +impl PatternLibrary { + /// Add `pat`, or replace the pattern with its id (marking the pixels changed). + pub fn upsert(&mut self, pat: Pattern) { + match self.items.iter_mut().find(|q| q.id == pat.id) { + Some(q) => { + *q = pat; + self.pixels_rev += 1; + } + None => self.items.push(pat), + } + } +} + +/// Whether `p` is a built-in pattern as shipped (same id, name, size and pixels): those are never +/// saved. A built-in replaced by an import, or renamed, is the user's and is. +pub(crate) fn is_pristine_builtin(p: &Pattern, builtins: &[Pattern]) -> bool { + builtins.iter().any(|b| b.id == p.id && b.name == p.name && (b.width, b.height) == (p.width, p.height) && b.surface == p.surface) +} + fn hash01(x: u32, y: u32, seed: u32) -> f32 { let mut h = x.wrapping_mul(0x8da6_b343) ^ y.wrapping_mul(0xd816_3841) ^ seed.wrapping_mul(0xcb1a_b31f); h ^= h >> 13; @@ -278,10 +300,7 @@ fn import(s: &mut Session, p: &Value) -> Result { let mut ids = Vec::new(); for pat in pats { ids.push(pat.id.clone()); - match s.patterns.items.iter_mut().find(|q| q.id == pat.id) { - Some(q) => *q = pat, - None => s.patterns.items.push(pat), - } + s.patterns.upsert(pat); } Ok(json!({"imported": ids, "count": ids.len()})) } diff --git a/crates/engine/src/preset_store.rs b/crates/engine/src/preset_store.rs index 83cb16a838..a065e48e6d 100644 --- a/crates/engine/src/preset_store.rs +++ b/crates/engine/src/preset_store.rs @@ -38,6 +38,11 @@ //! The Actions list (`actions.json`, see [`crate::actions_cmds`]) lives in the same directory. //! A missing file is an empty list. A corrupt or oversized file is skipped with a warning. //! Headless sessions have no store, so their actions stay in memory. +//! +//! So do the patterns the user defined or imported (`patterns.pat`, Photoshop's own pattern file +//! format, #9317): the whole user library in its order, rewritten when its ids, names or order +//! change. The built-in patterns are regenerated on start and never written. Like `actions.json`, +//! a file that fails to load is warned about and never overwritten. use std::collections::{BTreeMap, HashMap, HashSet}; use std::io::Read; @@ -67,6 +72,10 @@ pub const MAX_INDEX_BYTES: u64 = 4 << 20; pub const ACTIONS_FILE: &str = "actions.json"; /// Largest Actions file read or written. pub const MAX_ACTIONS_BYTES: u64 = 16 << 20; +/// The user's pattern library (`.pat`). +pub const PATTERNS_FILE: &str = "patterns.pat"; +/// Largest pattern library read or written. +pub const MAX_PATTERNS_BYTES: u64 = 256 << 20; /// The whole store never grows beyond this; further groups are skipped with a warning. pub const MAX_STORE_BYTES: u64 = 2 << 30; /// Largest tip side accepted from disk. @@ -550,6 +559,10 @@ pub struct PresetStore { actions_rev: u64, /// `actions.json` exists but failed to load: left on disk untouched, so it is never written. actions_skipped: bool, + /// [`patterns_key`] of the user patterns last written (or loaded). + patterns_key: u64, + /// `patterns.pat` exists but failed to load: left on disk untouched, so it is never written. + patterns_skipped: bool, warnings: Vec, /// Full tips loaded for brushes in use (behind a lock so `&Session` paths can load). cache: Mutex, @@ -568,6 +581,8 @@ pub struct Opened { pub warnings: Vec, /// Recorded actions (`actions.json`). Empty when the file is missing or unreadable. pub actions: Vec, + /// User patterns (`patterns.pat`), in library order. Empty when the file is missing or unreadable. + pub patterns: Vec, } /// Load a store (any thread; the desktop app does this in the background at start). @@ -644,6 +659,9 @@ pub fn open(backend: Box) -> Opened { 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 loaded_patterns = load_patterns(backend.as_ref(), &mut warnings); + let patterns_skipped = loaded_patterns.is_none(); + let patterns = loaded_patterns.unwrap_or_default(); let mut store = PresetStore { backend, synced_rev: None, @@ -653,6 +671,8 @@ pub fn open(backend: Box) -> Opened { index: None, actions_rev: 0, actions_skipped, + patterns_key: patterns_key(&patterns, 0), + patterns_skipped, warnings: Vec::new(), cache: Mutex::new(TipCache { limit: TIP_CACHE_BYTES, ..Default::default() }), }; @@ -710,7 +730,41 @@ pub fn open(backend: Box) -> Opened { let hidden_builtins = index.as_ref().map(|i| i.hidden_builtins.clone()).unwrap_or_default(); let order = index.as_ref().map(|i| i.order.clone()).unwrap_or_default(); store.index = index; - Opened { store, presets, hidden_builtins, order, warnings, actions } + Opened { store, presets, hidden_builtins, order, warnings, actions, patterns } +} + +/// `patterns.pat`: a missing file is no patterns. Anything else unreadable is a warning. +/// The saved patterns; `None` when `patterns.pat` exists but could not be loaded. +fn load_patterns(backend: &dyn PresetBackend, warnings: &mut Vec) -> Option> { + let bytes = match backend.read(PATTERNS_FILE, MAX_PATTERNS_BYTES) { + Ok(b) => b, + Err(e) if missing_file(&e) => return Some(Vec::new()), + Err(e) => { + warnings.push(format!("patterns: {PATTERNS_FILE} skipped: {e}")); + return None; + } + }; + match photocraft_io::pattern_map::read_pat(&bytes) { + Ok(p) => Some(p), + Err(e) => { + warnings.push(format!("patterns: {PATTERNS_FILE} skipped: {e}")); + None + } + } +} + +/// What decides whether the user patterns need rewriting: their ids, names, sizes and order, and +/// the library's [`pixels_rev`](crate::pattern_cmds::PatternLibrary::pixels_rev) (pixels replaced +/// under the same id). +fn patterns_key(patterns: &[photocraft_doc::Pattern], pixels_rev: u64) -> u64 { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + for p in patterns { + (&p.id, &p.name, p.width, p.height).hash(&mut h); + } + patterns.len().hash(&mut h); + pixels_rev.hash(&mut h); + h.finish() } /// `actions.json`: a missing file is an empty list. Anything else unreadable is a warning. @@ -1075,6 +1129,49 @@ impl PresetStore { } } +impl PresetStore { + /// Write the user patterns (the library minus the built-ins) when they changed since the last + /// load or write. An oversized library is warned once per change and not written. An I/O + /// error is warned and retried on the next command. + pub fn sync_patterns(&mut self, library: &[photocraft_doc::Pattern], pixels_rev: u64) { + let builtins = crate::pattern_cmds::builtin(); + let user: Vec = library.iter().filter(|p| !crate::pattern_cmds::is_pristine_builtin(p, &builtins)).cloned().collect(); + let key = patterns_key(&user, pixels_rev); + if self.patterns_key == key { + return; + } + self.patterns_key = key; + if self.patterns_skipped { + self.warnings.push(format!("Couldn't save patterns: {PATTERNS_FILE} could not be read, so it was left unchanged")); + return; + } + if user.is_empty() { + if let Err(e) = self.backend.remove(PATTERNS_FILE) { + self.warnings.push(format!("Couldn't save patterns: {e}")); + // Try again after the next command. + self.patterns_key = key.wrapping_add(1); + } + return; + } + let bytes = match photocraft_io::pattern_map::write_pat(&user) { + Ok(b) => b, + Err(e) => { + self.warnings.push(format!("Couldn't save patterns: {e}")); + return; + } + }; + if bytes.len() as u64 > MAX_PATTERNS_BYTES { + self.warnings.push(format!("patterns: the pattern library is too large to save ({} MB limit)", MAX_PATTERNS_BYTES >> 20)); + return; + } + if let Err(e) = self.backend.write(PATTERNS_FILE, &bytes) { + self.warnings.push(format!("Couldn't save patterns: {e}")); + // Try again after the next command. + self.patterns_key = key.wrapping_add(1); + } + } +} + fn builtin_names() -> &'static [String] { static NAMES: std::sync::OnceLock> = std::sync::OnceLock::new(); NAMES.get_or_init(|| photocraft_paint::presets::builtin().into_iter().map(|p| p.name).collect()) @@ -1091,7 +1188,24 @@ impl Session { /// built-in of the same name; presets created before the store finished loading win), hide /// deleted built-ins, then sync. Returns the load warnings. pub fn attach_preset_store(&mut self, opened: Opened) -> Vec { - let Opened { store, presets, hidden_builtins, order, mut warnings, actions: disk } = opened; + let Opened { store, presets, hidden_builtins, order, mut warnings, actions: disk, patterns } = opened; + // Saved patterns join the library after the built-ins, and a saved built-in (replaced by + // an import or renamed) takes the shipped one's place; a pattern defined or imported + // before the store finished loading keeps its place (and the same id isn't added twice). + let builtins = crate::pattern_cmds::builtin(); + for p in patterns { + match self.patterns.items.iter_mut().find(|q| q.id == p.id) { + Some(q) if crate::pattern_cmds::is_pristine_builtin(q, &builtins) => *q = p, + // The earlier copy wins; other pixels than the saved ones must still be written, + // which its id, name and size don't show. + Some(q) => { + if q.surface != p.surface { + self.patterns.pixels_rev += 1; + } + } + None => self.patterns.items.push(p), + } + } let lib = &mut self.tools.presets; lib.retain(|p| !(p.builtin && hidden_builtins.iter().any(|h| h.eq_ignore_ascii_case(&p.name)))); for p in presets { @@ -1136,7 +1250,7 @@ impl Session { warnings } - /// Write pending brush preset and Actions changes to the attached store (no-op without one). + /// Write pending brush preset, Actions and pattern changes to the attached store (no-op without one). pub fn sync_preset_store(&mut self) { let rev = self.tools.presets_rev; let actions_rev = self.actions.rev; @@ -1151,6 +1265,7 @@ impl Session { } } st.sync_actions(&self.actions.list, actions_rev); + st.sync_patterns(&self.patterns.items, self.patterns.pixels_rev); } } diff --git a/crates/engine/src/preset_store/tests.rs b/crates/engine/src/preset_store/tests.rs index 860ebd4fc3..e0080aa4cb 100644 --- a/crates/engine/src/preset_store/tests.rs +++ b/crates/engine/src/preset_store/tests.rs @@ -703,3 +703,170 @@ fn an_actions_file_that_failed_to_load_is_not_overwritten() { 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()); } + +/// #9317: a defined or imported pattern survives a restart, as in Photoshop; the built-in +/// patterns are never written, and deleting the last user pattern removes the file. +#[test] +fn user_patterns_survive_a_restart() { + let dir = TempDir::new("patterns"); + let builtin = Session::new().patterns.items.len(); + let (mut s, w) = session(&dir); + assert!(w.is_empty(), "{w:?}"); + s.execute("file.new", json!({"width": 12, "height": 10})).unwrap(); + s.execute("edit.fill", json!({"color": "#1ea05a"})).unwrap(); + let id = s.execute("edit.definePattern", json!({"name": "Green Tile"})).unwrap()["pattern"].as_str().unwrap().to_string(); + s.sync_preset_store(); + assert!(dir.files().contains(&PATTERNS_FILE.to_string()), "{:?}", dir.files()); + // Only the user's pattern is in the file. + let bytes = std::fs::read(dir.0.join(PATTERNS_FILE)).unwrap(); + let saved = photocraft_io::pattern_map::read_pat(&bytes).unwrap(); + assert_eq!(saved.iter().map(|p| p.name.as_str()).collect::>(), ["Green Tile"]); + drop(s); + + let (mut t, w) = session(&dir); + assert!(w.is_empty(), "{w:?}"); + assert_eq!(t.patterns.items.len(), builtin + 1); + let back = t.patterns.items.iter().find(|p| p.id == id).expect("the defined pattern is back after a restart"); + assert_eq!((back.name.as_str(), back.width, back.height), ("Green Tile", 12, 10)); + assert_eq!(back.surface, saved[0].surface, "with its pixels"); + // Renames are kept; deleting the last user pattern removes the file. + t.execute("pattern.rename", json!({"pattern": id, "name": "Moss"})).unwrap(); + t.sync_preset_store(); + let (u, _) = session(&dir); + assert!(u.patterns.items.iter().any(|p| p.id == id && p.name == "Moss")); + t.execute("pattern.delete", json!({"pattern": id})).unwrap(); + t.sync_preset_store(); + assert!(!dir.files().contains(&PATTERNS_FILE.to_string()), "{:?}", dir.files()); +} + +#[test] +fn a_patterns_file_that_failed_to_load_is_not_overwritten() { + let mem = MemBackend::default(); + let unreadable = b"not a pattern file".to_vec(); + mem.files.lock().unwrap().insert(PATTERNS_FILE.into(), unreadable.clone()); + let mut s = Session::new(); + let w = s.attach_preset_store(open(Box::new(mem.clone()))); + assert!(w.iter().any(|w| w.contains(PATTERNS_FILE)), "{w:?}"); + s.execute("file.new", json!({"width": 4, "height": 4})).unwrap(); + s.execute("edit.definePattern", json!({"name": "New"})).unwrap(); + s.sync_preset_store(); + assert_eq!(mem.files.lock().unwrap().get(PATTERNS_FILE), Some(&unreadable), "a patterns file the store could not read was overwritten"); + assert!(!s.preset_store.as_mut().unwrap().take_warnings().is_empty()); +} + +/// A `.pat` of one 4×4 pattern with id `id`, filled with `rgba`. +fn pat_bytes(id: &str, rgba: [f32; 4]) -> String { + use photocraft_geom::Rect; + use photocraft_raster::Surface; + let mut surface = Surface::new(photocraft_color::PixelFormat::RGBA8); + surface.fill_rect(Rect::new(0, 0, 4, 4), &rgba); + let mut p = photocraft_doc::Pattern::new("Same", surface, 4, 4); + p.id = id.into(); + photocraft_paint::tile::b64_encode(&photocraft_io::pattern_map::write_pat(&[p]).unwrap()) +} + +/// #9318 review: re-importing a pattern under the same id, name and size but with other pixels +/// (an edited `.pat`) is written, so a restart doesn't bring the old pixels back. +#[test] +fn a_same_id_pattern_replacement_survives_a_restart() { + let mem = MemBackend::default(); + let mut s = Session::new(); + s.attach_preset_store(open(Box::new(mem.clone()))); + let id = "6f1c2c1e-0000-4000-8000-000000009318"; + s.execute("pattern.import", json!({"data": pat_bytes(id, [1.0, 0.0, 0.0, 1.0])})).unwrap(); + s.sync_preset_store(); + s.execute("pattern.import", json!({"data": pat_bytes(id, [0.0, 0.0, 1.0, 1.0])})).unwrap(); + s.sync_preset_store(); + let mut t = Session::new(); + t.attach_preset_store(open(Box::new(mem.clone()))); + let back = t.patterns.items.iter().find(|p| p.id == id).expect("the pattern is back"); + let want = s.patterns.items.iter().find(|p| p.id == id).unwrap(); + assert_eq!(back.surface, want.surface, "with the replacement's pixels, not the first import's"); +} + +/// A [`MemBackend`] whose removals fail while `fail` is set. +#[derive(Clone, Default)] +struct FlakyRemove { + mem: MemBackend, + fail: std::sync::Arc, +} + +impl PresetBackend for FlakyRemove { + fn list(&self) -> Result, String> { + self.mem.list() + } + fn read(&self, name: &str, max: u64) -> Result, String> { + self.mem.read(name, max) + } + fn write(&self, name: &str, bytes: &[u8]) -> Result<(), String> { + self.mem.write(name, bytes) + } + fn remove(&self, name: &str) -> Result<(), String> { + if self.fail.load(std::sync::atomic::Ordering::SeqCst) { + return Err(format!("{name}: busy")); + } + self.mem.remove(name) + } +} + +/// #9318 review: deleting the last user pattern while `patterns.pat` can't be removed is +/// warned and retried, so the deleted pattern doesn't come back after a restart. +#[test] +fn a_failed_patterns_removal_is_retried() { + let backend = FlakyRemove::default(); + let mut s = Session::new(); + s.attach_preset_store(open(Box::new(backend.clone()))); + s.execute("file.new", json!({"width": 4, "height": 4})).unwrap(); + let id = s.execute("edit.definePattern", json!({"name": "Gone"})).unwrap()["pattern"].as_str().unwrap().to_string(); + s.sync_preset_store(); + assert!(backend.mem.files.lock().unwrap().contains_key(PATTERNS_FILE)); + backend.fail.store(true, std::sync::atomic::Ordering::SeqCst); + s.execute("pattern.delete", json!({"pattern": id})).unwrap(); + s.sync_preset_store(); + assert!(!s.preset_store.as_mut().unwrap().take_warnings().is_empty(), "the failure is warned"); + backend.fail.store(false, std::sync::atomic::Ordering::SeqCst); + s.execute("file.new", json!({"width": 4, "height": 4})).unwrap(); + s.sync_preset_store(); + assert!(!backend.mem.files.lock().unwrap().contains_key(PATTERNS_FILE), "the removal is retried"); +} + +/// #9318 review: an imported `.pat` that replaces a built-in pattern's pixels (same id) is saved +/// and replaces the built-in after a restart, once; untouched built-ins are still never written. +#[test] +fn an_imported_builtin_replacement_survives_a_restart() { + let mem = MemBackend::default(); + let mut s = Session::new(); + s.attach_preset_store(open(Box::new(mem.clone()))); + let builtin = s.patterns.items[0].id.clone(); + s.execute("pattern.import", json!({"data": pat_bytes(&builtin, [0.9, 0.1, 0.4, 1.0])})).unwrap(); + s.sync_preset_store(); + let saved = photocraft_io::pattern_map::read_pat(mem.files.lock().unwrap().get(PATTERNS_FILE).expect("written")).unwrap(); + assert_eq!(saved.iter().map(|p| p.id.as_str()).collect::>(), [builtin.as_str()], "only the replaced built-in"); + let mut t = Session::new(); + t.attach_preset_store(open(Box::new(mem.clone()))); + let back: Vec<_> = t.patterns.items.iter().filter(|p| p.id == builtin).collect(); + assert_eq!(back.len(), 1, "once"); + let want = s.patterns.items.iter().find(|p| p.id == builtin).unwrap(); + assert_eq!(back[0].surface, want.surface, "with the imported pixels"); +} + +/// #9318 review: a pattern imported before the store finished loading, with the id, name and +/// size of a saved one but other pixels, wins the merge and is written, so a restart keeps it. +#[test] +fn an_early_same_id_import_wins_and_is_saved() { + let mem = MemBackend::default(); + let id = "6f1c2c1e-0000-4000-8000-00000000e4a1"; + let mut s = Session::new(); + s.attach_preset_store(open(Box::new(mem.clone()))); + s.execute("pattern.import", json!({"data": pat_bytes(id, [1.0, 0.0, 0.0, 1.0])})).unwrap(); + s.sync_preset_store(); + // Next start: the same pattern with other pixels arrives before the store is attached. + let mut t = Session::new(); + t.execute("pattern.import", json!({"data": pat_bytes(id, [0.0, 1.0, 0.0, 1.0])})).unwrap(); + let early = t.patterns.items.iter().find(|p| p.id == id).unwrap().surface.clone(); + t.attach_preset_store(open(Box::new(mem.clone()))); + t.sync_preset_store(); + let mut u = Session::new(); + u.attach_preset_store(open(Box::new(mem.clone()))); + assert_eq!(u.patterns.items.iter().find(|p| p.id == id).unwrap().surface, early, "the early import's pixels were saved"); +} diff --git a/crates/engine/src/style_asl_cmds.rs b/crates/engine/src/style_asl_cmds.rs index 8d80c816a1..840459bf6d 100644 --- a/crates/engine/src/style_asl_cmds.rs +++ b/crates/engine/src/style_asl_cmds.rs @@ -55,10 +55,7 @@ fn import(s: &mut Session, p: &Value) -> Result { let mut pattern_ids = Vec::new(); for pat in imp.patterns { pattern_ids.push(pat.id.clone()); - match s.patterns.items.iter_mut().find(|q| q.id == pat.id) { - Some(q) => *q = pat, - None => s.patterns.items.push(pat), - } + s.patterns.upsert(pat); } let styles = &mut s.presets.styles; if replace { @@ -241,6 +238,24 @@ mod tests { assert!((f.opacity - l.opacity).abs() < 1e-4); } + /// #9318 review: an ASL that replaces a library pattern by id marks its pixels changed, so the + /// preset store writes the replacement (see `PatternLibrary::pixels_rev`). + #[test] + fn an_asl_pattern_replacement_bumps_pixels_rev() { + let mut s = session(); + let mut pat = s.patterns.items[1].clone(); + pat.id = "asl-replaced-pattern".into(); + let asl = |p: &photocraft_doc::Pattern| { + let style = AslStyle { name: "Pat".into(), effects: vec![], blend: None, opacity: None, fill_opacity: None }; + b64_encode(&asl_map::write_asl(&[style], std::slice::from_ref(p)).unwrap()) + }; + s.execute(IMPORT, json!({"data": asl(&pat), "group": "A"})).unwrap(); + let rev = s.patterns.pixels_rev; + pat.surface = s.patterns.items[2].surface.clone(); + s.execute(IMPORT, json!({"data": asl(&pat), "group": "B"})).unwrap(); + assert!(s.patterns.pixels_rev > rev, "the same-id replacement is marked"); + } + #[test] fn embedded_patterns_reach_the_library_and_the_document() { let mut s = session(); diff --git a/crates/psd/src/patterns.rs b/crates/psd/src/patterns.rs index 15558a700e..85bf285db9 100644 --- a/crates/psd/src/patterns.rs +++ b/crates/psd/src/patterns.rs @@ -391,6 +391,10 @@ fn write_pattern(p: &PsdPattern, out: &mut Vec) -> Result<()> { out.extend_from_slice(&(p.height as u16).to_be_bytes()); out.extend_from_slice(&(p.width as u16).to_be_bytes()); let units: Vec = p.name.encode_utf16().chain(std::iter::once(0)).collect(); + // The reader refuses longer names (see `read_pattern`). + if units.len() > 65_536 { + return Err(PsdError::LimitExceeded("pattern name length")); + } out.extend_from_slice(&(units.len() as u32).to_be_bytes()); for u in units { out.extend_from_slice(&u.to_be_bytes()); @@ -533,6 +537,9 @@ pub fn block_key(depth: u16) -> [u8; 4] { } } +/// The most patterns a `.pat` file may hold: more are refused on read and on write. +pub const MAX_PAT_PATTERNS: usize = 100_000; + /// Parses a `.pat` pattern file. pub fn parse_pat_file(data: &[u8]) -> Result> { let mut r = Rd { b: data, p: 0 }; @@ -541,10 +548,13 @@ pub fn parse_pat_file(data: &[u8]) -> Result> { return Err(PsdError::InvalidSignature { expected: "8BPT", found: [sig[0], sig[1], sig[2], sig[3]] }); } let _version = r.u16()?; - let count = r.u32()?; + let count = r.u32()? as usize; + if count > MAX_PAT_PATTERNS { + return Err(PsdError::LimitExceeded("pattern count")); + } let mut out = Vec::new(); let mut budget = PatternBudget::default(); - for _ in 0..count.min(100_000) { + for _ in 0..count { out.push(read_pattern(&mut r, &mut budget)?); } Ok(out) @@ -552,6 +562,9 @@ pub fn parse_pat_file(data: &[u8]) -> Result> { /// Writes a `.pat` pattern file. pub fn write_pat_file(patterns: &[PsdPattern]) -> Result> { + if patterns.len() > MAX_PAT_PATTERNS { + return Err(PsdError::LimitExceeded("pattern count")); + } let mut out = b"8BPT".to_vec(); out.extend_from_slice(&1u16.to_be_bytes()); out.extend_from_slice(&(patterns.len() as u32).to_be_bytes()); @@ -582,6 +595,28 @@ mod tests { } } + /// #9318 review: a name the reader refuses (more than 65,536 UTF-16 units with its NUL) is + /// refused on write, so a saved library can't become a file that loads nothing. + #[test] + fn a_name_the_reader_refuses_is_refused_on_write() { + let long = PsdPattern { name: "a".repeat(65_536), ..pat(8, false) }; + assert!(matches!(write_pat_file(&[long]), Err(PsdError::LimitExceeded(_)))); + let longest = PsdPattern { name: "a".repeat(65_535), ..pat(8, false) }; + assert_eq!(parse_pat_file(&write_pat_file(std::slice::from_ref(&longest)).unwrap()).unwrap(), [longest]); + } + + /// #9318 review: more patterns than a `.pat` holds are refused on write, and a file claiming + /// more is refused on read rather than silently cut to the first 100,000. + #[test] + fn more_patterns_than_a_pat_file_holds_are_refused() { + let many = vec![pat(8, false); MAX_PAT_PATTERNS + 1]; + assert!(matches!(write_pat_file(&many), Err(PsdError::LimitExceeded(_)))); + let mut header = b"8BPT".to_vec(); + header.extend_from_slice(&1u16.to_be_bytes()); + header.extend_from_slice(&(MAX_PAT_PATTERNS as u32 + 1).to_be_bytes()); + assert!(matches!(parse_pat_file(&header), Err(PsdError::LimitExceeded(_)))); + } + #[test] fn block_and_pat_file_round_trip_at_every_depth() { for depth in [8, 16, 32] {