From 91a32730ef88214d3a9a3e9beac3548274ff1de9 Mon Sep 17 00:00:00 2001 From: harrisonoconnorhover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Fri, 16 Oct 2026 08:02:23 -0400 Subject: [PATCH] fix: allow deleting parent pages (#956) --- ROADMAP.md | 4 +- crates/doc/src/build.rs | 49 ++++++- crates/engine/src/cmd/layout.rs | 25 ++++ crates/engine/src/cmd/parent_delete_tests.rs | 127 +++++++++++++++++++ crates/ui-egui/src/panels/pages.rs | 42 +++++- docs/agents.md | 9 ++ 6 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 crates/engine/src/cmd/parent_delete_tests.rs diff --git a/ROADMAP.md b/ROADMAP.md index 2aacdaf9..fa42b4c2 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -214,7 +214,7 @@ same references), and from the app's JSON control channel. See [docs/agents.md]( | # | Milestone | State | **Stage: pre-alpha** · next: alpha, ~0% and ~15–25 h away; beta ~28 points and ~420–750 h beyond that (the core-workflow gate, not the percentage: ready for real work is already ~47%) -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** minor (text threads by hand; the alpha gate waits on Primary Text Frame page fixes) · **Target:** Adobe InDesign 2026 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-16 · **Change:** minor (undoable parent deletion) · **Target:** Adobe InDesign 2026 > **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-13 · **Change:** minor (parent canvas refresh) · **Target:** Adobe InDesign 2026 > **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-13 · **Change:** minor (bounded control click/drag input) · **Target:** Adobe InDesign 2026 > **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** minor (text stroke behind fill) · **Target:** Adobe InDesign 2026 @@ -343,6 +343,8 @@ list) · [roadmap.md](docs/roadmap.md) (milestones, plan) · [parity-checklist.m ## Progress log +- **2026-10-16** · Parents can be deleted from their Pages-panel context menu or `layout.parents.delete {id}`. Applied-page and based-on references are cleared, local override copies survive, and Undo restores the parent. Addresses the deletion portion of #956; no readiness percentage change. + - **2026-10-13** · Switching between document and parent pages refreshes the canvas immediately, preventing black parent pages and stale document objects when the zoom stays the same. Ordinary edits retain partial repainting. No readiness percentage change. Newest first. diff --git a/crates/doc/src/build.rs b/crates/doc/src/build.rs index 9fb07555..43e0c7be 100644 --- a/crates/doc/src/build.rs +++ b/crates/doc/src/build.rs @@ -1,7 +1,7 @@ //! Creating documents (File → New → Document), presets, and page/spread structure edits //! (insert/delete/move pages, repagination into spreads, parents). -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::sync::Arc; use designcraft_geom::{Affine, Unit, units}; @@ -251,6 +251,53 @@ impl Document { id } + /// Delete a parent spread and unapply it wherever it is used. Local overrides survive as + /// ordinary items; the deleted parent's own items leave their text threads normally. + pub fn delete_parent(&mut self, id: SpreadId) -> Result<()> { + let index = self.parent_index(id).ok_or_else(|| DocError::Invalid(format!("no parent {id}")))?; + let parent = self.parents.get(index).ok_or_else(|| DocError::Invalid(format!("no parent {id}")))?; + let roots: Vec<_> = parent.items.iter().map(|item| item.id).collect(); + let mut deleted = HashSet::new(); + for item in &parent.items { + item.walk(&mut |item| { + deleted.insert(item.id); + }); + } + for item in roots { + self.remove_item(item)?; + } + self.parents.remove(index); + + let mut detached = Vec::new(); + for spread in self.spreads.iter_mut().chain(self.parents.iter_mut()) { + let spread = Arc::make_mut(spread); + if let Some(parent) = &mut spread.parent + && parent.based_on == Some(id) + { + parent.based_on = None; + } + for page in &mut spread.pages { + if page.parent == Some(id) { + page.parent = None; + } + page.overridden.retain(|item| !deleted.contains(item)); + } + for item in &spread.items { + item.walk(&mut |item| { + if item.overrides.is_some_and(|source| deleted.contains(&source)) { + detached.push(item.id); + } + }); + } + } + for item in detached { + if let Some(item) = self.item_mut(item) { + item.overrides = None; + } + } + Ok(()) + } + pub fn parent_index(&self, id: SpreadId) -> Option { self.parents.iter().position(|p| p.id == id) } diff --git a/crates/engine/src/cmd/layout.rs b/crates/engine/src/cmd/layout.rs index fe06ad3e..f1313dc4 100644 --- a/crates/engine/src/cmd/layout.rs +++ b/crates/engine/src/cmd/layout.rs @@ -246,6 +246,27 @@ pub fn specs() -> Vec { Ok(json!({"id": id.0, "prefix": prefix})) }) }), + cmd!( + "layout.parents.delete", + "Delete Parent", + [], + None, + "{id: parent spread id} — unapply the parent and keep local overrides as ordinary items", + has_doc, + |s, p| { + let id = p.get("id").and_then(Value::as_u64).map(SpreadId).ok_or_else(|| bad("layout.parents.delete", "missing parent id"))?; + s.edit(|d, sel| { + d.delete_parent(id)?; + *sel = Default::default(); + ok() + })?; + let st = s.doc_mut()?; + if st.doc.parents.is_empty() { + st.editing_parents = false; + } + ok() + } + ), cmd!(noundo "layout.parents.edit", "Edit Parents", [], None, "{on: bool} — show parent spreads on the canvas", has_doc, |s, p| { let st = s.doc_mut()?; st.editing_parents = p.get("on").and_then(Value::as_bool).unwrap_or(!st.editing_parents); @@ -1264,3 +1285,7 @@ mod spread_rotation_input_tests { assert_eq!(rotation(&s), 0, "zero resets the spread view"); } } + +#[cfg(test)] +#[path = "parent_delete_tests.rs"] +mod parent_delete_tests; diff --git a/crates/engine/src/cmd/parent_delete_tests.rs b/crates/engine/src/cmd/parent_delete_tests.rs new file mode 100644 index 00000000..0bfd89e7 --- /dev/null +++ b/crates/engine/src/cmd/parent_delete_tests.rs @@ -0,0 +1,127 @@ +//! Regression coverage for deleting parent spreads through the command boundary. + +use std::sync::Arc; + +use designcraft_doc::{ItemId, Selection, SpreadId, TextSel}; +use serde_json::{Value, json}; + +use crate::Session; + +fn frame(s: &mut Session, parent: usize, text: Option<&str>) -> ItemId { + let mut p = json!({"spread": {"kind": "parent", "index": parent}, "rect": [36, 36, 200, 100]}); + if let Some(text) = text { + p["content"] = json!("text"); + p["text"] = json!(text); + } + ItemId(s.execute("frame.create", &p).unwrap()["id"].as_u64().unwrap()) +} + +#[test] +fn parent_delete_rejects_invalid_ids_without_editing_or_adding_history() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + s.execute("layout.parents.edit", &json!({"on": true})).unwrap(); + let before = s.doc().unwrap().doc.clone(); + let history = s.doc().unwrap().history.undo.len(); + for params in [json!({}), json!({"id": null}), json!({"id": "A"}), json!({"id": -1}), json!({"id": 999999})] { + assert!(s.execute("layout.parents.delete", ¶ms).is_err(), "{params}"); + let st = s.doc().unwrap(); + assert!(Arc::ptr_eq(&st.doc, &before)); + assert_eq!(st.history.undo.len(), history); + assert!(st.editing_parents); + } +} + +#[test] +fn parent_delete_preserves_local_copies_cleans_references_and_roundtrips_undo() { + let mut s = Session::new(); + s.execute("file.new", &json!({"pages": 3, "facingPages": false})).unwrap(); + let a = s.doc().unwrap().doc.parents[0].id; + let text = frame(&mut s, 0, Some("Parent folio")); + let story = s.doc().unwrap().doc.item(text).unwrap().text_frame().unwrap().story; + let shape = frame(&mut s, 0, None); + let group = ItemId(s.execute("object.group", &json!({"ids": [text.0, shape.0]})).unwrap()["id"].as_u64().unwrap()); + let local = ItemId(s.execute("layout.overrideParentItems", &json!({"page": 0})).unwrap()["ids"][0].as_u64().unwrap()); + let local_text = s.doc().unwrap().doc.item(local).unwrap().children().iter().find(|i| i.text_frame().is_some()).unwrap().id; + let local_story = s.doc().unwrap().doc.item(local_text).unwrap().text_frame().unwrap().story; + assert_ne!(story, local_story); + + let b = SpreadId(s.execute("layout.parents.new", &json!({"prefix": "B"})).unwrap()["id"].as_u64().unwrap()); + let c = SpreadId(s.execute("layout.parents.new", &json!({"prefix": "C"})).unwrap()["id"].as_u64().unwrap()); + let inherited = frame(&mut s, 1, Some("Local parent edit")); + s.edit(|d, _| { + // Imported parents can base themselves on another parent and carry local overrides. + let bp = Arc::make_mut(&mut d.parents[1]); + bp.parent.as_mut().unwrap().based_on = Some(a); + bp.pages[0].parent = Some(a); + bp.pages[0].overridden.push(text); + Arc::make_mut(&mut d.parents[2]).parent.as_mut().unwrap().based_on = Some(b); + d.item_mut(inherited).unwrap().overrides = Some(text); + Ok(()) + }) + .unwrap(); + s.execute("layout.pages.applyParent", &json!({"pages": [1], "parent": "B"})).unwrap(); + let other_local = ItemId(s.execute("layout.overrideParentItems", &json!({"page": 1})).unwrap()["ids"][0].as_u64().unwrap()); + s.execute("layout.parents.edit", &json!({"on": true})).unwrap(); + s.doc_mut().unwrap().selection = Selection::text(TextSel::caret(story, 0)); + let before = s.doc().unwrap().doc.clone(); + let history = s.doc().unwrap().history.undo.len(); + before.check().unwrap(); + + s.execute("layout.parents.delete", &json!({"id": a.0})).unwrap(); + let st = s.doc().unwrap(); + assert_eq!(st.history.undo.len(), history + 1); + assert_eq!(st.selection, Selection::default()); + assert!(st.editing_parents, "other parents remain editable"); + let d = &st.doc; + assert_eq!(d.parents.iter().map(|p| p.id).collect::>(), [b, c]); + assert_eq!(d.page(0).unwrap().parent, None); + assert_eq!(d.page(1).unwrap().parent, Some(b)); + assert_eq!(d.page(2).unwrap().parent, None); + assert_eq!(d.parents[0].parent.as_ref().unwrap().based_on, None); + assert_eq!(d.parents[1].parent.as_ref().unwrap().based_on, Some(b)); + assert_eq!(d.parents[0].pages[0].parent, None); + assert!(d.parents[0].pages[0].overridden.is_empty()); + assert!([group, text, shape].iter().all(|id| d.item(*id).is_none())); + assert!(d.story(story).is_none(), "nested text frames leave no dangling story"); + assert_eq!(d.item(local).unwrap().overrides, None); + assert_eq!(d.story(local_story).unwrap().text, "Parent folio"); + assert!(d.page(0).unwrap().overridden.is_empty()); + assert_eq!(d.item(inherited).unwrap().overrides, None); + assert_eq!(d.item(other_local).unwrap().overrides, Some(inherited)); + assert_eq!(d.page(1).unwrap().overridden, [inherited]); + d.check().unwrap(); + let after = d.clone(); + + s.execute("edit.undo", &Value::Null).unwrap(); + assert_eq!(s.doc().unwrap().doc, before); + s.doc().unwrap().doc.check().unwrap(); + s.execute("edit.redo", &Value::Null).unwrap(); + assert_eq!(s.doc().unwrap().doc, after); + s.doc().unwrap().doc.check().unwrap(); +} + +#[test] +fn parent_delete_last_parent_returns_to_document_pages() { + let mut s = Session::new(); + s.execute("file.new", &json!({"pages": 2})).unwrap(); + let id = s.doc().unwrap().doc.parents[0].id; + s.execute("layout.parents.edit", &json!({"on": true})).unwrap(); + frame(&mut s, 0, Some("Last parent")); + s.execute("layout.parents.delete", &json!({"id": id.0})).unwrap(); + let st = s.doc().unwrap(); + assert!(st.doc.parents.is_empty()); + assert!(!st.editing_parents); + assert_eq!(st.doc.page_count(), 2); + assert!(st.doc.spreads.iter().flat_map(|sp| &sp.pages).all(|page| page.parent.is_none())); + s.execute("edit.undo", &Value::Null).unwrap(); + assert_eq!(s.doc().unwrap().doc.parents[0].id, id); + assert_eq!(s.doc().unwrap().selection, Selection::default()); + s.execute("edit.redo", &Value::Null).unwrap(); + assert!(s.doc().unwrap().doc.parents.is_empty()); + assert!(!s.doc().unwrap().editing_parents); + // A parent-free document still supports creating another parent. + s.execute("layout.parents.new", &json!({"prefix": "B"})).unwrap(); + assert_eq!(s.doc().unwrap().doc.parents.len(), 1); + s.doc().unwrap().doc.check().unwrap(); +} diff --git a/crates/ui-egui/src/panels/pages.rs b/crates/ui-egui/src/panels/pages.rs index 2d6d56da..c3d60b07 100644 --- a/crates/ui-egui/src/panels/pages.rs +++ b/crates/ui-egui/src/panels/pages.rs @@ -81,12 +81,12 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { let mut drop: Option<(PageDrag, usize)> = None; ui.spacing_mut().item_spacing.y = 0.0; // ---- Parents: [None] and each parent, 29 pt rows with a blank thumbnail at the right. - let mut rows: Vec<(String, Option, usize)> = vec![("[None]".into(), None, 1)]; + let mut rows = vec![("[None]".to_string(), None, 1, None)]; for p in &doc.parents { let label = p.parent.as_ref().map(|i| i.label()).unwrap_or_default(); - rows.push((label, p.parent.as_ref().map(|i| i.prefix.clone()), p.pages.len())); + rows.push((label, p.parent.as_ref().map(|i| i.prefix.clone()), p.pages.len(), Some(p.id))); } - for (label, prefix, n) in rows { + for (label, prefix, n, id) in rows { let (r, resp) = ui.allocate_exact_size(vec2(ui.available_width(), ROW_H), Sense::click_and_drag()); let active = editing_parents && prefix.is_some(); if active { @@ -104,6 +104,17 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { if resp.dragged() { drag_ghost(ui, &label, &t); } + if let Some(id) = id { + resp.context_menu(|ui| { + let label = crate::i18n::tr(&app.ui.language, "Delete Parent"); + if ui.button(crate::rtl::widget(ui, label)).clicked() { + if app.run("layout.parents.delete", json!({"id": id.0})).is_ok() { + app.views.clear(); + } + ui.close(); + } + }); + } } ui.add_space(4.0); let r = ui.available_rect_before_wrap(); @@ -424,6 +435,31 @@ mod tests { h } + #[test] + fn parent_context_menu_deletes_the_clicked_id_even_with_a_shared_prefix() { + let mut app = DesignApp::new(designcraft_engine::Session::new(), crate::Services::default()); + app.run("file.new", json!({"pages": 2, "facingPages": false})).unwrap(); + let original = app.session.doc().unwrap().doc.parents[0].id; + let added = app.run("layout.parents.new", json!({"prefix": "A", "name": "Other"})).unwrap()["id"].as_u64().unwrap(); + let mut h = harness(app); + // [None], A-Parent, then A-Other: target the actual row, not a menu helper. + let at = egui::pos2(40.0, ROW_H * 2.5); + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Secondary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Secondary, pressed: false, modifiers: egui::Modifiers::NONE }); + h.run_steps(3); + h.get_by_label("Delete Parent").click(); + h.run_steps(3); + let doc = &h.state().session.doc().unwrap().doc; + assert_eq!(doc.parents.len(), 1); + assert_eq!(doc.parents[0].id, original); + assert!(doc.parents.iter().all(|p| p.id.0 != added)); + assert_eq!(doc.page_count(), 2, "document pages are not deleted"); + h.state_mut().run("edit.undo", json!({})).unwrap(); + assert_eq!(h.state().session.doc().unwrap().doc.parents.len(), 2); + } + #[test] fn a_page_in_a_spread_offers_delete_page_and_delete_spread() { let mut app = DesignApp::new(designcraft_engine::Session::new(), crate::Services::default()); diff --git a/docs/agents.md b/docs/agents.md index b737ebb8..1930055c 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -74,3 +74,12 @@ designcraft-cli app --method ui.screenshot '{"path": "/tmp/w.png"}' # any contro `ui.menu.invoke {command}` without params acts like choosing the menu item: a command labelled "…" opens its dialog, which `ui.dialog.set` / `ui.dialog.confirm` fill in and close like a user would. + +## Parent pages + +In the Pages panel, right-click a parent row and choose **Delete Parent**. The equivalent +command is `layout.parents.delete {id}`, using the parent spread's numeric ID. It deletes that +parent's shared artwork and clears its assignments from document pages and other parents. +Local override copies remain as ordinary editable objects; document pages and their own content +remain. Undo restores the parent and its relationships in one step. Deleting the last parent +returns the canvas to document pages. Parent navigation is a separate operation.