diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index 4712351ca..8409a3787 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -156,7 +156,7 @@ pub fn specs() -> Vec { "Row Height…", ["Home", "Cells", "Format"], None, - "{rows?: \"2:5\" | \"2:2,4:5\", height: 20 (points on screen)}", + "{sheet?, rows?: \"2:5\" | \"2:2,4:5\" | \"Sheet2!2:5\", height: 20 (points on screen)}", has_doc, row_height ), @@ -165,28 +165,34 @@ pub fn specs() -> Vec { "Column Width…", ["Home", "Cells", "Format"], None, - "{cols?: \"B:D\" | \"B:B,D:F\", width: 64 (points) | chars: 8.43}", + "{sheet?, cols?: \"B:D\" | \"B:B,D:F\" | \"Sheet2!B:D\", width: 64 (points) | chars: 8.43}", has_doc, col_width ), - cmd!("home.autofitRowHeight", "AutoFit Row Height", ["Home", "Cells", "Format"], None, "{rows?}", has_doc, autofit_rows), - cmd!("home.autofitColumnWidth", "AutoFit Column Width", ["Home", "Cells", "Format"], None, "{cols?}", has_doc, autofit_cols), - cmd!("home.defaultWidth", "Default Width…", ["Home", "Cells", "Format"], None, "{width: 64}", has_doc, default_width), - cmd!("home.hideRows", "Hide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+9"), "{rows?}", has_doc, |s, p| hide( + cmd!("home.autofitRowHeight", "AutoFit Row Height", ["Home", "Cells", "Format"], None, "{sheet?, rows?}", has_doc, autofit_rows), + cmd!("home.autofitColumnWidth", "AutoFit Column Width", ["Home", "Cells", "Format"], None, "{sheet?, cols?}", has_doc, autofit_cols), + cmd!("home.defaultWidth", "Default Width…", ["Home", "Cells", "Format"], None, "{sheet?, width: 64}", has_doc, default_width), + cmd!("home.hideRows", "Hide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+9"), "{sheet?, rows?}", has_doc, |s, p| hide( s, p, true, true )), - cmd!("home.hideColumns", "Hide Columns", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+0"), "{cols?}", has_doc, |s, p| hide( - s, p, false, true - )), - cmd!("home.unhideRows", "Unhide Rows", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+Shift+9"), "{rows?}", has_doc, |s, p| hide( - s, p, true, false - )), + cmd!("home.hideColumns", "Hide Columns", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+0"), "{sheet?, cols?}", has_doc, |s, p| { + hide(s, p, false, true) + }), + cmd!( + "home.unhideRows", + "Unhide Rows", + ["Home", "Cells", "Format", "Hide & Unhide"], + Some("Cmd+Shift+9"), + "{sheet?, rows?}", + has_doc, + |s, p| hide(s, p, true, false) + ), cmd!( "home.unhideColumns", "Unhide Columns", ["Home", "Cells", "Format", "Hide & Unhide"], Some("Cmd+Shift+0"), - "{cols?}", + "{sheet?, cols?}", has_doc, |s, p| hide(s, p, false, false) ), @@ -933,19 +939,37 @@ fn cell_style(s: &mut Session, p: &Json) -> Result { // ---------------------------------------------------------------- sizes -fn line_targets(s: &Session, p: &Json, rows: bool) -> Result> { +/// The sheet and the row (or column) spans a size or visibility command works on. The lines are +/// `rows`/`cols` (`"2:5"`, `"B:B,D:F"`, optionally `Sheet!`-prefixed), else the selection's; the +/// sheet is `sheet`, else the prefix, else the active one. The selection belongs to the active +/// sheet, so another sheet needs explicit lines. +fn line_targets(s: &Session, p: &Json, rows: bool) -> Result<(usize, Vec<(u32, u32)>)> { let key = if rows { "rows" } else { "cols" }; + let mut sheet = target_sheet(s, p)?; if let Some(t) = str_param(p, key) { + let (prefix, body) = split_sheet(t); + if let Some(name) = prefix { + let named = s.doc()?.wb.sheet_index(&name).ok_or_else(|| bad(key, format!("no sheet named `{name}`")))?; + if p.get("sheet").is_some_and(|v| !v.is_null()) && named != sheet { + return Err(bad(key, format!("`{t}` names a sheet other than `sheet`"))); + } + sheet = named; + } // A comma-separated list ("B:B,D:F") targets several areas, like a multi-area selection. - return t + let lines = body .split(',') .map(|part| { let r = RangeRef::parse(part).ok_or_else(|| bad(key, format!("not a range: {part}")))?; Ok(if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }) }) - .collect(); + .collect::>>()?; + return Ok((sheet, lines)); } - Ok(s.doc()?.selection.ranges.iter().map(|r| if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }).collect()) + let d = s.doc()?; + if sheet != d.wb.active_sheet { + return Err(bad(key, format!("missing `{key}`: the selection is on the active sheet"))); + } + Ok((sheet, d.selection.ranges.iter().map(|r| if rows { (r.start.row, r.end.row) } else { (r.start.col, r.end.col) }).collect())) } fn row_height(s: &mut Session, p: &Json) -> Result { @@ -953,9 +977,8 @@ fn row_height(s: &mut Session, p: &Json) -> Result { if !(0.0..=546.0).contains(&h) { return Err(EngineError::Other("Row height must be between 0 and 409 points.".into())); } - let lines = line_targets(s, p, true)?; + let (si, lines) = line_targets(s, p, true)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; for (a, b) in lines { for r in a..=b.min(a.saturating_add(1_048_575)) { @@ -979,9 +1002,8 @@ fn col_width(s: &mut Session, p: &Json) -> Result { if !(0.0..=1800.0).contains(&w) { return Err(EngineError::Other("Column width must be between 0 and 255 characters.".into())); } - let lines = line_targets(s, p, false)?; + let (si, lines) = line_targets(s, p, false)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; for (a, b) in lines { for c in a..=b { @@ -1026,9 +1048,8 @@ pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { } fn autofit_cols(s: &mut Session, p: &Json) -> Result { - let lines = line_targets(s, p, false)?; + let (si, lines) = line_targets(s, p, false)?; edit(s, |cx| { - let si = cx.sheet_index(); let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; let mut widths = Vec::new(); let used = sh.used_range(); @@ -1065,9 +1086,8 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { } fn autofit_rows(s: &mut Session, p: &Json) -> Result { - let lines = line_targets(s, p, true)?; + let (si, lines) = line_targets(s, p, true)?; edit(s, |cx| { - let si = cx.sheet_index(); for (a, b) in lines { let b = b.min(a + 100_000); if let Some(sh) = cx.wb.sheet_mut(si) { @@ -1087,17 +1107,16 @@ fn autofit_rows(s: &mut Session, p: &Json) -> Result { fn default_width(s: &mut Session, p: &Json) -> Result { let w = f64_param(p, "width").ok_or_else(|| bad("home.defaultWidth", "missing `width`"))?; + let si = target_sheet(s, p)?; edit(s, |cx| { - let si = cx.sheet_index(); cx.sheet_mut(si)?.default_col_width = w.clamp(0.0, 1800.0) as f32; Ok(Json::Null) }) } fn hide(s: &mut Session, p: &Json, rows: bool, on: bool) -> Result { - let lines = line_targets(s, p, rows)?; + let (si, lines) = line_targets(s, p, rows)?; edit(s, |cx| { - let si = cx.sheet_index(); let sh = cx.sheet_mut(si)?; for (a, b) in lines { // Unhiding a single selected line also reveals hidden neighbours (Excel needs the diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index edcacf57d..8c9389543 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -13,16 +13,25 @@ pub fn specs() -> Vec { "Freeze Panes", ["View", "Window", "Freeze Panes"], None, - "{cell?: \"B2\" (rows above and columns left of it freeze)}", + "{sheet?, cell?: \"B2\" (rows above and columns left of it freeze); without `cell`, unfreezes a frozen sheet}", has_doc, freeze_panes ), - cmd!("view.freezeTopRow", "Freeze Top Row", ["View", "Window", "Freeze Panes"], None, "{}", has_doc, |s, _| set_freeze(s, Some((1, 0)))), - cmd!("view.freezeFirstColumn", "Freeze First Column", ["View", "Window", "Freeze Panes"], None, "{}", has_doc, |s, _| set_freeze( + cmd!("view.freezeTopRow", "Freeze Top Row", ["View", "Window", "Freeze Panes"], None, "{sheet?}", has_doc, |s, p| set_freeze( s, + target_sheet(s, p)?, + Some((1, 0)) + )), + cmd!("view.freezeFirstColumn", "Freeze First Column", ["View", "Window", "Freeze Panes"], None, "{sheet?}", has_doc, |s, p| set_freeze( + s, + target_sheet(s, p)?, Some((0, 1)) )), - cmd!("view.unfreezePanes", "Unfreeze Panes", ["View", "Window", "Freeze Panes"], None, "{}", has_doc, |s, _| set_freeze(s, None)), + cmd!("view.unfreezePanes", "Unfreeze Panes", ["View", "Window", "Freeze Panes"], None, "{sheet?}", has_doc, |s, p| set_freeze( + s, + target_sheet(s, p)?, + None + )), cmd!("view.gridlines", "Gridlines", ["View", "Show"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "gridlines")), cmd!("view.headings", "Headings", ["View", "Show"], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "headings")), cmd!("view.showZeros", "Show Zeros", [], None, "{on?}", has_doc, |s, p| sheet_flag(s, p, "zeros")), @@ -113,23 +122,28 @@ pub fn specs() -> Vec { } fn freeze_panes(s: &mut Session, p: &Json) -> Result { + let si = target_sheet(s, p)?; let d = s.doc()?; - let sh = d.wb.active().ok_or(EngineError::NoDocument)?; + let sh = d.wb.sheet(si).ok_or(EngineError::NoDocument)?; if sh.freeze.is_some() && p.get("cell").is_none() { - return set_freeze(s, None); + return set_freeze(s, si, None); } - let at = cell_param(s, p, "cell")?.unwrap_or(d.selection.active); + let at = match cell_param_on(s, p, "cell", si)? { + Some(c) => c, + None if si == d.wb.active_sheet => d.selection.active, + // The selection belongs to the active sheet. + None => return Err(bad("view.freezePanes", "missing `cell`: the selection is on the active sheet")), + }; let top = sh.view_top_left; let rows = at.row.saturating_sub(top.row); let cols = at.col.saturating_sub(top.col); let _ = (rows, cols); let (rows, cols) = (at.row, at.col); - set_freeze(s, if rows == 0 && cols == 0 { None } else { Some((rows, cols)) }) + set_freeze(s, si, if rows == 0 && cols == 0 { None } else { Some((rows, cols)) }) } -fn set_freeze(s: &mut Session, f: Option<(u32, u32)>) -> Result { +fn set_freeze(s: &mut Session, si: usize, f: Option<(u32, u32)>) -> Result { edit(s, |cx| { - let si = cx.sheet_index(); cx.sheet_mut(si)?.freeze = f; Ok(json!({"freeze": f})) }) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 2ac290654..a1917643f 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3098,6 +3098,49 @@ fn row_height_and_column_width_take_a_list_of_areas() { assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(1), 120.0, "a bad list changes nothing"); } +/// Programmatic callers lay out a sheet that isn't the active one: `sheet` (or a `Sheet!` prefix on +/// `rows`, `cols` or `cell`) picks it, instead of being ignored in favour of the active sheet. +#[test] +fn line_sizes_and_freeze_panes_take_a_sheet() { + let mut s = s(); + s.execute("home.insertSheet", json!({"name": "Data"})).unwrap(); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + let sheet = |s: &Session, i: usize| s.doc().unwrap().wb.sheets[i].clone(); + let before = sheet(&s, 0); + + s.execute("view.freezePanes", json!({"sheet": "Data", "cell": "A2"})).unwrap(); + s.execute("home.columnWidth", json!({"sheet": "Data", "cols": "A:A", "width": 150})).unwrap(); + s.execute("home.columnWidth", json!({"cols": "Data!B:B,D:D", "chars": 20})).unwrap(); + // A null `sheet` is no `sheet`, as everywhere else: the prefix decides. + s.execute("home.columnWidth", json!({"sheet": null, "cols": "Data!C:C", "chars": 20})).unwrap(); + s.execute("home.rowHeight", json!({"sheet": 1, "rows": "1:1", "height": 40})).unwrap(); + s.execute("home.hideColumns", json!({"sheet": "Data", "cols": "E:E"})).unwrap(); + s.execute("home.defaultWidth", json!({"sheet": "Data", "width": 80})).unwrap(); + let data = sheet(&s, 1); + assert_eq!(data.freeze, Some((1, 0))); + assert_eq!((data.col_width(0), data.col_width(1), data.col_width(2), data.col_width(3)), (150.0, 145.0, 145.0, 145.0)); + assert_eq!(data.row_height(0), 40.0); + assert!(data.cols.get(&4).is_some_and(|c| c.hidden)); + assert_eq!(data.default_col_width, 80.0); + assert_eq!(sheet(&s, 0), before, "the active sheet is untouched"); + assert_eq!(s.doc().unwrap().wb.active_sheet, 0); + + s.execute("view.freezePanes", json!({"cell": "Data!B3"})).unwrap(); + assert_eq!(sheet(&s, 1).freeze, Some((2, 1))); + s.execute("view.unfreezePanes", json!({"sheet": "Data"})).unwrap(); + s.execute("view.freezeTopRow", json!({"sheet": "Data"})).unwrap(); + assert_eq!(sheet(&s, 1).freeze, Some((1, 0))); + + // The selection belongs to the active sheet: another sheet needs explicit lines or a cell, and + // a prefix can't contradict `sheet`. + assert!(s.execute("home.columnWidth", json!({"sheet": "Data", "width": 10})).is_err()); + s.execute("view.unfreezePanes", json!({"sheet": "Data"})).unwrap(); + assert!(s.execute("view.freezePanes", json!({"sheet": "Data"})).is_err()); + assert!(s.execute("home.columnWidth", json!({"sheet": "Sheet1", "cols": "Data!B:B", "width": 10})).is_err()); + assert!(s.execute("home.rowHeight", json!({"rows": "Nope!1:1", "height": 10})).is_err()); + assert_eq!(sheet(&s, 0), before); +} + #[test] fn copying_areas_that_share_rows_or_columns_pastes_them_as_one_block() { // Issue #348: Excel slides lined-up areas together; formulas travel as their values. diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index de6bbc6c5..c699ccf69 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -466,7 +466,6 @@ fn other_sheet_prefix_is_error() { ok(&mut s, "execute_command", json!({"command": "sheet.activate", "params": {"sheet": 0}})); fails(&mut s, "set_cell", json!({"cell": "Nope!B2", "input": "x"})); // A command that cannot honour a sheet prefix must not drop it and use the active sheet. - cmd_fails(&mut s, "view.freezePanes", json!({"cell": "Other!B2"})); cmd_fails(&mut s, "formulas.tracePrecedents", json!({"cell": "Other!B2"})); ok(&mut s, "set_cell", json!({"cell": "Sheet1!B2", "input": "here"})); assert_eq!(ok(&mut s, "get_cell", json!({"cell": "B2"}))["text"], "here"); @@ -498,7 +497,10 @@ fn cross_sheet_cell_refs_resolve() { // The active sheet may be a quoted name with spaces. ok(&mut s, "execute_command", json!({"command": "sheet.activate", "params": {"sheet": "My Sheet"}})); ok(&mut s, "execute_command", json!({"command": "view.freezePanes", "params": {"cell": "'My Sheet'!B2"}})); - cmd_fails(&mut s, "view.freezePanes", json!({"cell": "Other!B2"})); + // Freeze Panes honours another sheet's prefix (or `sheet`) and leaves the active sheet alone. + ok(&mut s, "execute_command", json!({"command": "view.freezePanes", "params": {"cell": "Other!C4"}})); + let sheets = ok(&mut s, "inspect_workbook", json!({}))["sheets"].clone(); + assert_eq!((sheets[1]["freeze"].clone(), sheets[2]["freeze"].clone()), (json!([3, 2]), json!([1, 1])), "{sheets}"); // A prefix naming a different sheet than the `sheet` argument is an error, not resolved in favour of either. cmd_fails(&mut s, "cell.get", json!({"cell": "Other!B3", "sheet": "Sheet1"})); }