From dc841f854805fabefbf4d4e102f4006c01e08ef2 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Sun, 8 Nov 2026 14:44:47 -0500 Subject: [PATCH] M8: vector.edit.position moves points in one pass; refusal, empty-selection and scrub tests The handler grouped equal offsets by a linear scan per point, quadratic when every point moves by its own offset. Only a single move covering every point carries the regions' paint frames, so it now checks in one pass whether all offsets are equal: if so the points move as one, otherwise each moves by its own offset. The result is the same as before. Tests: booleans, a valid x with a refused y, and the overflow case are refused with no undo step; nothing selected is refused; a shared Y scrubs every point in one undo step and a Mixed X doesn't scrub. selection_position's doc comment names its one user, vector.edit.bounds. Co-Authored-By: Claude Opus 5.5 Model-Effort: Opus 5.5 drove and wrote --- crates/engine/src/canvas/vectoredit.rs | 25 ++++++++++--------- .../src/canvas/vectoredit/position_tests.rs | 23 +++++++++++++++++ crates/ui-egui/src/field_tests.rs | 4 +-- crates/ui-egui/src/vector_point_xy_tests.rs | 23 +++++++++++++++-- 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/canvas/vectoredit.rs b/crates/engine/src/canvas/vectoredit.rs index 47597ea92..db4dfc099 100644 --- a/crates/engine/src/canvas/vectoredit.rs +++ b/crates/engine/src/canvas/vectoredit.rs @@ -762,7 +762,7 @@ pub fn selected_parent_points(doc: &Document, ve: &VectorEdit) -> Vec { selected_points(net, ve).into_iter().filter_map(|v| net.vertices.get(v)).map(|v| t * v.point()).collect() } -/// Selection bounds' top-left in the parent's coordinates (what the X/Y fields show). +/// Selection bounds' top-left in the parent's coordinates (the `x` and `y` of `vector.edit.bounds`). pub fn selection_position(doc: &Document, ve: &VectorEdit) -> Option { let net = network(doc, ve.id)?; let t = doc.node(ve.id)?.transform; @@ -1017,18 +1017,19 @@ pub fn specs() -> Vec { if moves.is_empty() { return Err(bad("vector.edit.position", "select points")); } - // Points with the same offset move together, so moving every point by one offset - // still carries the regions' paint frames, as a drag of all of them does. - let mut groups: Vec<(Vec2, Vec)> = Vec::new(); - for (i, d) in moves { - match groups.iter_mut().find(|(g, _)| (*g - d).hypot() < 1e-9) { - Some((_, v)) => v.push(i), - None => groups.push((d, vec![i])), - } - } + // Points that all share one offset move as one, so moving every point of a layer + // by one offset still carries the regions' paint frames, as a drag of all of them + // does. Otherwise no one move covers every point, so each point moves alone. + let d0 = moves[0].1; + let uniform = moves.iter().all(|(_, d)| (*d - d0).hypot() < 1e-9); edit(s.doc_mut()?, ve.id, |net| { - for (d, v) in &groups { - ops::move_vertices(net, v, *d); + if uniform { + let all: Vec = moves.iter().map(|(i, _)| *i).collect(); + ops::move_vertices(net, &all, d0); + } else { + for (i, d) in &moves { + ops::move_vertices(net, &[*i], *d); + } } })?; Ok(json!({"ok": true})) diff --git a/crates/engine/src/canvas/vectoredit/position_tests.rs b/crates/engine/src/canvas/vectoredit/position_tests.rs index fb6af9105..efbddab8d 100644 --- a/crates/engine/src/canvas/vectoredit/position_tests.rs +++ b/crates/engine/src/canvas/vectoredit/position_tests.rs @@ -19,6 +19,10 @@ fn parent_xy(s: &Session) -> Vec<(f64, f64)> { selected_parent_points(s.doc().unwrap(), &ve).iter().map(|p| (p.x, p.y)).collect() } +fn undo_steps(s: &Session) -> usize { + s.ds().unwrap().history.undo.len() +} + fn near(a: f64, b: f64) -> bool { (a - b).abs() < 1e-6 } @@ -132,6 +136,7 @@ fn junk_is_refused_and_changes_nothing() { let id = rect(&mut s); let before = xs(&s, id); ex(&mut s, "vector.edit.select", json!({"vertices": [0, 1]})); + let steps = undo_steps(&s); for junk in [ json!({"x": {"div": 0}}), json!({"x": {"pow": 2}}), @@ -140,9 +145,27 @@ fn junk_is_refused_and_changes_nothing() { json!({"y": {"add": "x"}}), json!({"x": {}}), json!({"x": [1]}), + json!({"x": true}), + json!({"x": {"add": true}}), + // A valid x with a refused y changes neither. + json!({"x": 5, "y": {"div": 0}}), + // Point 1's x of 100 overflows; point 0's 0 alone would not. json!({"x": {"mul": 1e308}}), ] { assert!(s.execute("vector.edit.position", &junk).is_err(), "{junk}"); } assert_eq!(xs(&s, id), before); + assert_eq!(undo_steps(&s), steps, "a refused edit leaves no undo step"); +} + +#[test] +fn nothing_selected_is_refused() { + let mut s = Session::new(); + let id = rect(&mut s); + ex(&mut s, "vector.edit.select", json!({"vertices": []})); + let (before, steps) = (xs(&s, id), undo_steps(&s)); + assert!(parent_xy(&s).is_empty()); + let err = s.execute("vector.edit.position", &json!({"x": 5})).unwrap_err(); + assert!(err.to_string().contains("select points"), "{err}"); + assert_eq!((xs(&s, id), undo_steps(&s)), (before, steps)); } diff --git a/crates/ui-egui/src/field_tests.rs b/crates/ui-egui/src/field_tests.rs index 2b1ead4aa..001187a91 100644 --- a/crates/ui-egui/src/field_tests.rs +++ b/crates/ui-egui/src/field_tests.rs @@ -234,7 +234,7 @@ fn geom(u: &Ui, n: NodeId) -> (f64, f64, f64, f64) { (b.x0, b.y0, b.width(), b.height()) } -fn undo_steps(u: &Ui) -> usize { +pub(crate) fn undo_steps(u: &Ui) -> usize { u.app.session.ds().unwrap().history.undo.len() } @@ -261,7 +261,7 @@ pub(crate) fn type_into(u: &mut Ui, name: &str, text: &str, end: Key) { } /// Scrub field `name` by its label: `dx` points with `modifiers` held, then let go. -fn scrub(u: &mut Ui, name: &str, dx: f32, modifiers: Modifiers) { +pub(crate) fn scrub(u: &mut Ui, name: &str, dx: f32, modifiers: Modifiers) { let from = label(u, name); drag(u, from, dx, modifiers); } diff --git a/crates/ui-egui/src/vector_point_xy_tests.rs b/crates/ui-egui/src/vector_point_xy_tests.rs index 07acb17da..e93f62873 100644 --- a/crates/ui-egui/src/vector_point_xy_tests.rs +++ b/crates/ui-egui/src/vector_point_xy_tests.rs @@ -1,12 +1,12 @@ //! The vector edit panel's Point section: X and Y show the value the selected points share, or //! "Mixed", and a typed value sets every selected point's own coordinate. -use egui::Key; +use egui::{Key, Modifiers}; use figcraft_doc::NodeId; use figcraft_engine::Session; use serde_json::json; -use crate::field_tests::{field, type_into}; +use crate::field_tests::{field, scrub, type_into, undo_steps}; use crate::layers_drag_tests::Ui; fn setup() -> (Ui, NodeId) { @@ -58,3 +58,22 @@ fn one_point_shows_its_own_value() { assert_eq!(field(&mut u, "X").1, format!("{}", p.0)); assert_eq!(field(&mut u, "Y").1, format!("{}", p.1)); } + +#[test] +fn a_shared_value_scrubs_every_point_in_one_step_and_mixed_does_not_scrub() { + let (mut u, id) = setup(); + // Points 0 and 1 share their y: a scrub moves both to the same new y, as one undo step. + u.app.session.execute("vector.edit.select", &json!({"vertices": [0, 1]})).unwrap(); + let (before, steps) = (xs(&u, id), undo_steps(&u)); + scrub(&mut u, "Y", 20.0, Modifiers::NONE); + let after = xs(&u, id); + assert!(after[0].1 > before[0].1, "the scrub moved y: {before:?} -> {after:?}"); + assert_eq!(after[0].1, after[1].1, "both points take the scrubbed value"); + assert_eq!((after[0].0, after[1].0), (before[0].0, before[1].0), "x is untouched"); + assert_eq!(undo_steps(&u), steps + 1, "one undo step for the whole scrub"); + // Their x differs (Mixed): there is no value to scrub from, so nothing moves. + assert_eq!(field(&mut u, "X").1, "Mixed"); + let (before, steps) = (xs(&u, id), undo_steps(&u)); + scrub(&mut u, "X", 20.0, Modifiers::NONE); + assert_eq!((xs(&u, id), undo_steps(&u)), (before, steps)); +}