mirror of
https://github.com/storytold/lightcraft.git
synced 2026-11-01 14:07:34 -06:00
LR-LIB-SMARTALBUM: the catalog refuses an edit that makes a smart album include itself
Session::commit now goes through Catalog::apply_new, which looks at what an op would do to which album tests which (AlbumGraph, foresee) before doing it, and refuses one that puts a smart album on a loop: alone, in a batch, or as a new album. Undo, redo, replay and opening a library still bring back what was, loops included.
This commit is contained in:
Nathaniel Ledford
@@ -652,6 +652,44 @@ impl Catalog {
|
||||
Ok(inv)
|
||||
}
|
||||
|
||||
/// [`Self::apply`] for a new edit (not an undo, a redo or the replay of a saved log, which bring
|
||||
/// back what was): refused when it would put a smart album on a loop, one that includes itself
|
||||
/// through the albums it tests. THE place that rule is kept, whichever command, importer or
|
||||
/// task makes the edit, alone or in a batch. A library that already holds a loop still opens,
|
||||
/// and an edit that puts no album on one it was not on goes through, also to an album on a loop.
|
||||
///
|
||||
/// The check is made on what the edit would do to the albums, before it is done: a refused
|
||||
/// edit never touched the library, so there is nothing to take back.
|
||||
pub fn apply_new(&mut self, op: Op) -> Result<Op> {
|
||||
if let Some((id, name)) = self.would_put_on_a_loop(&op) {
|
||||
return Err(CatalogError::Invalid(format!(
|
||||
"smart album “{name}” (album {}) would include itself: its rules lead back to it through the albums they test",
|
||||
id.0
|
||||
)));
|
||||
}
|
||||
self.apply(op)
|
||||
}
|
||||
|
||||
/// The album `op` would newly put on a loop, as its id and name: the album the op is about
|
||||
/// when that is one of them. `None` when it makes none (or changes no album's rules).
|
||||
fn would_put_on_a_loop(&self, op: &Op) -> Option<(AlbumId, String)> {
|
||||
if !changes_what_albums_test(op) {
|
||||
return None;
|
||||
}
|
||||
let mut graph = AlbumGraph::of(self.albums.values());
|
||||
let now = graph.on_a_loop();
|
||||
let mut edited = Vec::new();
|
||||
foresee(&mut graph, op, &mut edited);
|
||||
let then = graph.on_a_loop();
|
||||
let name_of = |id: AlbumId| self.albums.get(&id).map(|a| a.name.clone()).unwrap_or_default();
|
||||
let new_edited = edited.iter().find(|(id, _)| then.contains(id) && !now.contains(id));
|
||||
match new_edited {
|
||||
Some((id, name)) => Some((*id, name.map_or_else(|| name_of(*id), str::to_string))),
|
||||
// an album the edit is not about (it tests one that now leads back to it)
|
||||
None => then.difference(&now).next().map(|id| (*id, name_of(*id))),
|
||||
}
|
||||
}
|
||||
|
||||
fn apply_inner(&mut self, op: Op) -> Result<Op> {
|
||||
// An op that changes one photo says which (`photo_mut`, and adding or removing it below). Every other kind
|
||||
// can change what any photo matches or where it sorts: an album's photos or rules, a stack, a keyword's
|
||||
@@ -1180,6 +1218,153 @@ impl Catalog {
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether `op` changes which albums a smart album tests: it adds or removes an album, or changes
|
||||
/// rules (alone or in a batch).
|
||||
fn changes_what_albums_test(op: &Op) -> bool {
|
||||
// listed kind by kind (as in `apply_inner`), so a new kind has to be put on one side
|
||||
match op {
|
||||
Op::AddAlbum { .. } | Op::RemoveAlbum { .. } | Op::SetAlbumRules { .. } => true,
|
||||
Op::Batch { ops } => ops.iter().any(changes_what_albums_test),
|
||||
Op::AddPhoto { .. }
|
||||
| Op::RemovePhoto { .. }
|
||||
| Op::SetDeleted { .. }
|
||||
| Op::SetLocal { .. }
|
||||
| Op::SetContent { .. }
|
||||
| Op::SetEmbeddedLens { .. }
|
||||
| Op::SetAsShotWb { .. }
|
||||
| Op::SetRating { .. }
|
||||
| Op::SetFlag { .. }
|
||||
| Op::SetLabel { .. }
|
||||
| Op::SetMeta { .. }
|
||||
| Op::SetCaptured { .. }
|
||||
| Op::SetFile { .. }
|
||||
| Op::Relink { .. }
|
||||
| Op::SetAnalysis { .. }
|
||||
| Op::SetDevelop { .. }
|
||||
| Op::SetVersions { .. }
|
||||
| Op::SetHistory { .. }
|
||||
| Op::PushHistory { .. }
|
||||
| Op::SetHistoryCursor { .. }
|
||||
| Op::RenameAlbum { .. }
|
||||
| Op::MoveAlbum { .. }
|
||||
| Op::SetAlbumOrder { .. }
|
||||
| Op::SetAlbumPhotos { .. }
|
||||
| Op::SetAlbumCover { .. }
|
||||
| Op::AddStack { .. }
|
||||
| Op::RemoveStack { .. }
|
||||
| Op::SetStack { .. }
|
||||
| Op::SetLabelName { .. }
|
||||
| Op::SetBrowsed { .. }
|
||||
| Op::SetKeyword { .. }
|
||||
| Op::SetFolderRecord { .. } => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// What `op` would do to `graph`, as far as loops go (which album tests which), without the
|
||||
/// checks `apply` makes: an op it would refuse is refused there. `edited` gets the albums the op
|
||||
/// is about, with their name when the op brings it.
|
||||
fn foresee<'a>(graph: &mut AlbumGraph, op: &'a Op, edited: &mut Vec<(AlbumId, Option<&'a str>)>) {
|
||||
match op {
|
||||
Op::AddAlbum { album } => {
|
||||
// an id that is taken stays as it is: `apply` refuses the add
|
||||
if let std::collections::hash_map::Entry::Vacant(slot) = graph.leads_to.entry(album.id) {
|
||||
slot.insert(album.smart.as_deref().map(Filter::albums_tested));
|
||||
edited.push((album.id, Some(album.name.as_str())));
|
||||
}
|
||||
}
|
||||
Op::RemoveAlbum { id } => {
|
||||
graph.leads_to.remove(id);
|
||||
}
|
||||
Op::SetAlbumRules { id, rules } => {
|
||||
// only a smart album has rules: `apply` refuses them for another
|
||||
if let Some(tested) = graph.leads_to.get_mut(id).filter(|t| t.is_some()) {
|
||||
*tested = Some(rules.albums_tested());
|
||||
edited.push((*id, None));
|
||||
}
|
||||
}
|
||||
Op::Batch { ops } => ops.iter().for_each(|op| foresee(graph, op, edited)),
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
/// Which album tests which: what a smart album's photos depend on beyond its own rules. Made in
|
||||
/// one look at the albums, for questions about all of them.
|
||||
struct AlbumGraph {
|
||||
/// Every album; `Some(albums its rules test)` for a smart album, `None` for any other.
|
||||
leads_to: std::collections::HashMap<AlbumId, Option<Vec<AlbumId>>>,
|
||||
}
|
||||
|
||||
impl AlbumGraph {
|
||||
fn of<'a>(albums: impl Iterator<Item = &'a Album>) -> AlbumGraph {
|
||||
AlbumGraph { leads_to: albums.map(|a| (a.id, a.smart.as_deref().map(Filter::albums_tested))).collect() }
|
||||
}
|
||||
|
||||
/// The smart albums that lead back to themselves: those in a ring of albums leading to one
|
||||
/// another (a strongly connected part of more than one album), or testing themselves. One
|
||||
/// walk over the albums (Tarjan's, with its own stack rather than recursion: a chain of
|
||||
/// thousands of albums must not overflow the call stack).
|
||||
fn on_a_loop(&self) -> std::collections::BTreeSet<AlbumId> {
|
||||
// only what tests an album can be on a ring: number those
|
||||
let testing: Vec<(AlbumId, &[AlbumId])> =
|
||||
self.leads_to.iter().filter_map(|(id, tested)| tested.as_deref().filter(|t| !t.is_empty()).map(|t| (*id, t))).collect();
|
||||
let number: std::collections::HashMap<AlbumId, usize> = testing.iter().enumerate().map(|(i, (id, _))| (*id, i)).collect();
|
||||
let leads: Vec<Vec<usize>> = testing.iter().map(|(_, t)| t.iter().filter_map(|to| number.get(to).copied()).collect()).collect();
|
||||
const UNSEEN: usize = usize::MAX;
|
||||
let n = testing.len();
|
||||
// when each was first reached, the earliest still-open album it leads back to, the open
|
||||
// ones in the order reached, and whether each ended up on a ring
|
||||
let (mut reached, mut back) = (vec![UNSEEN; n], vec![0usize; n]);
|
||||
let (mut open, mut is_open, mut ringed) = (Vec::new(), vec![false; n], vec![false; n]);
|
||||
let mut clock = 0usize;
|
||||
for root in 0..n {
|
||||
if reached[root] != UNSEEN {
|
||||
continue;
|
||||
}
|
||||
// (album, how many of its leads were followed)
|
||||
let mut walk: Vec<(usize, usize)> = vec![(root, 0)];
|
||||
while let Some(&(v, followed)) = walk.last() {
|
||||
if followed == 0 && reached[v] == UNSEEN {
|
||||
reached[v] = clock;
|
||||
back[v] = clock;
|
||||
clock += 1;
|
||||
open.push(v);
|
||||
is_open[v] = true;
|
||||
}
|
||||
if let Some(&w) = leads[v].get(followed) {
|
||||
if let Some(top) = walk.last_mut() {
|
||||
top.1 += 1;
|
||||
}
|
||||
if reached[w] == UNSEEN {
|
||||
walk.push((w, 0));
|
||||
} else if is_open[w] {
|
||||
back[v] = back[v].min(reached[w]);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
// every lead of `v` followed: it closes, and with it the ring it is the first of
|
||||
walk.pop();
|
||||
if let Some(&(parent, _)) = walk.last() {
|
||||
back[parent] = back[parent].min(back[v]);
|
||||
}
|
||||
if back[v] == reached[v] {
|
||||
let mut ring = Vec::new();
|
||||
while let Some(w) = open.pop() {
|
||||
is_open[w] = false;
|
||||
ring.push(w);
|
||||
if w == v {
|
||||
break;
|
||||
}
|
||||
}
|
||||
if ring.len() > 1 || leads[v].contains(&v) {
|
||||
ring.into_iter().for_each(|w| ringed[w] = true);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
testing.iter().zip(&ringed).filter(|(_, ringed)| **ringed).map(|((id, _), _)| *id).collect()
|
||||
}
|
||||
}
|
||||
|
||||
/// How siblings are listed: folders first, then the ones with a place of their own by it, then by
|
||||
/// name; the id settles any tie.
|
||||
fn album_order_key(a: &Album) -> (bool, bool, u32, String, AlbumId) {
|
||||
@@ -1189,6 +1374,8 @@ fn album_order_key(a: &Album) -> (bool, bool, u32, String, AlbumId) {
|
||||
#[cfg(test)]
|
||||
mod tests;
|
||||
#[cfg(test)]
|
||||
mod tests_album_loops;
|
||||
#[cfg(test)]
|
||||
mod tests_album_order;
|
||||
#[cfg(test)]
|
||||
mod tests_background;
|
||||
|
||||
@@ -0,0 +1,243 @@
|
||||
//! Smart albums that would include themselves, through the albums they test.
|
||||
//!
|
||||
//! The rule is the catalog's: a new edit that would put a smart album on such a loop is refused
|
||||
//! (`Catalog::apply_new`), whoever makes it and however it is packed (alone, in a batch, as a
|
||||
//! new album). A library that already holds a loop opens and keeps working (`Catalog::apply`,
|
||||
//! `replay` and snapshots take it).
|
||||
//!
|
||||
//! Scenarios:
|
||||
//! - When a new edit (adding a smart album, changing rules, alone or in a batch) would put a
|
||||
//! smart album on a loop, then it is refused, names the album, and nothing changed.
|
||||
//! - When a new edit makes no loop, then it goes through, whatever else is in the library.
|
||||
//! - Given a library that already holds a loop, then it opens, and edits that add no loop go
|
||||
//! through (also to the albums on it), and the loop can be mended.
|
||||
//! - Given a ring of thousands of smart albums, then the edit that closes it is refused without
|
||||
//! running out of stack.
|
||||
|
||||
use crate::*;
|
||||
|
||||
fn smart(c: &mut Catalog, name: &str, rules: Filter) -> AlbumId {
|
||||
let id = c.alloc_album_id();
|
||||
c.apply(Op::AddAlbum { album: Album { smart: Some(Box::new(rules)), ..Album::new(id, name) } }).unwrap();
|
||||
id
|
||||
}
|
||||
|
||||
fn plain(c: &mut Catalog, name: &str) -> AlbumId {
|
||||
let id = c.alloc_album_id();
|
||||
c.apply(Op::AddAlbum { album: Album::new(id, name) }).unwrap();
|
||||
id
|
||||
}
|
||||
|
||||
/// Rules of Album rules: `(op, album)` joined by `all`.
|
||||
fn tests(rules: &[(&str, AlbumId)]) -> Filter {
|
||||
let rules: Vec<serde_json::Value> = rules.iter().map(|(op, a)| serde_json::json!({"field": "album", "op": op, "value": a.0})).collect();
|
||||
serde_json::from_value(serde_json::json!({"ruleSet": {"match": "all", "rules": rules}})).unwrap()
|
||||
}
|
||||
|
||||
/// The same, with the albums in a group of the rules.
|
||||
fn tests_in_a_group(album: AlbumId) -> Filter {
|
||||
serde_json::from_value(serde_json::json!({"ruleSet": {"match": "all", "rules": [
|
||||
{"field": "rating", "op": "is", "value": 3},
|
||||
{"group": {"match": "any", "rules": [{"field": "album", "op": "is", "value": album.0}]}}
|
||||
]}}))
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
fn set_rules(id: AlbumId, rules: Filter) -> Op {
|
||||
Op::SetAlbumRules { id, rules: Box::new(rules) }
|
||||
}
|
||||
|
||||
fn rated(rating: u8) -> Filter {
|
||||
Filter { rating, ..Default::default() }
|
||||
}
|
||||
|
||||
/// What the albums are: the whole library as saved.
|
||||
fn saved(c: &Catalog) -> String {
|
||||
c.to_snapshot()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_new_edit_that_would_make_a_loop_is_refused() {
|
||||
let mut c = Catalog::new();
|
||||
let a = smart(&mut c, "A", rated(1));
|
||||
let b = smart(&mut c, "B", tests(&[("is", a)]));
|
||||
let chain = smart(&mut c, "Chain", tests(&[("isNot", b)]));
|
||||
let (x, y) = (smart(&mut c, "X", rated(2)), smart(&mut c, "Y", rated(3)));
|
||||
// F tests E; E is removed, so F tests an album that is not there (a saved rule may)
|
||||
let e = smart(&mut c, "E", rated(4));
|
||||
let f = smart(&mut c, "F", tests(&[("is", e)]));
|
||||
c.apply(Op::RemoveAlbum { id: e }).unwrap();
|
||||
let (before, revision) = (saved(&c), c.revision);
|
||||
let refused: Vec<(&str, Op, &str)> = vec![
|
||||
("tests an album that tests it", set_rules(a, tests(&[("is", b)])), "A"),
|
||||
("tests itself", set_rules(a, tests(&[("is", a)])), "A"),
|
||||
("tests itself through a group", set_rules(a, tests_in_a_group(a)), "A"),
|
||||
("tests the end of a chain that leads to it", set_rules(a, tests(&[("isNot", chain)])), "A"),
|
||||
("limited to itself by its album field", set_rules(a, Filter { album: Some(a), ..Default::default() }), "A"),
|
||||
("two edits, each alone without a loop", Op::Batch { ops: vec![set_rules(x, tests(&[("is", y)])), set_rules(y, tests(&[("is", x)]))] }, "X"),
|
||||
("a rename and a loop", Op::Batch { ops: vec![Op::RenameAlbum { id: a, name: "Renamed".into() }, set_rules(a, tests(&[("is", b)]))] }, "A"),
|
||||
(
|
||||
"a batch in a batch",
|
||||
Op::Batch {
|
||||
ops: vec![Op::Batch { ops: vec![set_rules(x, tests(&[("is", y)]))] }, Op::Batch { ops: vec![set_rules(y, tests(&[("is", x)]))] }],
|
||||
},
|
||||
"X",
|
||||
),
|
||||
(
|
||||
"a new album that takes the id an album was tested by",
|
||||
Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", f)]))), ..Album::new(e, "Back") } },
|
||||
"Back",
|
||||
),
|
||||
(
|
||||
"a new album and its own rules in one batch",
|
||||
Op::Batch {
|
||||
ops: vec![
|
||||
Op::AddAlbum { album: Album { smart: Some(Box::new(rated(1))), ..Album::new(AlbumId(90), "Fresh") } },
|
||||
set_rules(AlbumId(90), tests(&[("is", AlbumId(90))])),
|
||||
],
|
||||
},
|
||||
"Fresh",
|
||||
),
|
||||
];
|
||||
for (what, op, name) in refused {
|
||||
let e = c.apply_new(op).unwrap_err().to_string();
|
||||
assert!(e.contains(&format!("“{name}”")) && e.contains("include itself"), "{what}: {e}");
|
||||
assert_eq!(saved(&c), before, "{what}: nothing changed");
|
||||
assert_eq!(c.revision, revision, "{what}: not even looked at as changed");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_new_edit_that_makes_no_loop_goes_through() {
|
||||
let mut c = Catalog::new();
|
||||
let a = smart(&mut c, "A", rated(1));
|
||||
let b = smart(&mut c, "B", tests(&[("is", a)]));
|
||||
let list = plain(&mut c, "List");
|
||||
let c1 = smart(&mut c, "C", tests(&[("isNot", b)]));
|
||||
let allowed = vec![
|
||||
("a chain gets longer", set_rules(a, rated(2))),
|
||||
("testing a plain album", set_rules(a, tests(&[("is", list)]))),
|
||||
("a smart album testing another, the other way round", set_rules(c1, tests(&[("is", a), ("is", b)]))),
|
||||
(
|
||||
"a new smart album testing others",
|
||||
Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", c1)]))), ..Album::new(AlbumId(50), "New") } },
|
||||
),
|
||||
("a new plain album", Op::AddAlbum { album: Album::new(AlbumId(51), "Plain") }),
|
||||
("a rename", Op::RenameAlbum { id: b, name: "B2".into() }),
|
||||
("a removal", Op::RemoveAlbum { id: c1 }),
|
||||
("photos of a plain album", Op::SetAlbumPhotos { id: list, photos: vec![] }),
|
||||
("the album field of the rules", set_rules(b, Filter { album: Some(list), ..Default::default() })),
|
||||
(
|
||||
"a batch of edits that don't meet",
|
||||
Op::Batch { ops: vec![set_rules(a, rated(3)), Op::RenameAlbum { id: a, name: "A3".into() }, set_rules(b, tests(&[("is", a)]))] },
|
||||
),
|
||||
(
|
||||
"a batch that passes through a loop and ends without one",
|
||||
Op::Batch { ops: vec![set_rules(a, tests(&[("is", b)])), set_rules(b, rated(1))] },
|
||||
),
|
||||
];
|
||||
for (what, op) in allowed {
|
||||
let revision = c.revision;
|
||||
let undo = c.apply_new(op).unwrap_or_else(|e| panic!("{what}: {e}"));
|
||||
assert_eq!(c.revision, revision + 1, "{what}");
|
||||
assert!(c.albums().all(|al| !c.smart_album_problems(al.id).iter().any(|p| p.issue == rules::Issue::AlbumLoop)), "{what}: no loop");
|
||||
// and it is undone by what it returned
|
||||
c.apply(undo).unwrap();
|
||||
}
|
||||
}
|
||||
|
||||
/// The refusal is the only difference between the two ways in: an edit that makes no loop does
|
||||
/// the same through either.
|
||||
#[test]
|
||||
fn apply_new_changes_what_apply_changes() {
|
||||
let mut a = Catalog::new();
|
||||
let first = smart(&mut a, "A", rated(1));
|
||||
let second = smart(&mut a, "B", tests(&[("is", first)]));
|
||||
let mut b = Catalog::from_snapshot(&saved(&a)).unwrap();
|
||||
let ops = vec![set_rules(first, rated(4)), Op::RenameAlbum { id: second, name: "B2".into() }, set_rules(second, tests(&[("isNot", first)]))];
|
||||
for op in ops {
|
||||
let (ia, ib) = (a.apply(op.clone()).unwrap(), b.apply_new(op).unwrap());
|
||||
assert_eq!(Catalog::op_to_log_line(&ia), Catalog::op_to_log_line(&ib));
|
||||
}
|
||||
assert_eq!(saved(&a), saved(&b));
|
||||
}
|
||||
|
||||
/// A library saved by a version without the rule: it may hold a loop. It opens (a snapshot, a
|
||||
/// replayed log), its albums still list and count, edits that make no loop go through, also
|
||||
/// those to the albums on the loop, and an edit that closes another loop is still refused.
|
||||
#[test]
|
||||
fn a_library_that_already_holds_a_loop_opens_and_can_be_mended() {
|
||||
let mut old = Catalog::new();
|
||||
let mut log = String::new();
|
||||
let mut record = |c: &mut Catalog, op: Op| {
|
||||
log.push_str(&Catalog::op_to_log_line(&op));
|
||||
c.apply(op).unwrap();
|
||||
};
|
||||
let (first, second, other, third) = (AlbumId(1), AlbumId(2), AlbumId(3), AlbumId(4));
|
||||
record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(first, "First") } });
|
||||
record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", first)]))), ..Album::new(second, "Second") } });
|
||||
record(&mut old, set_rules(first, tests(&[("is", second)])));
|
||||
record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(other, "Other") } });
|
||||
record(&mut old, Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", other)]))), ..Album::new(third, "Third") } });
|
||||
let mut c = Catalog::new();
|
||||
assert_eq!(c.replay(&log).unwrap(), 5, "the log replays, loop and all");
|
||||
let mut from_snapshot = Catalog::from_snapshot(&c.to_snapshot()).unwrap();
|
||||
for lib in [&mut c, &mut from_snapshot] {
|
||||
// they still answer
|
||||
for id in [first, second, other, third] {
|
||||
let _ = (lib.album_photos(id), lib.album_count(id), lib.smart_album_problems(id));
|
||||
}
|
||||
// edits that make no loop go through: elsewhere, and to the albums on the loop
|
||||
lib.apply_new(Op::RenameAlbum { id: first, name: "First, renamed".into() }).unwrap();
|
||||
lib.apply_new(set_rules(other, rated(2))).unwrap();
|
||||
lib.apply_new(set_rules(third, tests(&[("isNot", other)]))).unwrap();
|
||||
lib.apply_new(Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", second)]))), ..Album::new(AlbumId(60), "Reads the loop") } })
|
||||
.unwrap();
|
||||
lib.apply_new(set_rules(first, Filter { rating: 3, rule_set: tests(&[("is", second)]).rule_set, ..Default::default() })).unwrap();
|
||||
// not one that closes another
|
||||
let e = lib.apply_new(set_rules(other, tests(&[("is", third)]))).unwrap_err().to_string();
|
||||
assert!(e.contains("“Other”") && e.contains("include itself"), "{e}");
|
||||
// mended by an edit that makes no loop
|
||||
lib.apply_new(set_rules(second, rated(1))).unwrap();
|
||||
assert!(!lib.smart_album_problems(first).iter().any(|p| p.issue == rules::Issue::AlbumLoop), "mended");
|
||||
assert!(!lib.smart_album_problems(second).iter().any(|p| p.issue == rules::Issue::AlbumLoop), "mended");
|
||||
}
|
||||
}
|
||||
|
||||
/// What an album on a loop holds is whatever it held before the refusal existed: the rule keeps
|
||||
/// new loops out and decides nothing about the photos of old ones.
|
||||
#[test]
|
||||
fn what_an_album_on_a_loop_holds_is_left_as_it_was() {
|
||||
let mut c = Catalog::new();
|
||||
let id = c.alloc_photo_id();
|
||||
c.apply(Op::AddPhoto { photo: Box::new(Photo::new(id, Source::Demo { scene: 1 }, "p.jpg", "JPEG", 60, 40, "2026-02-01T10:00:00")) }).unwrap();
|
||||
c.apply(Op::SetRating { id, rating: 3 }).unwrap();
|
||||
let first = smart(&mut c, "First", Filter::default());
|
||||
let second = smart(&mut c, "Second", tests(&[("is", first)]));
|
||||
c.apply(set_rules(first, tests(&[("is", second)]))).unwrap();
|
||||
let held = |c: &Catalog| [first, second].map(|a| (c.album_photos(a), c.album_count(a)));
|
||||
let looped = held(&c);
|
||||
// edits that go through the check, one of them to an album on the loop
|
||||
c.apply_new(set_rules(first, Filter { rule_set: tests(&[("is", second)]).rule_set, ..Default::default() })).unwrap();
|
||||
let id = c.alloc_album_id();
|
||||
c.apply_new(Op::AddAlbum { album: Album { smart: Some(Box::new(tests(&[("is", second)]))), ..Album::new(id, "Reads the loop") } }).unwrap();
|
||||
assert_eq!(held(&c), looped);
|
||||
}
|
||||
|
||||
/// The search for loops walks the albums with its own stack: a ring of thousands of smart albums
|
||||
/// is found, not overflowed.
|
||||
#[test]
|
||||
fn a_very_long_ring_is_refused_without_running_out_of_stack() {
|
||||
const N: usize = 20_000;
|
||||
let mut c = Catalog::new();
|
||||
let ids: Vec<AlbumId> = (0..N).map(|i| smart(&mut c, &format!("S{i}"), Filter::default())).collect();
|
||||
// each tests the next; the last one is left open
|
||||
for pair in ids.windows(2) {
|
||||
c.apply(set_rules(pair[0], tests(&[("is", pair[1])]))).unwrap();
|
||||
}
|
||||
let (first, last) = (ids[0], ids[N - 1]);
|
||||
let e = c.apply_new(set_rules(last, tests(&[("is", first)]))).unwrap_err().to_string();
|
||||
assert!(e.contains("include itself"), "{e}");
|
||||
// and what makes no ring goes through
|
||||
c.apply_new(set_rules(last, rated(5))).unwrap();
|
||||
}
|
||||
@@ -628,10 +628,12 @@ impl Session {
|
||||
|
||||
// ---------------------------------------------------------------- ops, undo
|
||||
|
||||
/// Apply an op as one undoable step.
|
||||
/// Apply an op as one undoable step. A new edit: one that would make a smart album include
|
||||
/// itself is refused here, whichever command, task or caller asked ([`Catalog::apply_new`]);
|
||||
/// undo, redo and opening a library bring back what was and are not held to that.
|
||||
pub fn commit(&mut self, label: &str, op: Op) -> Result<()> {
|
||||
let fwd = op.clone();
|
||||
let inv = self.apply_catalog(op)?;
|
||||
let inv = self.apply_through(op, Catalog::apply_new)?;
|
||||
self.pending_log.push(fwd);
|
||||
self.undo.push(UndoEntry { label: label.to_string(), op: inv, folder: None, retry_inverse: Vec::new(), retry_sidecars: Vec::new() });
|
||||
self.commits = self.commits.wrapping_add(1);
|
||||
@@ -645,7 +647,14 @@ impl Session {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// [`Catalog::apply`] for what brings back what was (an undo or a redo).
|
||||
fn apply_catalog(&mut self, op: Op) -> Result<Op> {
|
||||
self.apply_through(op, Catalog::apply)
|
||||
}
|
||||
|
||||
/// Apply `op` with `apply` (the catalog's way in for a new edit or for bringing back what
|
||||
/// was), then forget what the session cached of the photos it replaced.
|
||||
fn apply_through(&mut self, op: Op, apply: fn(&mut Catalog, Op) -> std::result::Result<Op, lightcraft_catalog::CatalogError>) -> Result<Op> {
|
||||
// Retain only cache IDs while the catalog consumes the operation. Cloning an import
|
||||
// batch here also cloned every photo, although commit already keeps the journal copy.
|
||||
let mut invalidated = Vec::new();
|
||||
@@ -664,7 +673,7 @@ impl Session {
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
let inv = self.catalog.apply(op)?;
|
||||
let inv = apply(&mut self.catalog, op)?;
|
||||
for id in invalidated {
|
||||
self.media.forget(id);
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ use serde_json::{Value, json};
|
||||
|
||||
use crate::{Selection, Session, command_specs};
|
||||
|
||||
mod album_loops;
|
||||
mod album_order;
|
||||
mod album_remove;
|
||||
mod albums_local;
|
||||
|
||||
@@ -0,0 +1,102 @@
|
||||
//! A smart album never comes to include itself by a new edit, whatever makes the edit
|
||||
//! (`Session::commit` keeps the catalog's rule, `Catalog::apply_new`); a library that already
|
||||
//! holds such a loop opens, works and can be mended, and undo and redo move across it freely.
|
||||
//!
|
||||
//! Scenarios:
|
||||
//! - When an edit that no command checked (an op committed on its own, in a batch of edits that
|
||||
//! are fine one by one, or as a new album) would put a smart album on a loop, then it is
|
||||
//! refused, and there is nothing to undo or to journal.
|
||||
//! - When commands make the edits, then every way to a loop stays refused and ordinary edits go
|
||||
//! on, one undo step each.
|
||||
//! - Given a library that holds a loop, then edits that add none go through, and undo and redo
|
||||
//! bring the loop back and take it away without being refused.
|
||||
|
||||
use lightcraft_catalog::{Album, AlbumId, Filter, Op};
|
||||
use serde_json::json;
|
||||
|
||||
use crate::Session;
|
||||
|
||||
fn tests(albums: &[(&str, u64)]) -> Filter {
|
||||
let rules: Vec<_> = albums.iter().map(|(op, a)| json!({"field": "album", "op": op, "value": a})).collect();
|
||||
serde_json::from_value(json!({"ruleSet": {"match": "all", "rules": rules}})).unwrap()
|
||||
}
|
||||
|
||||
fn smart(s: &mut Session, name: &str, rules: serde_json::Value) -> u64 {
|
||||
s.execute("album.createSmart", &json!({"name": name, "rules": rules})).unwrap()["id"].as_u64().unwrap()
|
||||
}
|
||||
|
||||
fn set(id: u64, rules: Filter) -> Op {
|
||||
Op::SetAlbumRules { id: AlbumId(id), rules: Box::new(rules) }
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_edit_no_command_checked_cant_make_a_loop() {
|
||||
let mut s = Session::new();
|
||||
let a = smart(&mut s, "A", json!({"rating": 1}));
|
||||
let b = smart(&mut s, "B", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}}));
|
||||
let (x, y) = (smart(&mut s, "X", json!({"rating": 2})), smart(&mut s, "Y", json!({"rating": 3})));
|
||||
let gone = smart(&mut s, "Gone", json!({"rating": 4}));
|
||||
let waits = smart(&mut s, "Waits", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": gone}]}}));
|
||||
s.execute("album.delete", &json!({"id": gone})).unwrap();
|
||||
let (steps, logged, snapshot) = (s.undo.len(), s.pending_log.len(), s.catalog.to_snapshot());
|
||||
let back = Album { smart: Some(Box::new(tests(&[("is", waits)]))), ..Album::new(AlbumId(gone), "Back") };
|
||||
for (what, op) in [
|
||||
("rules that test an album that tests it", set(a, tests(&[("is", b)]))),
|
||||
("two edits, each fine alone", Op::Batch { ops: vec![set(x, tests(&[("is", y)])), set(y, tests(&[("is", x)]))] }),
|
||||
("a new album that takes the id of one a rule tests", Op::AddAlbum { album: back }),
|
||||
] {
|
||||
let e = s.commit("Edit", op).unwrap_err().to_string();
|
||||
assert!(e.contains("include itself"), "{what}: {e}");
|
||||
assert_eq!((s.undo.len(), s.pending_log.len()), (steps, logged), "{what}: nothing to undo, nothing journaled");
|
||||
assert_eq!(s.catalog.to_snapshot(), snapshot, "{what}: nothing changed");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn commands_refuse_every_way_to_a_loop_and_ordinary_edits_go_on() {
|
||||
let mut s = Session::with_demo();
|
||||
let a = smart(&mut s, "A", json!({"rating": 1}));
|
||||
let b = smart(&mut s, "B", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}}));
|
||||
let steps = s.undo.len();
|
||||
let looping = |s: &mut Session, params: serde_json::Value| {
|
||||
let e = s.execute("album.setRules", ¶ms).unwrap_err().to_string();
|
||||
assert!(e.contains("include itself"), "{e}");
|
||||
};
|
||||
looping(&mut s, json!({"id": a, "rules": {"ruleSet": {"rules": [{"field": "album", "op": "is", "value": b}]}}}));
|
||||
looping(&mut s, json!({"id": a, "name": "Renamed too", "rules": {"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}}}));
|
||||
assert_eq!((s.undo.len(), s.catalog.album(AlbumId(a)).map(|x| x.name.clone())), (steps, Some("A".into())));
|
||||
// ordinary edits: a rename together with new rules is one undo step
|
||||
s.execute("album.setRules", &json!({"id": b, "name": "B2", "rules": {"rating": 2}})).unwrap();
|
||||
s.execute("album.rename", &json!({"id": a, "name": "A2"})).unwrap();
|
||||
assert_eq!(s.undo.len(), steps + 2);
|
||||
s.execute("edit.undo", &json!({})).unwrap();
|
||||
s.execute("edit.undo", &json!({})).unwrap();
|
||||
assert_eq!(s.catalog.album(AlbumId(b)).map(|x| x.name.clone()), Some("B".into()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_library_that_holds_a_loop_goes_on_and_undo_and_redo_cross_it() {
|
||||
let mut s = Session::with_demo();
|
||||
let a = smart(&mut s, "A", json!({"rating": 1}));
|
||||
let b = smart(&mut s, "B", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}}));
|
||||
// as a version without the rule saved it
|
||||
s.catalog.apply(set(a, tests(&[("is", b)]))).unwrap();
|
||||
let on_a_loop = |s: &Session| {
|
||||
[a, b]
|
||||
.into_iter()
|
||||
.filter(|id| s.catalog.smart_album_problems(AlbumId(*id)).iter().any(|p| p.issue == lightcraft_catalog::rules::Issue::AlbumLoop))
|
||||
.count()
|
||||
};
|
||||
assert_eq!(on_a_loop(&s), 2);
|
||||
// edits that add no loop: a rename, a change that leaves the rule as it is, a new album
|
||||
s.execute("album.rename", &json!({"id": a, "name": "Still looping"})).unwrap();
|
||||
s.execute("album.setRules", &json!({"id": a, "rules": {"rating": 3}})).unwrap();
|
||||
smart(&mut s, "Reads the loop", json!({"ruleSet": {"rules": [{"field": "album", "op": "is", "value": a}]}}));
|
||||
// mending it is one step; undo brings back what was and is not refused; redo mends again
|
||||
s.execute("album.setRules", &json!({"id": b, "replace": true, "rules": {"rating": 2}})).unwrap();
|
||||
assert_eq!(on_a_loop(&s), 0);
|
||||
s.execute("edit.undo", &json!({})).unwrap();
|
||||
assert_eq!(on_a_loop(&s), 2, "undo is no new edit");
|
||||
s.execute("edit.redo", &json!({})).unwrap();
|
||||
assert_eq!(on_a_loop(&s), 0);
|
||||
}
|
||||
Reference in New Issue
Block a user