diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 453b68a69..f5153473e 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -699,10 +699,11 @@ impl Catalog { | Op::SetFolderRecord { .. } => self.changes.broad = self.revision.saturating_add(1), } Ok(match op { - Op::AddPhoto { photo } => { + Op::AddPhoto { mut photo } => { if self.photos.contains_key(&photo.id) { return Err(CatalogError::Invalid(format!("photo {:?} exists", photo.id))); } + photo.trim_history(); let id = photo.id; self.next_photo = self.next_photo.max(id.0.saturating_add(1)); self.photos.insert(id, Arc::new(*photo)); @@ -753,7 +754,14 @@ impl Catalog { } Op::SetHistory { id, history } => { let p = self.photo_mut(id)?; - Op::SetHistory { id, history: std::mem::replace(&mut p.history, history) } + let old = std::mem::replace(&mut p.history, history); + let old_cursor = p.history_cursor; + p.trim_history(); + if p.history_cursor == old_cursor { + Op::SetHistory { id, history: old } + } else { + Op::Batch { ops: vec![Op::SetHistory { id, history: old }, Op::SetHistoryCursor { id, cursor: old_cursor }] } + } } Op::PushHistory { id, step } => { let p = self.photo_mut(id)?; @@ -1054,8 +1062,9 @@ impl Catalog { /// finds in them, and walks folders up to the top): every photo, album and stack carries the /// id it is filed under; the id counters lie past every id in use; a stack keeps only photos /// in the catalog, once each, and goes when fewer than two are left; an album whose parent is - /// missing, not a folder or inside itself moves to the top level. Returns what was changed - /// (nothing, for a catalog this build wrote). + /// missing, not a folder or inside itself moves to the top level; a photo keeps its newest + /// [`HISTORY_LIMIT`] History steps. Returns what was changed (nothing, for a catalog this build + /// wrote). pub fn repair(&mut self) -> Vec { let mut changed = Vec::new(); for (id, p) in self.photos.iter_mut() { @@ -1063,6 +1072,10 @@ impl Catalog { changed.push(format!("photo {} was filed as {}", p.id.0, id.0)); Arc::make_mut(p).id = *id; } + if p.history.len() > HISTORY_LIMIT { + let n = Arc::make_mut(p).trim_history(); + changed.push(format!("photo {}: {n} History step(s) beyond the newest {HISTORY_LIMIT} dropped", id.0)); + } } for (id, a) in self.albums.iter_mut() { if a.id != *id { diff --git a/crates/catalog/src/model.rs b/crates/catalog/src/model.rs index b405e1db9..fbf3294db 100644 --- a/crates/catalog/src/model.rs +++ b/crates/catalog/src/model.rs @@ -354,6 +354,17 @@ pub fn preview_stamp(preview_only: &Option) -> String { } impl Photo { + /// Keep the newest [`HISTORY_LIMIT`](crate::HISTORY_LIMIT) History steps; the cursor moves with + /// its step, or to the oldest step kept when its own is dropped. Returns how many were dropped. + pub fn trim_history(&mut self) -> usize { + let n = self.history.len().saturating_sub(crate::HISTORY_LIMIT); + if n > 0 { + self.history.drain(..n); + self.history_cursor = self.history_cursor.map(|c| c.saturating_sub(n)); + } + n + } + /// The History step the settings are at (the cursor, clamped to the list; `None` = no history). pub fn history_index(&self) -> Option { let last = self.history.len().checked_sub(1)?; diff --git a/crates/catalog/src/tests_journal.rs b/crates/catalog/src/tests_journal.rs index 26b4e902a..d50c0e533 100644 --- a/crates/catalog/src/tests_journal.rs +++ b/crates/catalog/src/tests_journal.rs @@ -262,6 +262,31 @@ fn push_history_is_bounded_and_invertible() { assert_eq!(c.to_snapshot(), before); } +/// A photo added with, or given, a whole History (op log replay, undo of a removal) kept every +/// step: only `PushHistory` applied the limit (#2579). +#[test] +fn add_photo_and_set_history_are_bounded_and_invertible() { + let mut c = Catalog::new(); + let id = c.alloc_photo_id(); + let step = |i: usize| HistoryStep { label: format!("s{i}"), settings: Arc::new(DevelopSettings::default()) }; + let mut p = Photo::new(id, Source::Demo { scene: 1 }, "a", "JPEG", 1, 1, "t"); + p.history = (0..HISTORY_LIMIT + 5).map(step).collect(); + p.history_cursor = Some(10); + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let p = c.photo(id).unwrap(); + assert_eq!((p.history.len(), p.history[0].label.as_str()), (HISTORY_LIMIT, "s5")); + assert_eq!(p.history_cursor, Some(5), "the cursor stays on step s10"); + + let before = c.to_snapshot(); + let inv = c.apply(Op::SetHistory { id, history: (0..HISTORY_LIMIT * 2).map(step).collect() }).unwrap(); + let p = c.photo(id).unwrap(); + assert_eq!(p.history.len(), HISTORY_LIMIT); + assert_eq!(p.history[0].label, format!("s{HISTORY_LIMIT}")); + assert_eq!(p.history_cursor, Some(0), "its step was dropped: the oldest one kept"); + c.apply(inv).unwrap(); + assert_eq!(c.to_snapshot(), before); +} + #[test] fn fs_store_roundtrip() { let dir = std::env::temp_dir().join(format!("lc-journal-{}", std::process::id())); diff --git a/crates/catalog/src/tests_repair.rs b/crates/catalog/src/tests_repair.rs index 9feb038a5..b39922a32 100644 --- a/crates/catalog/src/tests_repair.rs +++ b/crates/catalog/src/tests_repair.rs @@ -195,6 +195,33 @@ fn id_counters_are_moved_past_the_ids_in_use() { assert!(c.apply(Op::AddPhoto { photo: Box::new(p) }).is_err(), "the id is taken: refused, not overwritten"); } +/// History was bounded only where an edit adds a step, so a snapshot holding more steps kept +/// every one in memory on each load (#2579). The newest are kept and the cursor stays on its step. +#[test] +fn history_beyond_the_limit_is_trimmed_on_load() { + let (mut c, ids) = sound(); + let extra = HISTORY_LIMIT * 5; + let step = HistoryStep { label: "s0".into(), settings: std::sync::Arc::new(lightcraft_develop::DevelopSettings::default()) }; + c.apply(Op::PushHistory { id: ids[0], step }).unwrap(); + c.apply(Op::SetHistoryCursor { id: ids[0], cursor: Some(extra - 3) }).unwrap(); + let (loaded, report) = load_edited(&c, |v| { + let history = &mut v["photos"][ids[0].0.to_string()]["history"]; + let one = history[0].clone(); + *history = (0..extra) + .map(|i| { + let mut s = one.clone(); + s["label"] = format!("s{i}").into(); + s + }) + .collect(); + }); + let p = loaded.photo(ids[0]).unwrap(); + assert_eq!(p.history.len(), HISTORY_LIMIT); + assert_eq!(p.history[0].label, format!("s{}", extra - HISTORY_LIMIT)); + assert_eq!(p.history_index().map(|i| p.history[i].label.clone()), Some(format!("s{}", extra - 3))); + assert_eq!(report.repaired.len(), 1, "{:?}", report.repaired); +} + /// A sound catalog loads unchanged. #[test] fn a_sound_snapshot_needs_no_repair() {