diff --git a/crates/engine/src/cmd/extra/scripts.rs b/crates/engine/src/cmd/extra/scripts.rs index b14fbdb23..e9b877672 100644 --- a/crates/engine/src/cmd/extra/scripts.rs +++ b/crates/engine/src/cmd/extra/scripts.rs @@ -138,7 +138,7 @@ pub(super) fn run_script(s: &mut Session, p: &Json) -> Result { let opened_dialog = s.ui_requests.get(before..).is_some_and(|new| new.iter().any(|r| matches!(r, crate::UiRequest::Dialog(..)))); let changed = wb_before.as_ref().is_some_and(|(uid, wb)| s.docs.iter().any(|d| d.uid == *uid && !std::sync::Arc::ptr_eq(&d.wb, wb))); if !opened_dialog || changed { - ran_steps.push((id.to_string(), crate::record::with_toggle_state(st["params"].clone(), &result))); + ran_steps.push((id.to_string(), crate::record::with_toggle_state(id, st["params"].clone(), &result))); } } } diff --git a/crates/engine/src/cmd/python.rs b/crates/engine/src/cmd/python.rs index 8e7007ea1..02d0ccb4e 100644 --- a/crates/engine/src/cmd/python.rs +++ b/crates/engine/src/cmd/python.rs @@ -1,6 +1,6 @@ -//! Python in cells: making and editing Python cells. This build stores the code and the last -//! result a file cached; it doesn't run Python, so a cell with new or changed code shows -//! `#BLOCKED!`. +//! Python in cells: making and editing Python cells, and their return type. This build stores +//! the code and the last result a file cached; it doesn't run Python, so a cell with new or +//! changed code shows `#BLOCKED!`. use std::sync::Arc; @@ -12,15 +12,26 @@ use super::{CommandSpec, bad, cell_param_on, cmd, edit, has_doc, str_param, targ use crate::{EngineError, Result, Session}; pub fn specs() -> Vec { - vec![cmd!( - "python.set", - "Set Python Code", - [], - None, - "{cell?, code, output?: object|value} makes the cell (default: the active cell) a Python cell with this code; the code must fit in Excel's 8,192-character formula once written as PY(…). `output` is the return type: `value` (an Excel value, the default) or `object` (a Python object)", - has_doc, - set - )] + vec![ + cmd!( + "python.set", + "Set Python Code", + [], + None, + "{cell?, code, output?: object|value} makes the cell (default: the active cell) a Python cell with this code; the code must fit in Excel's 8,192-character formula once written as PY(…). `output` is the return type: `value` (an Excel value, the default) or `object` (a Python object)", + has_doc, + set + ), + cmd!( + "python.output", + "Python Output", + [], + Some("Ctrl+Alt+Shift+M"), + "{cell?, mode?: object|value} sets the return type of a Python cell (default: the active cell): `object` (a Python object) or `value` (an Excel value). Without `mode` it switches to the other one. The cell keeps its code; a cached result of the old return type goes, as with `python.set`, so the cell shows #BLOCKED! until it runs", + has_doc, + output + ), + ] } fn target_cell(s: &Session, p: &Json) -> Result<(usize, CellRef)> { @@ -36,12 +47,17 @@ fn existing(s: &Session, sheet: usize, at: CellRef) -> Result> { Ok(s.doc()?.wb.sheet(sheet).and_then(|sh| sh.cell(at)).and_then(|c| c.py.as_deref()).cloned()) } -fn output_mode(p: &Json, key: &str) -> Result> { +fn output_mode(cmd: &str, p: &Json, key: &str) -> Result> { + match p.get(key) { + None | Some(Json::Null) => return Ok(None), + Some(Json::String(_)) => {} + Some(_) => return Err(bad(cmd, format!("`{key}` is `object` or `value`"))), + } match str_param(p, key) { None => Ok(None), Some(m) if m.eq_ignore_ascii_case("object") => Ok(Some(true)), Some(m) if m.eq_ignore_ascii_case("value") => Ok(Some(false)), - Some(m) => Err(bad("python.set", format!("`{key}` is `object` or `value`, not `{m}`"))), + Some(m) => Err(bad(cmd, format!("`{key}` is `object` or `value`, not `{m}`"))), } } @@ -52,13 +68,22 @@ fn set(s: &mut Session, p: &Json) -> Result { Some(_) => return Err(bad("python.set", "`code` must be text")), None => return Err(bad("python.set", "missing `code`")), }; - let mode = output_mode(p, "output")?; + let mode = output_mode("python.set", p, "output")?; let old = existing(s, sheet, at)?; let object = mode.or(old.as_ref().map(|o| o.object)).unwrap_or(false); - let mut py = PyCode { object, ..PyCode::new(code) }; + store(s, "python.set", sheet, at, PyCode { object, ..PyCode::new(code) }, old)?; + let d = s.doc()?; + let shown = d.wb.sheet(sheet).map(|sh| sh.value(at)).unwrap_or_default(); + Ok(json!({"cell": at.a1(), "value": crate::cmd::inspect::value_json(&shown)})) +} + +/// Writes `py` into the cell, which held `old`: what `python.set` and `python.output` share. The +/// code must fit in one cell and a table header takes none; a locked cell on a protected sheet is +/// refused by the edit itself. +fn store(s: &mut Session, cmd: &str, sheet: usize, at: CellRef, mut py: PyCode, old: Option) -> Result<()> { if !py.fits() { return Err(bad( - "python.set", + cmd, format!("the code doesn't fit in one cell: written as PY(…) it would be longer than Excel's {MAX_FORMULA_CHARS}-character formula limit"), )); } @@ -89,8 +114,24 @@ fn set(s: &mut Session, p: &Json) -> Result { sh.set_cell(at, Cell { value, formula: None, style, py: Some(Arc::new(py)) }); cx.touch(sheet, at); Ok(()) - })?; + }) +} + +/// `python.output`: the cell's return type. The code stays; a cached result belongs to the old +/// return type, so it goes, as with `python.set` and a new return type: the cell shows +/// `#BLOCKED!` until it runs, and its old spill goes with its readers recalculated. +fn output(s: &mut Session, p: &Json) -> Result { + let (sheet, at) = target_cell(s, p)?; + let Some(old) = existing(s, sheet, at)? else { + return Err(EngineError::Other(format!("{} isn't a Python cell, so it has no Python output to change", at.a1()))); + }; + let object = output_mode("python.output", p, "mode")?.unwrap_or(!old.object); + let mode = |object: bool| if object { "object" } else { "value" }; + if object != old.object { + let py = PyCode { code: old.code.clone(), object, output: None, stale: false }; + store(s, "python.output", sheet, at, py, Some(old))?; + } let d = s.doc()?; let shown = d.wb.sheet(sheet).map(|sh| sh.value(at)).unwrap_or_default(); - Ok(json!({"cell": at.a1(), "value": crate::cmd::inspect::value_json(&shown)})) + Ok(json!({"cell": at.a1(), "output": mode(object), "value": crate::cmd::inspect::value_json(&shown)})) } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 4ea043a68..d8b7c4408 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -792,7 +792,7 @@ impl Session { // A toggle (Bold) is recorded with the state it set, as Excel records // `Bold = True`: playing it back sets that state rather than flipping it again. let params = match &result { - Ok(r) if params.is_object() => record::with_toggle_state(cmd::journal_params(¶ms), r), + Ok(r) if params.is_object() => record::with_toggle_state(id, cmd::journal_params(¶ms), r), _ => cmd::journal_params(¶ms), }; let kept = rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params }); diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs index 269d11aed..a97082330 100644 --- a/crates/engine/src/record.rs +++ b/crates/engine/src/record.rs @@ -74,15 +74,21 @@ impl Default for Recorder { /// `params` with the state a toggle set (Bold's `on`, from the command's result), as Excel /// records `Bold = True`: playing the step back sets that state rather than flipping it again. -pub(crate) fn with_toggle_state(mut params: Json, result: &Json) -> Json { - if let Some(on @ Json::Bool(_)) = result.get("on") { +/// `python.output` without `mode` is a toggle too: it keeps the return type it set (the result's +/// `output`) as its `mode`. +pub(crate) fn with_toggle_state(id: &str, mut params: Json, result: &Json) -> Json { + let state = match id { + "python.output" => result.get("output").filter(|o| o.is_string()).map(|o| ("mode", o)), + _ => result.get("on").filter(|o| o.is_boolean()).map(|o| ("on", o)), + }; + if let Some((key, state)) = state { if params.is_null() { params = Json::Object(serde_json::Map::new()); } if let Json::Object(p) = &mut params - && !p.contains_key("on") + && p.get(key).is_none_or(Json::is_null) { - p.insert("on".into(), on.clone()); + p.insert(key.into(), state.clone()); } } params diff --git a/crates/engine/src/tests/python_commands.rs b/crates/engine/src/tests/python_commands.rs index 6d0bfac42..eb794c609 100644 --- a/crates/engine/src/tests/python_commands.rs +++ b/crates/engine/src/tests/python_commands.rs @@ -89,3 +89,157 @@ fn set_over_a_spilling_formula_takes_the_spill_away() { assert_eq!(v(&s, "A3"), Value::Empty); assert!(s.doc().unwrap().wb.sheet(0).unwrap().spill_ranges.is_empty()); } + +/// A cached result as a file would have kept it on the cell: `[[1], [2], [3]]`, a list. +fn cache_list(s: &mut crate::Session, cell: &str) { + let at = gridcraft_core::CellRef::parse(cell).unwrap(); + let d = s.doc_mut().unwrap(); + let wb = std::sync::Arc::make_mut(&mut d.wb); + let sh = wb.sheet_mut(0).unwrap(); + let py = std::sync::Arc::make_mut(sh.cells.get_mut(at).unwrap().py.as_mut().unwrap()); + let arr = gridcraft_core::Array::new(3, 1, vec![1.0.into(), 2.0.into(), 3.0.into()]).map(Value::from).unwrap(); + py.output = Some(gridcraft_model::PyOutput { value: arr, type_name: "list".into(), preview: "[1, 2, 3]".into(), convertible: true }); +} + +/// A1 holds `[1, 2, 3]` with its cached list spilled over A1:A3 as Excel values; B1 reads A3. +fn cached_spill_with_a_reader() -> crate::Session { + let mut s = s(); + set(&mut s, "A1", "[1, 2, 3]"); + cache_list(&mut s, "A1"); + s.execute("formulas.calculateNow", json!({})).unwrap(); + s.execute("cell.set", json!({"cell": "B1", "input": "=SUM(A3)"})).unwrap(); + assert_eq!(v(&s, "A1"), Value::Number(1.0)); + assert_eq!(v(&s, "B1"), Value::Number(3.0)); + s +} + +fn output_of(s: &mut crate::Session, cell: &str) -> serde_json::Value { + s.execute("cell.get", json!({"cell": cell})).unwrap()["python"]["output"].clone() +} + +fn undo_depth(s: &mut crate::Session) -> Option { + s.execute("history.list", json!({})).unwrap()["undo"].as_array().map(Vec::len) +} + +#[test] +fn output_switches_the_return_type_and_drops_a_result_of_the_other_type() { + let mut s = cached_spill_with_a_reader(); + // The mode it already has changes nothing and adds no undo step. + let undo = undo_depth(&mut s); + let r = s.execute("python.output", json!({"cell": "A1", "mode": "value"})).unwrap(); + assert_eq!(r["output"], "value"); + assert_eq!(undo_depth(&mut s), undo); + assert_eq!(v(&s, "A3"), Value::Number(3.0), "the kept result still spills"); + // Without `mode` it switches. The cached list is an Excel value's result: as with python.set + // and a new return type, the cell has no result until it runs, and its old spill goes with + // its readers recalculated. + s.execute("selection.set", json!({"cell": "A1"})).unwrap(); + let r = s.execute("python.output", json!({})).unwrap(); + assert_eq!(r["output"], "object"); + assert_eq!(v(&s, "A1"), Value::Error(CellError::Blocked)); + assert_eq!(v(&s, "A3"), Value::Empty, "the old spill goes"); + assert_eq!(v(&s, "B1"), Value::Number(0.0), "and what read it is recalculated"); + assert_eq!(s.execute("cell.get", json!({"cell": "A1"})).unwrap()["python"]["code"], "[1, 2, 3]", "the code stays"); + // The same as python.set with the same code and the other return type. + let mut by_set = cached_spill_with_a_reader(); + by_set.execute("python.set", json!({"cell": "A1", "code": "[1, 2, 3]", "output": "object"})).unwrap(); + for c in ["A1", "A3", "B1"] { + assert_eq!(v(&s, c), v(&by_set, c), "{c}"); + } + assert_eq!(output_of(&mut s, "A1"), output_of(&mut by_set, "A1")); + // Switching back brings no result back: the dropped one belonged to the old type. + s.execute("python.output", json!({"cell": "A1", "mode": "value"})).unwrap(); + assert_eq!(v(&s, "A1"), Value::Error(CellError::Blocked)); + assert_eq!(v(&s, "A3"), Value::Empty); + // Each switch is one undo step, and undoing the first brings the result and its spill back. + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(output_of(&mut s, "A1"), "object"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(output_of(&mut s, "A1"), "value"); + assert_eq!(v(&s, "A1"), Value::Number(1.0)); + assert_eq!(v(&s, "A3"), Value::Number(3.0)); + assert_eq!(v(&s, "B1"), Value::Number(3.0)); + // A cell with no result keeps showing #BLOCKED!. + set(&mut s, "C1", "x = 1"); + s.execute("python.output", json!({"cell": "C1"})).unwrap(); + assert_eq!(v(&s, "C1"), Value::Error(CellError::Blocked)); + assert_eq!(output_of(&mut s, "C1"), "object"); +} + +#[test] +fn output_refuses_table_headers_and_locked_cells_as_set_does() { + let mut s = s(); + // A Python cell in a table's header row (as a file could hold one): python.set refuses to + // put code there, and python.output refuses to change it. + s.execute("range.setValues", json!({"range": "C1", "values": [["Name", "Score"], ["a", 1]]})).unwrap(); + s.execute("insert.table", json!({"range": "C1:D2", "headers": true})).unwrap(); + { + let d = s.doc_mut().unwrap(); + let wb = std::sync::Arc::make_mut(&mut d.wb); + let cell = wb.sheet_mut(0).unwrap().cells.get_mut(gridcraft_core::CellRef::parse("D1").unwrap()).unwrap(); + cell.py = Some(std::sync::Arc::new(gridcraft_model::PyCode::new("'Score'"))); + } + let e = s.execute("python.output", json!({"cell": "D1"})).unwrap_err(); + assert!(e.to_string().contains("table header"), "{e}"); + assert_eq!(output_of(&mut s, "D1"), "value"); + // A locked cell on a protected sheet refuses it; an unlocked one formatted Hidden takes it, + // as it takes python.set. + set(&mut s, "F1", "1"); + set(&mut s, "G1", "2"); + s.execute("home.formatCells", json!({"range": "G1", "style": {"protection": {"locked": false, "hidden": true}}})).unwrap(); + s.execute("review.protectSheet", json!({})).unwrap(); + let e = s.execute("python.output", json!({"cell": "F1"})).unwrap_err(); + assert!(e.to_string().contains("protected"), "{e}"); + assert_eq!(output_of(&mut s, "F1"), "value"); + s.execute("python.set", json!({"cell": "G1", "code": "2"})).unwrap(); + s.execute("python.output", json!({"cell": "G1"})).unwrap(); + assert_eq!(output_of(&mut s, "G1"), "object"); +} + +#[test] +fn output_refuses_cells_without_python_and_bad_modes() { + let mut s = s(); + // Empty params: the active cell, which holds no Python code. + let e = s.execute("python.output", json!({})).unwrap_err(); + assert!(e.to_string().contains("isn't a Python cell"), "{e}"); + s.execute("cell.set", json!({"cell": "B1", "input": "=1+1"})).unwrap(); + assert!(s.execute("python.output", json!({"cell": "B1"})).is_err()); + set(&mut s, "A1", "1"); + for p in [json!({"cell": "A1", "mode": "sideways"}), json!({"cell": "A1", "mode": 1}), json!({"cell": "nowhere"})] { + assert!(s.execute("python.output", p.clone()).is_err(), "{p}"); + } + assert_eq!(s.execute("cell.get", json!({"cell": "A1"})).unwrap()["python"]["output"], "value"); + // python.set refuses a return type that isn't text too, rather than ignoring it. + assert!(s.execute("python.set", json!({"cell": "A1", "code": "1", "output": true})).is_err()); +} + +#[test] +fn a_recorded_switch_keeps_the_return_type_it_set() { + // python.output without `mode` switches, so a recording keeps the return type the switch set + // (as Bold records `on`): played back on a cell that already has it, nothing switches back. + let mut s = s(); + set(&mut s, "A1", "1"); + s.execute("macro.record", json!({"relative": false})).unwrap(); + s.execute("python.output", json!({"cell": "A1"})).unwrap(); + s.execute("macro.record", json!({"stop": true, "name": "Rec"})).unwrap(); + assert_eq!(output_of(&mut s, "A1"), "object"); + let m = s.execute("macro.get", json!({"name": "Rec"})).unwrap(); + let m = crate::macros::parse_macro(&m).unwrap(); + let steps = crate::macros::command_steps(&m.steps); + let step = steps.iter().find(|(_, id, _)| *id == "python.output").map(|(_, _, p)| (*p).clone()); + assert_eq!(step.and_then(|p| p.get("mode").cloned()), Some(json!("object"))); + for _ in 0..2 { + s.execute("macro.run", json!({"name": "Rec", "onConflict": "overwrite"})).unwrap(); + assert_eq!(output_of(&mut s, "A1"), "object"); + } + // A run inside a script is recorded the same way. + s.execute("automate.newScript", json!({"name": "Switch", "commands": [{"command": "python.output", "params": {"cell": "A1"}}]})).unwrap(); + s.execute("macro.record", json!({"relative": false})).unwrap(); + s.execute("automate.runScript", json!({"name": "Switch"})).unwrap(); + s.execute("macro.record", json!({"stop": true, "name": "Rec2"})).unwrap(); + assert_eq!(output_of(&mut s, "A1"), "value"); + for _ in 0..2 { + s.execute("macro.run", json!({"name": "Rec2", "onConflict": "overwrite"})).unwrap(); + assert_eq!(output_of(&mut s, "A1"), "value"); + } +} diff --git a/crates/engine/tests/it/command_sequence_fuzz.rs b/crates/engine/tests/it/command_sequence_fuzz.rs index c32190dcd..89b3e9186 100644 --- a/crates/engine/tests/it/command_sequence_fuzz.rs +++ b/crates/engine/tests/it/command_sequence_fuzz.rs @@ -793,6 +793,7 @@ const CURATED: &[(&str, usize)] = &[ ("formulas.deleteName", 1), ("edit.replace", 1), ("python.set", 3), + ("python.output", 2), ]; fn generate(rng: &mut Rng, s: &Session) -> Step { @@ -845,6 +846,14 @@ fn curated_params(rng: &mut Rng, wb: &Workbook, id: &str) -> Json { } p } + "python.output" => { + // Mostly the cells python.set writes to; a missing mode switches. + let mut p = json!({"cell": near_cell(rng).a1()}); + if rng.chance(50) { + p["mode"] = json!(if rng.chance(50) { "object" } else { "value" }); + } + p + } "range.fill" => json!({"range": near_range(rng).a1(), "input": input(rng, wb)}), "selection.set" => { let mut p = json!({"range": range(rng)});