diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index fc98b4a1..08cb3f3b 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -13,22 +13,40 @@ use crate::{DocState, Result, Session}; pub fn specs() -> Vec { vec![ cmd!("table.insertRowAbove", "Insert Above", ["Table Layout", "Rows & Columns"], None, "{row?, id?}", has_selection, |s, p| rows_cols( - s, p, "rowAbove" + s, + p, + "rowAbove", + "table.insertRowAbove" )), cmd!("table.insertRowBelow", "Insert Below", ["Table Layout", "Rows & Columns"], None, "{row?, id?}", has_selection, |s, p| rows_cols( - s, p, "rowBelow" + s, + p, + "rowBelow", + "table.insertRowBelow" )), cmd!("table.insertColumnLeft", "Insert Left", ["Table Layout", "Rows & Columns"], None, "{col?, id?}", has_selection, |s, p| rows_cols( - s, p, "colLeft" + s, + p, + "colLeft", + "table.insertColumnLeft" )), cmd!("table.insertColumnRight", "Insert Right", ["Table Layout", "Rows & Columns"], None, "{col?, id?}", has_selection, |s, p| rows_cols( - s, p, "colRight" + s, + p, + "colRight", + "table.insertColumnRight" )), cmd!("table.deleteRow", "Delete Rows", ["Table Layout", "Rows & Columns"], None, "{row?, id?}", has_selection, |s, p| rows_cols( - s, p, "delRow" + s, + p, + "delRow", + "table.deleteRow" )), cmd!("table.deleteColumn", "Delete Columns", ["Table Layout", "Rows & Columns"], None, "{col?, id?}", has_selection, |s, p| rows_cols( - s, p, "delCol" + s, + p, + "delCol", + "table.deleteColumn" )), cmd!("table.merge", "Merge Cells", ["Table Layout", "Merge"], None, "{from: [r, c], to: [r, c], id?}", has_selection, merge), cmd!( @@ -228,16 +246,26 @@ impl TableFrame { } } -fn cell_param(p: &Value, key: &str) -> Option<(usize, usize)> { - let a = p.get(key)?.as_array()?; - Some((a.first()?.as_u64()? as usize, a.get(1)?.as_u64()? as usize)) +/// A `[r, c]` cell param: `Ok(None)` when absent (callers may fall back to the selection), `Err` when +/// present but not a valid cell, so a bad cell never acts on the selection. +fn cell_param(p: &Value, key: &str, cmd: &str) -> Result> { + let v = match p.get(key) { + None | Some(Value::Null) => return Ok(None), + Some(v) => v, + }; + let idx = |i: usize| v.as_array().and_then(|a| a.get(i)).and_then(Value::as_u64).and_then(|x| usize::try_from(x).ok()); + match (idx(0), idx(1)) { + (Some(r), Some(c)) => Ok(Some((r, c))), + _ => Err(bad(cmd, format!("`{key}` is not a valid cell: {v}"))), + } } /// The table's row/col to act on: params, the edited cell, or the selected cells. -fn at(s: &Session, p: &Value) -> (usize, usize) { +/// A present but invalid `row`/`col` is an error; only an absent one falls back. +fn at(s: &Session, p: &Value, cmd: &str) -> Result<(usize, usize)> { let st = s.active(); let cell = st.and_then(|d| d.selection.text.as_ref().and_then(|t| t.cell)).or_else(|| st.and_then(|d| d.selection.cells.map(|c| c.1))); - (usize_param(p, "row").or(cell.map(|c| c.0)).unwrap_or(0), usize_param(p, "col").or(cell.map(|c| c.1)).unwrap_or(0)) + Ok((opt_usize_param(p, "row", cmd)?.or(cell.map(|c| c.0)).unwrap_or(0), opt_usize_param(p, "col", cmd)?.or(cell.map(|c| c.1)).unwrap_or(0))) } fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> Result<()>) -> Result { @@ -285,10 +313,10 @@ fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> R Ok(r) } -fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { - let (r, c) = at(s, p); +fn rows_cols(s: &mut Session, p: &Value, op: &str, name: &'static str) -> Result { + let (r, c) = at(s, p, name)?; let op = op.to_string(); - with_table(s, p, "table", move |t| { + with_table(s, p, name, move |t| { match op.as_str() { "rowAbove" => t.insert_row(r), "rowBelow" => t.insert_row(r.saturating_add(1)), @@ -296,12 +324,12 @@ fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { "colRight" => t.insert_col(c.saturating_add(1)), "delRow" => { if !t.delete_row(r) { - return Err(bad("table.deleteRow", "can't delete the last row")); + return Err(bad(name, "can't delete the last row")); } } _ => { if !t.delete_col(c) { - return Err(bad("table.deleteColumn", "can't delete the last column")); + return Err(bad(name, "can't delete the last column")); } } } @@ -311,8 +339,8 @@ fn rows_cols(s: &mut Session, p: &Value, op: &str) -> Result { fn merge(s: &mut Session, p: &Value) -> Result { let sel = s.doc()?.selection.cells; - let from = cell_param(p, "from").or(sel.map(|c| c.1)).ok_or_else(|| bad("table.merge", "missing `from`"))?; - let to = cell_param(p, "to").or(sel.map(|c| c.2)).ok_or_else(|| bad("table.merge", "missing `to`"))?; + let from = cell_param(p, "from", "table.merge")?.or(sel.map(|c| c.1)).ok_or_else(|| bad("table.merge", "missing `from`"))?; + let to = cell_param(p, "to", "table.merge")?.or(sel.map(|c| c.2)).ok_or_else(|| bad("table.merge", "missing `to`"))?; with_table( s, p, @@ -322,7 +350,10 @@ fn merge(s: &mut Session, p: &Value) -> Result { } fn split(s: &mut Session, p: &Value) -> Result { - let cell = cell_param(p, "cell").unwrap_or_else(|| at(s, p)); + let cell = match cell_param(p, "cell", "table.split")? { + Some(c) => c, + None => at(s, p, "table.split")?, + }; with_table(s, p, "table.split", move |t| if t.split(cell.0, cell.1) { Ok(()) } else { Err(bad("table.split", "that cell isn't merged")) }) } @@ -536,7 +567,7 @@ fn distribute(s: &mut Session, p: &Value, rows: bool) -> Result { } fn column_width(s: &mut Session, p: &Value) -> Result { - let (_, c) = at(s, p); + let (_, c) = at(s, p, "table.columnWidth")?; let w = f64_param(p, "width").ok_or_else(|| bad("table.columnWidth", "missing `width`"))?.clamp(4.0, 4000.0); let keep_total = bool_or(p, "keepTotal", false); with_table(s, p, "table.columnWidth", move |t| { @@ -565,7 +596,7 @@ fn column_width(s: &mut Session, p: &Value) -> Result { } fn row_height(s: &mut Session, p: &Value) -> Result { - let (r, _) = at(s, p); + let (r, _) = at(s, p, "table.rowHeight")?; let h = f64_param(p, "height").ok_or_else(|| bad("table.rowHeight", "missing `height`"))?.clamp(4.0, 4000.0); with_table(s, p, "table.rowHeight", move |t| { if let Some(x) = t.rows.get_mut(r) { @@ -580,8 +611,8 @@ fn select_cells(s: &mut Session, p: &Value) -> Result { Some(i) => i, None => s.doc()?.selection.shapes.first().copied().ok_or_else(|| bad("table.selectCells", "select a table"))?, }; - let from = cell_param(p, "from").unwrap_or((0, 0)); - let to = cell_param(p, "to").unwrap_or(from); + let from = cell_param(p, "from", "table.selectCells")?.unwrap_or((0, 0)); + let to = cell_param(p, "to", "table.selectCells")?.unwrap_or(from); s.select(|_, sel| { sel.cells = Some((id, from, to)); sel.shapes = vec![id]; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5105fcf3..a0f200ca 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4941,6 +4941,55 @@ fn paragraph_commands_clamp_a_direct_hostile_text_selection() { assert_eq!(body.paragraphs[0].props.align, Some(deckcraft_model::text::Align::Center)); } +#[test] +fn table_row_col_commands_reject_an_invalid_index() { + let mut s = session(); + let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"], ["e", "f"]])); + for (cmd, key) in [ + ("table.deleteRow", "row"), + ("table.deleteColumn", "col"), + ("table.insertRowAbove", "row"), + ("table.insertColumnLeft", "col"), + ("table.split", "row"), + ] { + for bad in [json!(-1), json!(1.5), json!("1")] { + let r = s.execute(cmd, &json!({"id": id.0, key: bad})); + assert!(r.is_err(), "{cmd} {key}={bad} should be rejected, got {r:?}"); + let t = table_of(&s, id); + assert_eq!((t.n_rows(), t.n_cols()), (3, 2), "{cmd} {key}={bad} must leave the table unchanged"); + } + } +} + +#[test] +fn table_cell_commands_reject_an_invalid_cell_and_name_the_registered_command() { + let mut s = session(); + let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"], ["e", "f"]])); + s.execute("table.selectCells", &json!({"id": id.0, "from": [0, 0], "to": [1, 1]})).unwrap(); + for (cmd, params) in [ + ("table.split", json!({"id": id.0, "cell": [-1, 0]})), + ("table.merge", json!({"id": id.0, "from": [-1, 0], "to": [1, 1]})), + ("table.merge", json!({"id": id.0, "from": [0, 0], "to": "x"})), + ("table.rowHeight", json!({"id": id.0, "row": -1, "height": 50.0})), + ("table.rowHeight", json!({"id": id.0, "row": 1.5, "height": 50.0})), + ("table.columnWidth", json!({"id": id.0, "col": -1, "width": 50.0})), + ("table.columnWidth", json!({"id": id.0, "col": "1", "width": 50.0, "keepTotal": true})), + ] { + let before = table_of(&s, id); + let (rows, cols) = (before.rows.iter().map(|r| r.height).collect::>(), before.cols.clone()); + let r = s.execute(cmd, ¶ms); + assert!(r.is_err(), "{cmd} {params} should be rejected, got {r:?}"); + let t = table_of(&s, id); + assert_eq!((t.n_rows(), t.n_cols()), (3, 2), "{cmd} {params} must leave the table unchanged"); + assert_eq!(t.rows.iter().map(|r| r.height).collect::>(), rows, "{cmd} {params} must not change row heights"); + assert_eq!(t.cols, cols, "{cmd} {params} must not change column widths"); + } + let e = s.execute("table.deleteRow", &json!({"id": id.0, "row": -1})).unwrap_err().to_string(); + assert!(e.contains("table.deleteRow"), "error should name the registered command: {e}"); + let e = s.execute("table.insertColumnLeft", &json!({"id": id.0, "col": -1})).unwrap_err().to_string(); + assert!(e.contains("table.insertColumnLeft"), "error should name the registered command: {e}"); +} + #[test] fn table_column_width_of_a_huge_column_is_ignored() { let mut s = session();