mirror of
https://github.com/storytold/designcraft.git
synced 2026-11-10 18:40:27 -06:00
Command API: refuse a page index past the end, and say how many pages went (#2460)
* Command API: refuse a page index past the end, and say how many pages went layout.pages.delete answered ok for an index past the last page and deleted nothing for it, so a caller off by one removed a different range and read a success; on a 336-page book that is page 337 instead of 336. Both commands now check every index against the document and refuse it with the sentence the MCP export commands have used since #70, return how many pages they touched so a no-op and a success are told apart, and state in their params that the indices count from 0. * Pages: count the pages a delete will remove, not the indices it was given delete_pages refused [0, 0, 1, 2] on a four-page document as if it would leave none, because the guard measured the length of the list instead of the pages it names. Same family as the index check next to it: the count was of the wrong thing. A test in the engine pins it. * Pages: refuse a malformed page list instead of dropping what it cannot read Review finding: page_indices used filter_map, so a list holding a string, a fraction or nothing at all was silently shortened — the caller was told a page was handled when it never was, which is the same mistake the command now refuses one level down. Every entry must be a whole number, and pages must be a list. Tests cover all three shapes, and the new test functions carry docstrings so the coverage check has nothing to report.
This commit is contained in:
kuseidos
@@ -581,12 +581,14 @@ impl Document {
|
||||
|
||||
/// Delete absolute pages (items whose centre is on a deleted page are deleted too).
|
||||
pub fn delete_pages(&mut self, pages: &[usize]) -> Result<()> {
|
||||
if pages.len() >= self.page_count() {
|
||||
return Err(DocError::Invalid("a document needs at least one page".into()));
|
||||
}
|
||||
let mut doomed: Vec<(usize, usize)> = pages.iter().filter_map(|p| self.page_loc(*p)).collect();
|
||||
doomed.sort();
|
||||
doomed.dedup();
|
||||
// The pages it will remove, not the indices it was given: asking for [0, 0, 1, 2] of four
|
||||
// pages removes three of them and leaves one, which is allowed.
|
||||
if doomed.len() >= self.page_count() {
|
||||
return Err(DocError::Invalid("a document needs at least one page".into()));
|
||||
}
|
||||
let mut remove_items: Vec<ItemId> = Vec::new();
|
||||
for &(si, pi) in &doomed {
|
||||
let sp = &self.spreads[si];
|
||||
|
||||
@@ -5,7 +5,7 @@ use super::parents::{
|
||||
parent_label_taken, parent_name_param, parent_options, parent_page_size, parent_page_target, parent_prefix_param, parent_ref, parent_spread_index,
|
||||
};
|
||||
use super::sections::{ChapterEdit, SectionEdit, chapter_numbering, start_number};
|
||||
use super::{CommandSpec, LAYER_COLORS, Layer, LayerId, Margins, SpreadId, Value, bad, cmd, has_doc, json, ok, str_param};
|
||||
use super::{CommandSpec, LAYER_COLORS, Layer, LayerId, Margins, Result, SpreadId, Value, bad, cmd, has_doc, json, ok, str_param};
|
||||
|
||||
pub fn specs() -> Vec<CommandSpec> {
|
||||
vec![
|
||||
@@ -81,19 +81,25 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
})
|
||||
}
|
||||
),
|
||||
cmd!("layout.pages.delete", "Delete Pages", ["Layout", "Pages"], None, "{pages: [index]}", has_doc, |s, p| {
|
||||
let pages: Vec<usize> = p
|
||||
.get("pages")
|
||||
.and_then(Value::as_array)
|
||||
.map(|a| a.iter().filter_map(Value::as_u64).map(|v| v as usize).collect())
|
||||
.unwrap_or_default();
|
||||
s.edit(|d, sel| {
|
||||
d.delete_pages(&pages)?;
|
||||
sel.items.retain(|i| d.item(*i).is_some());
|
||||
sel.text = sel.text.filter(|t| d.story(t.story).is_some());
|
||||
ok()
|
||||
})
|
||||
}),
|
||||
cmd!(
|
||||
"layout.pages.delete",
|
||||
"Delete Pages",
|
||||
["Layout", "Pages"],
|
||||
None,
|
||||
"{pages: [0-based page indices]} → {deleted: how many pages were removed}",
|
||||
has_doc,
|
||||
|s, p| {
|
||||
const ID: &str = "layout.pages.delete";
|
||||
let pages = page_indices(ID, s.doc()?.doc.page_count(), p)?;
|
||||
let deleted = distinct_pages(&pages);
|
||||
s.edit(|d, sel| {
|
||||
d.delete_pages(&pages)?;
|
||||
sel.items.retain(|i| d.item(*i).is_some());
|
||||
sel.text = sel.text.filter(|t| d.story(t.story).is_some());
|
||||
Ok(json!({"deleted": deleted}))
|
||||
})
|
||||
}
|
||||
),
|
||||
cmd!("layout.pages.move", "Move Pages…", ["Layout", "Pages"], None, "{from, to}", has_doc, |s, p| {
|
||||
let from = p.get("from").and_then(Value::as_u64).ok_or_else(|| bad("layout.pages.move", "missing from"))? as usize;
|
||||
let to = p.get("to").and_then(Value::as_u64).ok_or_else(|| bad("layout.pages.move", "missing to"))? as usize;
|
||||
@@ -165,18 +171,16 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
"Apply Parent to Pages…",
|
||||
["Layout", "Pages"],
|
||||
None,
|
||||
"{pages: [index], parent: id|\"A-Parent\"|\"A\"|null (null or \"[None]\" clears)}",
|
||||
"{pages: [0-based page indices], parent: id|\"A-Parent\"|\"A\"|null (null or \"[None]\" clears)} → {applied: how many pages took the parent}",
|
||||
has_doc,
|
||||
|s, p| {
|
||||
let pages: Vec<usize> = p
|
||||
.get("pages")
|
||||
.and_then(Value::as_array)
|
||||
.map(|a| a.iter().filter_map(Value::as_u64).map(|v| v as usize).collect())
|
||||
.unwrap_or_default();
|
||||
let parent = parent_ref("layout.pages.applyParent", &s.doc()?.doc, p.get("parent"))?.unwrap_or(None);
|
||||
const ID: &str = "layout.pages.applyParent";
|
||||
let pages = page_indices(ID, s.doc()?.doc.page_count(), p)?;
|
||||
let applied = distinct_pages(&pages);
|
||||
let parent = parent_ref(ID, &s.doc()?.doc, p.get("parent"))?.unwrap_or(None);
|
||||
s.edit(|d, _| {
|
||||
d.apply_parent(&pages, parent)?;
|
||||
ok()
|
||||
Ok(json!({"applied": applied}))
|
||||
})
|
||||
}
|
||||
),
|
||||
@@ -828,3 +832,129 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
}),
|
||||
]
|
||||
}
|
||||
|
||||
/// The page indices a `layout.pages.*` command was given, in the order it passed them.
|
||||
///
|
||||
/// Both commands count pages from 0. An index past the last page used to be dropped in
|
||||
/// silence, so a caller that was off by one deleted a different range and still read
|
||||
/// `ok`; the sentence is the one the MCP export commands have used since #70.
|
||||
fn page_indices(id: &str, count: usize, p: &Value) -> Result<Vec<usize>> {
|
||||
// A list that is missing, is not a list, or holds something that is not a whole number is
|
||||
// refused rather than filtered: dropping an entry silently is the same mistake this fixes,
|
||||
// one level up — the caller would be told a page was handled when it never was.
|
||||
let raw = p.get("pages").and_then(Value::as_array).ok_or_else(|| bad(id, "pages takes a list of page indices"))?;
|
||||
let mut out = Vec::with_capacity(raw.len());
|
||||
for v in raw {
|
||||
let n = v.as_u64().ok_or_else(|| bad(id, format!("pages takes whole numbers, and {v} is not one")))?;
|
||||
let i = usize::try_from(n).unwrap_or(usize::MAX);
|
||||
if i >= count {
|
||||
return Err(bad(id, format!("no page {i} (the document has {count} pages, indices are 0-based)")));
|
||||
}
|
||||
out.push(i);
|
||||
}
|
||||
Ok(out)
|
||||
}
|
||||
|
||||
/// How many pages a list of indices names, counting a repeated index once.
|
||||
fn distinct_pages(pages: &[usize]) -> usize {
|
||||
let mut v = pages.to_vec();
|
||||
v.sort_unstable();
|
||||
v.dedup();
|
||||
v.len()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod page_index_tests {
|
||||
use serde_json::json;
|
||||
|
||||
fn four_pages() -> crate::Session {
|
||||
let mut s = crate::Session::new();
|
||||
s.execute("file.new", &json!({"pages": 4})).unwrap();
|
||||
s
|
||||
}
|
||||
|
||||
fn pages_left(s: &crate::Session) -> usize {
|
||||
s.doc().unwrap().doc.page_count()
|
||||
}
|
||||
|
||||
/// An index past the last page is an error that names it, and nothing is removed.
|
||||
#[test]
|
||||
fn a_page_index_past_the_end_is_refused_and_nothing_is_deleted() {
|
||||
let mut s = four_pages();
|
||||
let err = s.execute("layout.pages.delete", &json!({"pages": [4]})).unwrap_err().to_string();
|
||||
assert!(err.contains("no page 4"), "the message names the index: {err}");
|
||||
assert!(err.contains("indices are 0-based"), "and the base: {err}");
|
||||
assert_eq!(pages_left(&s), 4, "nothing went");
|
||||
}
|
||||
|
||||
/// The reply says how many pages went, and a page named twice is one page.
|
||||
#[test]
|
||||
fn deleting_says_how_many_pages_went() {
|
||||
let mut s = four_pages();
|
||||
let r = s.execute("layout.pages.delete", &json!({"pages": [0, 1]})).unwrap();
|
||||
assert_eq!(r["deleted"], 2);
|
||||
assert_eq!(pages_left(&s), 2);
|
||||
// A page named twice is one page, not two.
|
||||
let mut s = four_pages();
|
||||
let r = s.execute("layout.pages.delete", &json!({"pages": [1, 1]})).unwrap();
|
||||
assert_eq!(r["deleted"], 1);
|
||||
assert_eq!(pages_left(&s), 3);
|
||||
}
|
||||
|
||||
/// Three pages named of four leave one, which is allowed even with a repeat.
|
||||
#[test]
|
||||
fn a_page_named_twice_does_not_count_as_two_against_the_last_one_left() {
|
||||
// Four pages, three named (one of them twice): that leaves one and is allowed.
|
||||
let mut s = four_pages();
|
||||
let r = s.execute("layout.pages.delete", &json!({"pages": [0, 0, 1, 2]})).unwrap();
|
||||
assert_eq!(r["deleted"], 3);
|
||||
assert_eq!(pages_left(&s), 1);
|
||||
}
|
||||
|
||||
/// Applying a parent reports how many pages took it, and the pages really do.
|
||||
#[test]
|
||||
fn applying_a_parent_says_how_many_pages_took_it() {
|
||||
let mut s = four_pages();
|
||||
s.execute("layout.parents.new", &json!({"prefix": "B"})).unwrap();
|
||||
let r = s.execute("layout.pages.applyParent", &json!({"pages": [1, 2], "parent": "B"})).unwrap();
|
||||
assert_eq!(r["applied"], 2);
|
||||
let doc = s.doc().unwrap();
|
||||
for page in [1usize, 2] {
|
||||
assert!(doc.doc.page(page).unwrap().parent.is_some(), "page {page} took the parent");
|
||||
}
|
||||
}
|
||||
|
||||
/// Refused before the edit starts, so no page changes parent.
|
||||
#[test]
|
||||
fn a_page_index_past_the_end_is_refused_by_a_parent() {
|
||||
let mut s = four_pages();
|
||||
// Pages are born on the default parent A, so what must not change is which parent they have.
|
||||
let b = s.execute("layout.parents.new", &json!({"prefix": "B"})).unwrap()["id"].as_u64().unwrap();
|
||||
let before: Vec<_> = (0..4).map(|i| s.doc().unwrap().doc.page(i).unwrap().parent.map(|p| p.0)).collect();
|
||||
let err = s.execute("layout.pages.applyParent", &json!({"pages": [4], "parent": "B"})).unwrap_err().to_string();
|
||||
assert!(err.contains("no page 4"), "{err}");
|
||||
let after: Vec<_> = (0..4).map(|i| s.doc().unwrap().doc.page(i).unwrap().parent.map(|p| p.0)).collect();
|
||||
assert_eq!(before, after, "no page changed parent");
|
||||
assert!(!after.contains(&Some(b)), "and none took B");
|
||||
}
|
||||
|
||||
/// A malformed list is refused instead of quietly dropping the entries it cannot read.
|
||||
#[test]
|
||||
fn a_malformed_page_list_is_refused_instead_of_ignored() {
|
||||
let mut s = four_pages();
|
||||
for list in [json!({"pages": "4"}), json!({"pages": [1, "2"]}), json!({})] {
|
||||
let err = s.execute("layout.pages.delete", &list).unwrap_err().to_string();
|
||||
assert!(err.contains("pages takes"), "refused: {list} → {err}");
|
||||
}
|
||||
assert_eq!(pages_left(&s), 4, "and nothing went");
|
||||
}
|
||||
|
||||
/// Both commands' `params` doc states that the indices count from 0.
|
||||
#[test]
|
||||
fn both_commands_state_the_base_in_their_params() {
|
||||
for id in ["layout.pages.delete", "layout.pages.applyParent"] {
|
||||
let spec = crate::cmd::command_specs().iter().find(|s| s.id == id).unwrap();
|
||||
assert!(spec.params.contains("0-based"), "{id} says its base: {}", spec.params);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user