From 6fcc4e38d56c6c94f89e413f81ee5401558e615f Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sat, 11 Jul 2026 09:46:28 +0800 Subject: [PATCH] perf(editor): single-pass translate_selected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each selected node cost three full-tree walks (flex-flow check, ancestor-dedup, find) per nudge/drag frame — near-quadratic on select-all. One recursive walk now threads parent-flex and ancestor-in-set context down; equivalence with the old algorithm is pinned by a 10-case reference-implementation test suite. --- crates/op-editor-core/src/lib.rs | 2 + crates/op-editor-core/src/mutators.rs | 38 +- .../src/translate_equivalence_tests.rs | 367 ++++++++++++++++++ crates/op-editor-core/src/walkers.rs | 72 ++++ 4 files changed, 460 insertions(+), 19 deletions(-) create mode 100644 crates/op-editor-core/src/translate_equivalence_tests.rs diff --git a/crates/op-editor-core/src/lib.rs b/crates/op-editor-core/src/lib.rs index 1ce8ec2d9..8a04996b6 100644 --- a/crates/op-editor-core/src/lib.rs +++ b/crates/op-editor-core/src/lib.rs @@ -155,6 +155,8 @@ mod tests_geometry; mod tests_mutators; #[cfg(test)] mod tests_pages; +#[cfg(test)] +mod translate_equivalence_tests; pub use agent_settings::{ AcpAgentConfig, AcpAgentConnectOutcome, AcpAgentConnectPhase, AcpAgentConnection, diff --git a/crates/op-editor-core/src/mutators.rs b/crates/op-editor-core/src/mutators.rs index d87426556..3484e9eea 100644 --- a/crates/op-editor-core/src/mutators.rs +++ b/crates/op-editor-core/src/mutators.rs @@ -393,11 +393,27 @@ impl EditorState { /// Translate every node in the selection set by `(dx, dy)` doc /// px. Containers carry their subtree (child coords are parent- /// relative); an ancestor-already-in-set dedup stops descendants - /// shifting twice. + /// shifting twice. A child of an auto-layout (flex) parent is + /// engine-positioned; materializing x/y here flips it to + /// `Position::Absolute` in jian-core and detaches it from flex + /// flow, so such a node is skipped (a free drag of it is a no-op). + /// + /// Single recursive walk over `active_children` (Task 10): + /// previously this ran three full-tree walks (`is_flow_child_of_flex` + /// / `is_ancestor_in_set` / `find_node_mut`) PER selected id — + /// O(|selection| × nodes), near-quadratic on select-all. The editable + /// set is collected once into a `HashSet<&str>` and + /// [`walkers::translate_editable_subtree`] threads both skip + /// conditions down the recursion as it descends, so the whole tree + /// is visited exactly once regardless of selection size. pub fn translate_selected(&mut self, dx: f64, dy: f64) -> bool { if self.selection.set.is_empty() || (dx == 0.0 && dy == 0.0) { return false; } + // Owned, not borrowed from `self.selection` — the `HashSet<&str>` + // built below borrows these local `NodeId`s instead, so it can + // coexist with the `&mut self` borrow `active_children_mut()` + // needs. let editable: Vec = self .selection .set @@ -408,25 +424,9 @@ impl EditorState { if editable.is_empty() { return false; } + let editable_set: HashSet<&str> = editable.iter().map(NodeId::as_str).collect(); let children = self.active_children_mut(); - let mut moved = false; - for target in &editable { - // A child of an auto-layout (flex) parent is engine-positioned; - // materializing x/y here flips it to Position::Absolute in - // jian-core and detaches it from flex flow. A free drag of - // such a node is a no-op (it cannot move independently of the - // layout engine). Checked before the ancestor dedup. - if walkers::is_flow_child_of_flex(children, target) { - continue; - } - if !walkers::is_ancestor_in_set(children, target, &editable) { - if let Some(node) = find_node_mut(children, target) { - walkers::translate_subtree(node, dx, dy); - moved = true; - } - } - } - moved + walkers::translate_editable_subtree(children, &editable_set, dx, dy, false, false) } // --- Tree ops ---------------------------------------------------- diff --git a/crates/op-editor-core/src/translate_equivalence_tests.rs b/crates/op-editor-core/src/translate_equivalence_tests.rs new file mode 100644 index 000000000..407b3c2b0 --- /dev/null +++ b/crates/op-editor-core/src/translate_equivalence_tests.rs @@ -0,0 +1,367 @@ +//! Equivalence oracle for Task 10's `translate_selected` rewrite. +//! +//! `translate_selected` used to run three full-tree walks PER selected +//! id (`is_flow_child_of_flex` / `is_ancestor_in_set` / `find_node_mut`) +//! — O(|selection| × nodes), near-quadratic on select-all. It was +//! replaced with a single recursive walk +//! ([`walkers::translate_editable_subtree`]) that threads both skip +//! conditions down the recursion instead of recomputing them from +//! scratch per id. +//! +//! [`reference_translate_selected`] below is a byte-for-byte port of the +//! OLD three-walk body, kept here ONLY as a test oracle (production code +//! never calls it). Every case constructs a document, clones the +//! `EditorState`, runs the reference implementation on one clone and +//! the live `EditorState::translate_selected` on the other with an +//! IDENTICAL selection + delta, and asserts the resulting `doc`s are +//! byte-for-byte equal. + +#![cfg(test)] + +use crate::node_id::NodeId; +use crate::pen_node_ext::PenNodeExt; +use crate::state::EditorState; +use crate::test_support::{flex_frame, flow_rect, frame, group, rect, state_with}; +use crate::walkers::{self, find_node_mut}; +use jian_ops_schema::node::PenNode; +use jian_ops_schema::page::PenPage; + +/// Byte-for-byte port of `translate_selected`'s pre-Task-10 body — three +/// full-tree walks per selected id. See the module doc: this exists +/// ONLY to pin down old behavior as an equivalence oracle. +fn reference_translate_selected(state: &mut EditorState, dx: f64, dy: f64) -> bool { + if state.selection.set.is_empty() || (dx == 0.0 && dy == 0.0) { + return false; + } + let editable: Vec = state + .selection + .set + .iter() + .filter(|id| state.is_editable(id)) + .cloned() + .collect(); + if editable.is_empty() { + return false; + } + let children = state.active_children_mut(); + let mut moved = false; + for target in &editable { + if walkers::is_flow_child_of_flex(children, target) { + continue; + } + if !walkers::is_ancestor_in_set(children, target, &editable) { + if let Some(node) = find_node_mut(children, target) { + walkers::translate_subtree(node, dx, dy); + moved = true; + } + } + } + moved +} + +/// Force every container-capable node's `children` field to `Some(_)` +/// up front (recursively). This exists ONLY to neutralize an +/// orthogonal, pre-existing quirk of the SHARED `find_node_mut` walker +/// (used by the old reference body): its search calls +/// `PenNodeExt::children_mut()` on every non-matching node it visits +/// while looking for a target, and `children_mut()` eagerly upgrades +/// a container-capable node's `children: None` to `Some(vec![])` +/// (`Option::get_or_insert_with`) as a side effect — even for nodes +/// that turn out to be unrelated to the search. That upgrade depends +/// on find_node_mut's data-dependent visitation order (which siblings +/// it happens to scan before finding a match), not on the actual +/// translate/skip/dedup semantics this test exists to pin down. The +/// single-pass rewrite deliberately visits every node once during its +/// own descent (see `translate_editable_subtree`), so without this +/// normalization the two implementations would disagree on which +/// untouched leaves flip from `None` to `Some(vec![])` — a distinction +/// that carries no geometric meaning (an absent vs. empty children +/// list behaves identically everywhere else) but would still trip +/// `PenDocument`'s derived `PartialEq`. Normalizing BOTH clones to the +/// same starting shape keeps the comparison scoped to what actually +/// matters: node positions and the `moved` verdict. +fn normalize_children(nodes: &mut [PenNode]) { + for node in nodes.iter_mut() { + if let Some(children) = node.children_mut() { + normalize_children(children); + } + } +} + +/// Runs both implementations from identical clones of `state` with an +/// identical selection + delta, then asserts they land on the same +/// document AND the same `moved` verdict. +fn assert_equivalent(mut state: EditorState, selected: &[&str], dx: f64, dy: f64) { + state.selection.set = selected.iter().map(|id| NodeId::new(*id)).collect(); + state.selection.anchor = state.selection.set.last().cloned().unwrap_or(NodeId::NONE); + normalize_children(&mut state.doc.children); + if let Some(pages) = state.doc.pages.as_mut() { + for page in pages.iter_mut() { + normalize_children(&mut page.children); + } + } + + let mut reference = state.clone(); + let mut actual = state; + + let ref_moved = reference_translate_selected(&mut reference, dx, dy); + let actual_moved = actual.translate_selected(dx, dy); + + assert_eq!( + ref_moved, actual_moved, + "moved verdict diverged for selection {selected:?}" + ); + assert_eq!( + reference.doc, actual.doc, + "resulting document diverged for selection {selected:?}" + ); +} + +#[test] +fn equivalence_single_leaf() { + assert_equivalent( + state_with(vec![rect("n1", "A", 10.0, 10.0, 50.0, 50.0)]), + &["n1"], + 7.0, + 3.0, + ); +} + +#[test] +fn equivalence_nested_containers_ancestor_and_descendant_dedup() { + // n1 (frame) > n2 (frame) > n3 (rect) — select all three. The + // ancestor dedup must fire twice: n2 is skipped because n1 (its + // ancestor) is selected, n3 is skipped because n2 (its ancestor, + // even though itself skipped) is selected. + let doc = frame( + "n1", + "Outer", + 0.0, + 0.0, + 200.0, + 200.0, + vec![frame( + "n2", + "Inner", + 10.0, + 10.0, + 100.0, + 100.0, + vec![rect("n3", "Leaf", 5.0, 5.0, 20.0, 20.0)], + )], + ); + assert_equivalent(state_with(vec![doc]), &["n1", "n2", "n3"], 5.0, -4.0); +} + +#[test] +fn equivalence_descendant_only_no_ancestor_selected() { + // Same tree, but only the deepest leaf is selected — no dedup + // applies, the leaf itself must translate. + let doc = frame( + "n1", + "Outer", + 0.0, + 0.0, + 200.0, + 200.0, + vec![frame( + "n2", + "Inner", + 10.0, + 10.0, + 100.0, + 100.0, + vec![rect("n3", "Leaf", 5.0, 5.0, 20.0, 20.0)], + )], + ); + assert_equivalent(state_with(vec![doc]), &["n3"], 2.0, 2.0); +} + +#[test] +fn equivalence_flex_parent_and_flow_children() { + // A flex frame plus its two flow children, with EVERY id selected + // at once: the frame moves (top-level, no ancestor), the flow + // children are skipped because their immediate parent is flex. + let doc = flex_frame( + "f1", + "Flex", + 100.0, + 100.0, + 200.0, + 300.0, + vec![ + flow_rect("c1", "A", 80.0, 24.0), + flow_rect("c2", "B", 80.0, 24.0), + ], + ); + assert_equivalent(state_with(vec![doc]), &["f1", "c1", "c2"], 5.0, 7.0); +} + +#[test] +fn equivalence_flex_child_selected_alone() { + let doc = flex_frame( + "f1", + "Flex", + 0.0, + 0.0, + 200.0, + 300.0, + vec![flow_rect("c1", "A", 80.0, 24.0)], + ); + assert_equivalent(state_with(vec![doc]), &["c1"], 9.0, 11.0); +} + +#[test] +fn equivalence_locked_node_in_selection_is_excluded() { + let mut locked = rect("n2", "Locked", 60.0, 60.0, 30.0, 30.0); + locked.base_mut().locked = Some(true); + let doc = frame( + "n1", + "Frame", + 0.0, + 0.0, + 200.0, + 200.0, + vec![rect("n3", "Free", 10.0, 10.0, 20.0, 20.0), locked], + ); + // n1 stays free-standing (not selected) so n3's translate isn't + // deduped; n2 is locked and must be excluded from the editable set + // entirely (no translate, no dedup contribution). + assert_equivalent(state_with(vec![doc]), &["n2", "n3"], 4.0, -6.0); +} + +#[test] +fn equivalence_hidden_ancestor_does_not_dedupe_a_selected_child() { + // The ancestor is selected but HIDDEN — `is_editable` excludes it + // from the editable set, so it must not count as an "ancestor in + // set" for its selected, visible child either. + let mut hidden_parent = frame( + "n1", + "Hidden", + 0.0, + 0.0, + 200.0, + 200.0, + vec![rect("n2", "Child", 10.0, 10.0, 20.0, 20.0)], + ); + hidden_parent.base_mut().visible = Some(false); + assert_equivalent(state_with(vec![hidden_parent]), &["n1", "n2"], 3.0, 3.0); +} + +#[test] +fn equivalence_overlapping_selection_across_disjoint_branches() { + // Two independent subtrees, each with an ancestor+descendant pair + // selected, plus one lone top-level leaf. Exercises dedup running + // independently per branch within a single pass. + let branch_a = frame( + "a1", + "A", + 0.0, + 0.0, + 100.0, + 100.0, + vec![rect("a2", "AChild", 5.0, 5.0, 10.0, 10.0)], + ); + let branch_b = group( + "b1", + "B", + vec![ + rect("b2", "BChild1", 5.0, 5.0, 10.0, 10.0), + rect("b3", "BChild2", 20.0, 20.0, 10.0, 10.0), + ], + ); + let leaf = rect("c1", "Lone", 300.0, 300.0, 40.0, 40.0); + assert_equivalent( + state_with(vec![branch_a, branch_b, leaf]), + &["a1", "a2", "b1", "b3", "c1"], + -3.0, + 8.0, + ); +} + +#[test] +fn equivalence_large_multi_selection_select_all() { + // A wide, moderately deep forest with every node selected at once — + // the near-quadratic case the single-pass rewrite targets. Mixes + // top-level leaves, nested containers with multiple children each, + // and one locked leaf to keep the editable-set filter exercised. + let mut roots = Vec::new(); + let mut all_ids: Vec = Vec::new(); + for i in 0..12u32 { + let root_id = format!("root{i}"); + all_ids.push(root_id.clone()); + let mut mid_children = Vec::new(); + for j in 0..6u32 { + let mid_id = format!("mid{i}_{j}"); + all_ids.push(mid_id.clone()); + let mut leaves = Vec::new(); + for k in 0..4u32 { + let leaf_id = format!("leaf{i}_{j}_{k}"); + all_ids.push(leaf_id.clone()); + let mut leaf = rect(&leaf_id, "Leaf", k as f64, k as f64, 8.0, 8.0); + // Lock exactly one leaf per mid-container so the editable + // filter has real work to do at scale. + if k == 3 { + leaf.base_mut().locked = Some(true); + } + leaves.push(leaf); + } + mid_children.push(frame( + &mid_id, + "Mid", + j as f64 * 10.0, + j as f64 * 10.0, + 60.0, + 60.0, + leaves, + )); + } + roots.push(frame( + &root_id, + "Root", + i as f64 * 100.0, + 0.0, + 300.0, + 300.0, + mid_children, + )); + } + let selected: Vec<&str> = all_ids.iter().map(String::as_str).collect(); + assert_equivalent(state_with(roots), &selected, 11.0, -13.0); +} + +#[test] +fn equivalence_multi_page_document_only_touches_the_active_page() { + // A second, inactive page carries an id that COLLIDES with nothing + // on the active page but would corrupt the result if either + // implementation accidentally walked `doc.pages` instead of the + // active page's children. + let mut state = state_with(vec![]); + state.doc.children.clear(); + state.doc.pages = Some(vec![ + PenPage { + id: "p0".to_string(), + name: "Page 0".to_string(), + children: vec![rect("x1", "Other page leaf", 0.0, 0.0, 10.0, 10.0)], + state: None, + lifecycle: None, + }, + PenPage { + id: "p1".to_string(), + name: "Page 1".to_string(), + children: vec![frame( + "n1", + "Active page frame", + 20.0, + 20.0, + 100.0, + 100.0, + vec![rect("n2", "Active page child", 5.0, 5.0, 10.0, 10.0)], + )], + state: None, + lifecycle: None, + }, + ]); + state.ui.active_page_index = 1; + assert_equivalent(state, &["n1", "n2"], 6.0, 2.0); +} diff --git a/crates/op-editor-core/src/walkers.rs b/crates/op-editor-core/src/walkers.rs index 93f336216..4182fddd8 100644 --- a/crates/op-editor-core/src/walkers.rs +++ b/crates/op-editor-core/src/walkers.rs @@ -433,6 +433,78 @@ pub fn is_ancestor_in_set(children: &[PenNode], target: &NodeId, set: &[NodeId]) false } +/// Single-pass replacement for the old `is_flow_child_of_flex` + +/// `is_ancestor_in_set` + `find_node_mut` triple (one full-tree walk +/// PER selected id). This descends the forest exactly once, carrying +/// two contexts down the recursion instead of recomputing them from +/// scratch for every id: +/// +/// - `parent_is_flex` — whether the CURRENT node's immediate parent is +/// an auto-layout (flex) container. Mirrors `is_flow_child_of_flex`, +/// which only cares about the immediate parent, not any ancestor — +/// so this is recomputed fresh (from the node just visited) at each +/// recursion level, never accumulated. +/// - `ancestor_in_set` — whether ANY proper ancestor's id is in +/// `editable`. Mirrors `is_ancestor_in_set`'s top-down ancestor-chain +/// search. Accumulates via OR as the recursion descends, and is +/// evaluated BEFORE folding in the current node's own membership (a +/// node is never its own ancestor). +/// +/// A node translates iff its id is in `editable` AND neither guard +/// applies — matching `translate_selected`'s per-id skip order (flex +/// check first, then the ancestor dedup) exactly. `editable` already +/// reflects the `is_editable` pre-filter (hidden / locked ids excluded +/// by the caller), so this walk never re-checks visibility / lock +/// state itself. +/// +/// Recursion is gated on the IMMUTABLE [`PenNodeExt::children`] check +/// first, only reaching for [`PenNodeExt::children_mut`] when that +/// already reports `Some`. `children_mut` eagerly upgrades a +/// container-capable node's `children: None` to `Some(vec![])` +/// (`Option::get_or_insert_with`) — harmless for the few nodes +/// `find_node_mut`'s per-id search happened to pass over in the old +/// three-walk body, but this walk visits every node in the forest +/// exactly once, so calling `children_mut` unconditionally here would +/// silently materialize an empty `children` list on every leaf +/// Rectangle/Frame in the document on every drag frame — bloating the +/// in-memory doc and any subsequent `.op` serialization. A node with +/// no children has nothing left to translate inside it either way, so +/// skipping it is both cheaper and correct. +pub fn translate_editable_subtree( + children: &mut [PenNode], + editable: &HashSet<&str>, + dx: f64, + dy: f64, + parent_is_flex: bool, + ancestor_in_set: bool, +) -> bool { + let mut moved = false; + for child in children.iter_mut() { + let in_set = editable.contains(child.id_str()); + if in_set && !parent_is_flex && !ancestor_in_set { + translate_subtree(child, dx, dy); + moved = true; + } + let child_is_flex = child.is_auto_layout_container(); + let child_ancestor_in_set = ancestor_in_set || in_set; + if child.children().is_some() { + if let Some(grand) = child.children_mut() { + if translate_editable_subtree( + grand, + editable, + dx, + dy, + child_is_flex, + child_ancestor_in_set, + ) { + moved = true; + } + } + } + } + moved +} + /// First duplicate id found in the forest, or `None` when all ids /// are unique. pub fn find_duplicate(children: &[PenNode], seen: &mut HashSet) -> Option {