Tables: commands given a table id run with nothing in a table selected (#2607)

* Tables: commands given a table id run with nothing in a table selected

Table commands whose params list `story?` and `table?` (table.insertRow,
table.deleteColumn, table.options, the Convert Rows commands and the
others) were refused with "place the insertion point in a table" when the
call named the table but the selection was not in one. A call that passes
`table` now acts on that table whatever is selected. A table that is not
in the document, or not in the given `story`, is refused, and a `story`
that is not a non-negative integer is an error.

The menus are unchanged: with nothing in a table selected, the table
items stay disabled, and calls that name no table are still refused.

* Tables: a bad story is refused whether or not a table is selected

A call that names a table with a story that is not a non-negative integer (a string, a negative number, a fraction or null) was refused only when nothing in a table was selected. With a table selected, the command ignored the story and acted on the named table. Both paths now read story with one helper and refuse it the same way.

* Tables: an edit to a named table keeps a caret in footnote text

With the caret in footnote text, a table command that names a table
runs, and the edit cleared the caret's footnote address, so the next
typing went into the story. clamp_selection now leaves footnote
addresses alone, as in_cell does.

crates/engine/src/cmd.rs

@@ -331,6 +331,24 @@ pub(crate) fn named_targets(spec: &CommandSpec, p: &Value) -> Option<Vec<ItemId>
     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<u64> {
+    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<Option<u64>> {
+    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<Vec<ItemId>> {
     if let Some(ids) = ids_param(p, "ids") {

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;

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<Tgt> {
     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)) {

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.

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<Value>| 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]

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,