diff --git a/crates/engine/src/finite_geometry_tests.rs b/crates/engine/src/finite_geometry_tests.rs index 416b250c8..52ba2a575 100644 --- a/crates/engine/src/finite_geometry_tests.rs +++ b/crates/engine/src/finite_geometry_tests.rs @@ -38,3 +38,48 @@ fn huge_scale_is_refused_and_the_file_still_reopens() { assert!(n.width.is_finite() && n.height.is_finite(), "{} x {}", n.width, n.height); assert_roundtrips(&s); } + +/// A settled document of `groups` groups of `per` rectangles each, and one of the rectangles. +fn large(groups: usize, per: usize) -> (figcraft_doc::Document, crate::NodeId) { + use figcraft_doc::NodeKind; + let mut doc = figcraft_doc::Document::new("Large"); + let page = doc.first_page().unwrap(); + let mut last = None; + for g in 0..groups { + let group = doc.create(page, NodeKind::Group, "group", 0.0, g as f64 * 20.0, 10.0, 10.0).unwrap(); + for i in 0..per { + last = Some(doc.create(group, NodeKind::Rectangle, "rect", i as f64 * 20.0, 0.0, 10.0, 10.0).unwrap()); + } + } + doc.take_dirty(); + (doc, last.unwrap()) +} + +#[test] +fn an_edit_checks_only_what_it_changed_not_the_whole_document() { + // #1672: the check went over every layer of the file after every edit, so a batch slowed down + // with the size of the file, not of the edit. Count the layers it looks at instead of timing it. + let (big, id) = large(2000, 100); + let mut s = Session::with_document(big); + let before = s.doc().unwrap().clone(); + s.execute("node.set", &json!({"ids": [id], "props": {"name": "renamed"}})).unwrap(); + let checked = crate::session::finite_check_candidates(s.doc().unwrap(), &before); + assert!(checked.contains(&id), "the renamed layer is checked: {checked:?}"); + assert!(checked.len() <= 10, "a rename among 200,000 layers checks {} of them", checked.len()); +} + +#[test] +fn a_layer_already_out_of_range_does_not_block_edits_to_others() { + // A damaged file can hold a layer with a non-finite size; edits to other layers still go + // through, and an edit that touches the damaged layer is refused. + let (mut doc, id) = large(2, 3); + let page = doc.first_page().unwrap(); + let broken = doc.create(page, figcraft_doc::NodeKind::Rectangle, "broken", 0.0, 0.0, 10.0, 10.0).unwrap(); + doc.node_mut(broken).unwrap().width = f64::NAN; + doc.take_dirty(); + let mut s = Session::with_document(doc); + assert!(s.execute("node.set", &json!({"ids": [id], "props": {"name": "renamed"}})).is_ok()); + let moved = s.execute("transform.move", &json!({"ids": [broken], "dx": 5, "dy": 0})); + let err = moved.expect_err("an edit of the broken layer is refused").to_string(); + assert!(err.contains("broken"), "{err}"); +} diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index 99101dc2b..113b75cdb 100644 --- a/crates/engine/src/session.rs +++ b/crates/engine/src/session.rs @@ -10,5 +10,7 @@ use super::{ mod access; pub use access::Forward; mod exec; +#[cfg(test)] +pub(crate) use exec::finite_check_candidates; mod new; mod undo; diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index 050244813..097f44c78 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -16,6 +16,13 @@ fn command_carries_import_bytes(id: &str, params: &Value) -> bool { } } +/// The layers `check_finite_geometry` looks at: only those the command added or changed. The diff +/// skips what the two versions share, so the check costs about the size of the edit, not of the +/// document (#1672). +pub(crate) fn finite_check_candidates(doc: &Document, before: &Document) -> Vec { + doc.changed_ids(before, usize::MAX).unwrap_or_default() +} + impl Session { /// Append a live event with the active document's identity (nothing without one). Inside a /// [`Session::grouped`] call the event is buffered and committed only when the group succeeds. @@ -179,7 +186,7 @@ impl Session { } let broken = |n: &Node| !(n.transform.as_coeffs().iter().all(|c| c.is_finite()) && n.width.is_finite() && n.height.is_finite()); let untouched = |n: &Node| bdoc.node_arc(n.id).zip(ds.doc.node_arc(n.id)).is_some_and(|(a, b)| Arc::ptr_eq(a, b)); - match ds.doc.nodes().find(|n| broken(n) && !untouched(n)) { + match finite_check_candidates(&ds.doc, bdoc).iter().filter_map(|i| ds.doc.node(*i)).find(|n| broken(n) && !untouched(n)) { Some(n) => Err(bad(id, format!("\"{}\" would get a position or size beyond the number range", n.name))), None => Ok(()), }