diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 03b6082db..dc1acaf15 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -331,6 +331,24 @@ pub(crate) fn named_targets(spec: &CommandSpec, p: &Value) -> Option id_param(p, "id").map(|i| vec![i]) } +/// The table a call names with `table`, for a command whose params document an optional `table`. +/// `None` when the call names no table. [`table_story`] reads the `story` that goes with it. +pub(crate) fn named_table(spec: &CommandSpec, p: &Value) -> Option { + if !spec.params.split(|c: char| !c.is_ascii_alphanumeric() && c != '?').any(|w| w == "table?") { + return None; + } + p.get("table").and_then(Value::as_u64) +} + +/// The `story` a table command's call names alongside `table`, or `None` when the call has no +/// `story`. A `story` that is present but not a non-negative integer, `null` included, is an error. +pub(crate) fn table_story(p: &Value, cmd: &str) -> Result> { + match p.get("story") { + None => Ok(None), + Some(v) => v.as_u64().map(Some).ok_or_else(|| bad(cmd, "`story` must be a non-negative integer")), + } +} + /// Targets: `ids` / `id` params or the selection. pub(crate) fn targets(s: &Session, p: &Value) -> Result> { if let Some(ids) = ids_param(p, "ids") { diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index d3743df9e..7674d1c70 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -14,7 +14,7 @@ use designcraft_doc::{ }; use serde_json::{Value, json}; -use super::{CommandSpec, bad, cmd, f64_or, has_doc, has_text, ok, str_param}; +use super::{CommandSpec, bad, cmd, f64_or, has_doc, has_text, ok, str_param, table_story}; use crate::{Result, Session}; mod cell; diff --git a/crates/engine/src/cmd/table/target.rs b/crates/engine/src/cmd/table/target.rs index 2e1784560..629493e9b 100644 --- a/crates/engine/src/cmd/table/target.rs +++ b/crates/engine/src/cmd/table/target.rs @@ -1,6 +1,6 @@ use super::{ CellAddr, CellRange, Document, Result, Selection, Session, StoryId, Table, TableSel, TextSel, Value, bad, compose, floor_char_boundary, has_doc, - str_param, + str_param, table_story, }; // ---------- enablement and targeting ---------- @@ -58,8 +58,8 @@ pub(super) fn target(s: &Session, p: &Value, cmd: &str) -> Result { let st = s.doc()?; let doc = &st.doc; let explicit_table = p.get("table").and_then(Value::as_u64); - let explicit_story = p.get("story").and_then(Value::as_u64).map(StoryId); let (story, table, range) = if let Some(tid) = explicit_table { + let explicit_story = table_story(p, cmd)?.map(StoryId); let sid = explicit_story.or_else(|| find_table(doc, tid)).ok_or_else(|| bad(cmd, format!("no table {tid}")))?; (sid, tid, None) } else if let Some(c) = st.selection.cells { @@ -116,8 +116,10 @@ pub(super) fn clamp_selection(d: &Document, sel: &mut Selection) { None => sel.cells = None, } } + // Footnote text is addressed as a cell too, but isn't in a table. if let Some(t) = sel.text && let Some(c) = t.cell + && c.footnote_id().is_none() { let story = d.story(t.story); match story.and_then(|x| x.tables.get(&c.table)) { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index f87371531..3d43a80e0 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -680,9 +680,22 @@ impl Session { Ok(r) } - /// Fails with `Disabled` unless the command is enabled or names objects it can act on. + /// Fails with `Disabled` unless the command is enabled or names objects or a table it can act on. fn check_enabled(&self, spec: &CommandSpec, id: &str, params: &Value) -> Result<()> { if let Err(e) = (spec.enabled)(self) { + // A table command that takes `story` and `table` acts on the table the call names, so + // the selection doesn't matter: with nothing selected, the caret outside a table, or + // selected rows the command can't act on, `{"table": 3}` still runs. + if let Some(table) = cmd::named_table(spec, params) + && cmd::has_doc(self).is_ok() + { + let story = cmd::table_story(params, id)?; + let st = self.doc()?; + if !st.doc.stories.iter().any(|(sid, x)| story.is_none_or(|n| sid.0 == n) && x.tables.contains_key(&table)) { + return Err(EngineError::Disabled(id.into(), format!("no table {table}"))); + } + return Ok(()); + } // A command that takes `ids` acts on the objects the call names, so the selection // doesn't matter: with a text caret, nothing selected, or a selection the command // can't act on, `{"ids": [5]}` still runs. diff --git a/crates/engine/src/tests/table.rs b/crates/engine/src/tests/table.rs index 77bcb9bfd..3941778f2 100644 --- a/crates/engine/src/tests/table.rs +++ b/crates/engine/src/tests/table.rs @@ -493,6 +493,107 @@ fn rows_columns_merge_and_options() { assert_eq!(st.doc.story(sid).unwrap().text, "Intro"); } +/// A table command given `story` and `table` acts on that table with nothing selected, the caret +/// outside the table, or a selection it can't act on, and a table that isn't there is refused. +#[test] +fn table_commands_run_on_a_named_table_without_a_table_selection() { + let (mut s, sid, _) = session_with_frame(); + let tid = s.execute("table.insert", &json!({"rows": 2, "cols": 2})).unwrap()["table"].as_u64().unwrap(); + let size = |s: &mut Session| { + let t = s.execute("table.get", &json!({"story": sid, "table": tid})).unwrap(); + (t["rows"].as_array().unwrap().len(), t["columns"].as_array().unwrap().len()) + }; + assert_eq!(size(&mut s), (2, 2)); + s.execute("edit.deselectAll", &json!({})).unwrap(); + s.execute("table.insertRow", &json!({"story": sid, "table": tid, "where": "below"})).unwrap(); + assert_eq!(size(&mut s), (3, 2)); + s.execute("table.insertColumnRight", &json!({"table": tid})).unwrap(); + assert_eq!(size(&mut s), (3, 3)); + s.execute("table.convertRowsToHeader", &json!({"story": sid, "table": tid, "rows": [0, 0]})).unwrap(); + assert_eq!(s.execute("table.get", &json!({"table": tid})).unwrap()["headerRows"], 1); + // A selected header row can't become a header, but the named rows can. + s.execute("table.select", &json!({"table": tid, "rows": [0, 0], "what": "row"})).unwrap(); + assert!(!crate::cmd::find_command("table.convertRowsToHeader").unwrap().info(&s).enabled); + s.execute("table.convertRowsToHeader", &json!({"table": tid, "rows": [1, 1]})).unwrap(); + assert_eq!(s.execute("table.get", &json!({"table": tid})).unwrap()["headerRows"], 2); + // The caret in the story's text, outside the table. + s.execute("text.select", &json!({"story": sid, "anchor": 0, "focus": 0})).unwrap(); + s.execute("table.deleteRow", &json!({"story": sid, "table": tid, "rows": [2, 2]})).unwrap(); + assert_eq!(size(&mut s), (2, 3)); + check(&s); + s.execute("edit.deselectAll", &json!({})).unwrap(); + let disabled = |r: Result| match r { + Err(crate::EngineError::Disabled(_, why)) => why, + other => panic!("expected Disabled, got {other:?}"), + }; + assert_eq!(disabled(s.execute("table.insertRow", &json!({"table": tid + 100}))), format!("no table {}", tid + 100)); + assert_eq!(disabled(s.execute("table.insertRow", &json!({"story": sid + 100, "table": tid}))), format!("no table {tid}")); + for story in [json!("x"), json!(-1), json!(null)] { + let r = s.execute("table.insertRow", &json!({"story": story, "table": tid})); + assert!(matches!(r, Err(crate::EngineError::BadParams { .. })), "{r:?}"); + } + assert_eq!(size(&mut s), (2, 3)); +} + +/// A `story` that is not a non-negative integer is refused the same way whether or not a table is +/// selected, and the table is left unchanged. +#[test] +fn a_bad_story_is_refused_with_or_without_a_table_selection() { + let (mut s, _, _) = session_with_frame(); + let tid = s.execute("table.insert", &json!({"rows": 2, "cols": 2})).unwrap()["table"].as_u64().unwrap(); + let rows = |s: &mut Session| s.execute("table.get", &json!({"table": tid})).unwrap()["rows"].as_array().unwrap().len(); + for select in [json!({"table": tid, "rows": [0, 0], "what": "row"}), json!(null)] { + if select.is_null() { + s.execute("edit.deselectAll", &json!({})).unwrap(); + } else { + s.execute("table.select", &select).unwrap(); + } + for story in [json!("x"), json!(-1), json!(1.5), json!(null)] { + let r = s.execute("table.insertRow", &json!({"story": story, "table": tid})); + assert!(matches!(r, Err(crate::EngineError::BadParams { .. })), "{select} {story}: {r:?}"); + } + assert_eq!(rows(&mut s), 2, "{select}"); + } +} + +/// An edit to a named table leaves a caret in footnote text where it was. +#[test] +fn a_named_table_edit_keeps_a_footnote_caret() { + let (mut s, sid, _) = session_with_frame(); + let tid = s.execute("table.insert", &json!({"rows": 2, "cols": 2})).unwrap()["table"].as_u64().unwrap(); + s.execute("text.select", &json!({"story": sid, "anchor": 0, "focus": 0})).unwrap(); + s.execute("footnote.insert", &json!({"text": "Note"})).unwrap(); + let before = s.doc().unwrap().selection.clone(); + assert!(before.text.and_then(|t| t.cell).and_then(|c| c.footnote_id()).is_some(), "the caret is in the footnote"); + s.execute("table.insertRow", &json!({"table": tid})).unwrap(); + assert_eq!(s.execute("table.get", &json!({"table": tid})).unwrap()["rows"].as_array().unwrap().len(), 3); + assert_eq!(s.doc().unwrap().selection, before); + check(&s); +} + +/// With nothing selected, the table commands stay disabled for menus and for calls that name no +/// table. +#[test] +fn table_commands_stay_disabled_with_nothing_selected() { + let (mut s, _, _) = session_with_frame(); + s.execute("table.insert", &json!({"rows": 2, "cols": 2})).unwrap(); + let in_table = ["table.insertRow", "table.deleteColumn", "table.merge", "table.options", "table.selectTable", "style.table.apply"]; + let in_cell = ["table.convertRowsToHeader", "table.nextCell"]; + let info = |s: &Session, id: &str| crate::cmd::find_command(id).unwrap().info(s); + for id in in_table.iter().chain(&in_cell) { + assert!(info(&s, id).enabled, "{id} is enabled with the caret in a cell"); + } + s.execute("edit.deselectAll", &json!({})).unwrap(); + for (ids, reason) in [(&in_table[..], "place the insertion point in a table"), (&in_cell[..], "not in a table cell")] { + for id in ids { + let i = info(&s, id); + assert!(!i.enabled, "{id}"); + assert_eq!(i.disabled_reason.as_deref(), Some(reason), "{id}"); + assert!(matches!(s.execute(id, &json!({})), Err(crate::EngineError::Disabled(..))), "{id}"); + } + } +} + /// `table.insert` styles the table in the same step; a style that isn't there, or isn't a name, /// leaves the story alone. Its dimensions stop at the documented limits. #[test] diff --git a/docs/mcp.md b/docs/mcp.md index 61e59df4d..f7e0b8187 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -206,6 +206,14 @@ is an error that gives the story length in bytes, and the story is left unchange that has a `cell` (`{table, row, col}`) is in that table cell's text, and its `start` / `end` count from the start of the cell. +## Tables without a selection + +A table command acts on the selected cells or on the table that holds the insertion point. A command whose params list +`story?` and `table?` also runs when nothing in a table is selected, or the selection is one it can't act on, if the +call passes `table` (the id that `table.insert` and `table.get` return) and, optionally, `story`. A `table` that is not +in the document, or not in the given `story`, is an error, and so is a `story` that is not a non-negative integer. The +Table menu items stay disabled while nothing in a table is selected. + ## Text and image fitting Hyphenation is on by default for new text (`hyphenate: true`). To turn it off, select the text or a text frame,