collective
Browse / github.com/storytold/deckcraft / patch
table.cellAlign takes a horizontal align; numbering restarts at a paragraph's own start; style commands with nothing to set are refused
table.cellAlign takes a horizontal align; numbering restarts at a paragraph's own start; style commands with nothing to set are refused Three fixes from agent runs on tables and text (the last one part of #298: commands answer agents in words). - `table.cellAlign {align: left|center|right|justify}` sets every paragraph of the target cells; `anchor` stays the vertical alignment and is optional when `align` is given. - A paragraph with its own numbering start begins a new run at that number: layout used to add the start to the running count, so `start: k` set paragraph by paragraph read 1, 3, 5. A change of start between neighbouring numbered paragraphs now restarts the count, as PowerPoint does. - `shape.line`, `shape.effects`
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- zberry (imported from pull request #1323, open)
- Base
38d2ade2b77de39c0e06278ddfd44d1082ff80a8- Size
- +120 −12 in 7 files
- Review
- pending
- Shared
- 2026-10-28
- Id
40f07e8db5d0b47fbb8e2a9d
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/format.rs crates/engine/src/cmd/mod.rs crates/engine/src/cmd/shape.rs crates/engine/src/cmd/table.rs crates/engine/src/tests.rs crates/text/src/lib.rs crates/text/src/tests.rs
@@ -145,7 +145,7 @@ pub fn specs() -> Vec<CommandSpec> { "Numbering", ["Home", "Paragraph"], None,- "{on?: bool, scheme?: arabicPeriod|arabicParenR|romanUcPeriod|romanLcPeriod|alphaUcPeriod|alphaLcParenR|alphaLcPeriod, start?: n, color?, size?: % of text}",+ "{on?: bool, scheme?: arabicPeriod|arabicParenR|romanUcPeriod|romanLcPeriod|alphaUcPeriod|alphaLcParenR|alphaLcPeriod, start?: n (a paragraph whose start differs from the one before begins a new run of numbers at n, as PowerPoint's Start at does), color?, size?: % of text}", has_text_or_shapes, numbering ),@@ -883,6 +883,7 @@ fn line_spacing(s: &mut Session, p: &Value) -> Result<Value> { } fn paragraph(s: &mut Session, p: &Value) -> Result<Value> {+ require_any(p, "format.paragraph", &["align", "indentLeft", "indentFirst", "spaceBefore", "spaceAfter", "lineSpacing", "lineSpacingPt"])?; let a = str_param(p, "align").map(|a| match a { "center" => Align::Center, "right" => Align::Right,@@ -1132,6 +1133,7 @@ fn wrap(s: &mut Session, p: &Value) -> Result<Value> { format_body(s, &move |b| b.wrap = Some(on)) } fn margins(s: &mut Session, p: &Value) -> Result<Value> {+ require_any(p, "format.margins", &["left", "top", "right", "bottom"])?; let (l, t, r, b) = (f64_param(p, "left"), f64_param(p, "top"), f64_param(p, "right"), f64_param(p, "bottom")); format_body(s, &move |bp| { if l.is_some() {@@ -297,6 +297,16 @@ pub fn parse_params(doc: &str) -> std::result::Result<Vec<Param<'_>>, String> { /// growing would duplicate ids (#407). pub(crate) const NO_ID_SPACE: &str = "the presentation has used every object id and can hold no more"; +/// A style command whose keys are all optional is refused when it is given none of them, rather+/// than answering `changed` for a call that sets nothing.+pub(crate) fn require_any(p: &Value, cmd: &str, keys: &[&str]) -> Result<()> {+ if keys.iter().any(|k| p.get(*k).is_some_and(|v| !v.is_null())) {+ return Ok(());+ }+ let (last, rest) = keys.split_last().map_or(("", &[][..]), |(l, r)| (*l, r));+ Err(bad(cmd, format!("`{cmd}` was given nothing to set: pass {} or {last}", rest.join(", "))))+}+ pub(crate) fn bad(cmd: &str, msg: impl Into<String>) -> EngineError { EngineError::BadParams { cmd: cmd.into(), msg: msg.into() } }@@ -683,6 +683,7 @@ fn line_end(v: &str) -> Option<LineEnd> { } fn line(s: &mut Session, p: &Value) -> Result<Value> {+ require_any(p, "shape.line", &["color", "none", "width", "dash", "cap", "join", "compound", "head", "tail", "headSize", "tailSize", "reset"])?; let reset = bool_or(p, "reset", false); let none = bool_or(p, "none", false); let color = color_param(p, "color");@@ -802,6 +803,11 @@ fn shadow_preset(v: &Value) -> Option<Option<Shadow>> { } fn effects(s: &mut Session, p: &Value) -> Result<Value> {+ require_any(+ p,+ "shape.effects",+ &["shadow", "glow", "softEdges", "reflection", "bevel", "threeD", "shadowProps", "glowProps", "reflectionProps", "reset"],+ )?; let reset = bool_or(p, "reset", false); let shadow = p.get("shadow").and_then(shadow_preset); let glow: Option<Option<Glow>> = p.get("glow").map(|g| match g {@@ -1085,6 +1091,7 @@ fn crop(s: &mut Session, p: &Value) -> Result<Value> { } fn picture_adjust(s: &mut Session, p: &Value) -> Result<Value> {+ require_any(p, "picture.adjust", &["brightness", "contrast", "saturation", "grayscale", "transparency", "reset"])?; edit_shapes(s, p, "picture.adjust", |sh| { if let deckcraft_model::ShapeKind::Picture { fill } = &mut sh.kind { let a = &mut fill.adjust;@@ -87,7 +87,7 @@ pub fn specs() -> Vec<CommandSpec> { "Align Cell Text", ["Table Layout", "Alignment"], None,- "{anchor: top|middle|bottom, cells?, id?}",+ "{anchor?: top|middle|bottom (vertical; top when neither anchor nor align is given), align?: left|center|right|justify (horizontal, applied to every paragraph of the cells), cells?, id?}", has_selection, cell_align ),@@ -499,18 +499,37 @@ fn cell_margins(s: &mut Session, p: &Value) -> Result<Value> { } fn cell_align(s: &mut Session, p: &Value) -> Result<Value> {- use deckcraft_model::text::Anchor;- let a = match str_param(p, "anchor").unwrap_or("top") {- "middle" | "center" | "ctr" => Anchor::Middle,- "bottom" | "b" => Anchor::Bottom,- "top" | "t" => Anchor::Top,- o => return Err(bad("table.cellAlign", format!("unknown anchor {o:?}; one of top, middle, bottom"))),+ use deckcraft_model::text::{Align, Anchor};+ // Without `align`, the vertical anchor is the whole command (top when not given).+ let align = match str_param(p, "align") {+ None => None,+ Some("left" | "l") => Some(Align::Left),+ Some("center" | "centre" | "ctr") => Some(Align::Center),+ Some("right" | "r") => Some(Align::Right),+ Some("justify" | "just") => Some(Align::Justify),+ Some(o) => return Err(bad("table.cellAlign", format!("unknown align {o:?}; one of left, center, right, justify"))),+ };+ let anchor = match (str_param(p, "anchor"), align) {+ (None, Some(_)) => None,+ (a, _) => Some(match a.unwrap_or("top") {+ "middle" | "center" | "ctr" => Anchor::Middle,+ "bottom" | "b" => Anchor::Bottom,+ "top" | "t" => Anchor::Top,+ o => return Err(bad("table.cellAlign", format!("unknown anchor {o:?}; one of top, middle, bottom"))),+ }), }; let targets = target_cells(s, p)?; with_table(s, p, "table.cellAlign", move |t| { for (r, c) in targets(t) { if let Some(cell) = t.cell_mut(r, c) {- cell.anchor = Some(a);+ if anchor.is_some() {+ cell.anchor = anchor;+ }+ if let Some(a) = align {+ for para in &mut cell.text.paragraphs {+ para.props.align = Some(a);+ }+ } } } Ok(())@@ -4724,3 +4724,38 @@ fn slide_hide_and_layout_reject_invalid_index() { } assert!(s.doc().unwrap().doc.slides.iter().all(|x| !x.hidden)); }++#[test]+fn table_cell_align_sets_the_horizontal_alignment() {+ use deckcraft_model::text::{Align, Anchor};+ let mut s = session();+ let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"]]));+ s.execute("table.cellAlign", &json!({"id": id.0, "align": "center", "cells": [[0, 0], [1, 0]]})).unwrap();+ s.execute("table.cellAlign", &json!({"id": id.0, "align": "right", "anchor": "bottom", "cells": [[0, 1]]})).unwrap();+ let t = table_of(&s, id);+ let al = |r, c| t.cell(r, c).unwrap().text.paragraphs.iter().map(|p| p.props.align).collect::<Vec<_>>();+ assert_eq!(al(0, 0), vec![Some(Align::Center)]);+ assert_eq!(al(1, 0), vec![Some(Align::Center)]);+ assert_eq!(al(0, 1), vec![Some(Align::Right)]);+ assert_eq!(al(1, 1), vec![None], "other cells are untouched");+ assert_eq!(t.cell(0, 1).unwrap().anchor, Some(Anchor::Bottom));+ assert_eq!(t.cell(0, 0).unwrap().anchor, None, "align alone leaves the vertical anchor");+ assert!(s.execute("table.cellAlign", &json!({"id": id.0, "align": "diagonal"})).is_err());+}++#[test]+fn style_commands_given_nothing_to_set_are_refused() {+ let mut s = session();+ s.execute("insert.textBox", &json!({"text": "Hi", "rect": [50.0, 50.0, 200.0, 100.0]})).unwrap();+ for cmd in ["shape.line", "shape.effects", "picture.adjust", "format.paragraph", "format.margins"] {+ let rev = s.doc().unwrap().revision;+ let e = s.execute(cmd, &json!({})).unwrap_err().to_string();+ assert!(e.contains("given nothing to set") && e.contains(cmd), "{cmd}: {e}");+ assert_eq!(s.doc().unwrap().revision, rev, "{cmd} changed the deck");+ }+ let e = s.execute("shape.line", &json!({})).unwrap_err().to_string();+ assert!(e.contains("pass color, none, width, dash"), "{e}");+ // A key, even one that changes nothing visible, is something to set.+ s.execute("shape.line", &json!({"width": 2})).unwrap();+ s.execute("format.margins", &json!({"left": 5})).unwrap();+}@@ -1871,6 +1871,9 @@ fn layout_scaled(ctx: &Ctx, shape: &Shape, body: &TextBody, bp: &BodyProps, opts let mut out = TextLayout { inner, font_scale: scale, line_reduction: reduce, rotation, ..Default::default() }; let mut y = 0.0; let mut numbering: Vec<u32> = vec![0; 9];+ // The `start_at` each level's current run of numbers began with: a paragraph that changes it+ // starts a new run (PowerPoint's "Start at").+ let mut run_start: Vec<u32> = vec![0; 9]; let mut all_lines: Vec<(LineInfo, LinePlace, f64, Option<(String, f64, f64)>)> = vec![]; let mut prev_after = 0.0; // How far the last line's full pitch reaches past the text (exact spacing; see below).@@ -1881,6 +1884,12 @@ fn layout_scaled(ctx: &Ctx, shape: &Shape, body: &TextBody, bp: &BodyProps, opts let lvl = para.level.min(8) as usize; let pp = resolve::para(ctx, shape, para); let number = if matches!(pp.bullet, Some(Bullet::AutoNum { .. })) && !para.is_empty() {+ if let (Some(Bullet::AutoNum { start_at, .. }), Some(rs), Some(n)) = (&pp.bullet, run_start.get_mut(lvl), numbering.get_mut(lvl))+ && *rs != *start_at+ {+ *rs = *start_at;+ *n = 0;+ } if let Some(n) = numbering.get_mut(lvl) { *n += 1; }@@ -1888,10 +1897,11 @@ fn layout_scaled(ctx: &Ctx, shape: &Shape, body: &TextBody, bp: &BodyProps, opts *deeper = 0; } numbering.get(lvl).copied()- } else if matches!(pp.bullet, Some(Bullet::AutoNum { .. })) {+ } else if let Some(Bullet::AutoNum { start_at, .. }) = &pp.bullet { // Preview the next number without consuming it: empty paragraphs do not advance- // numbering in the saved document or affect later populated paragraphs.- numbering.get(lvl).map(|n| n.saturating_add(1))+ // numbering in the saved document or affect later populated paragraphs. A paragraph+ // with its own start would begin a new run, so it previews that run's first number.+ if run_start.get(lvl).is_some_and(|rs| *rs != *start_at) { Some(1) } else { numbering.get(lvl).map(|n| n.saturating_add(1)) } } else { if !para.is_empty() { for n in numbering.iter_mut().skip(lvl) {@@ -1840,3 +1840,28 @@ fn an_empty_paragraph_keeps_its_paragraph_mark_height() { let h = l.lines[1].bottom - l.lines[1].top; assert!((h - h10).abs() < 0.5, "a 10pt empty paragraph should give {h10}, got {h}"); }++#[test]+fn a_paragraph_with_its_own_start_at_restarts_the_count_there() {+ // PowerPoint's "Start at" on a paragraph begins a new run of numbers at that value, so setting+ // start k on paragraph k reads 1, 2, 3, not 1, 3, 5.+ // The bullets' glyph ids next to those of the expected labels, as pairs (drawn, expected).+ let check = |starts: &[u32], expect: &[u32]| {+ let (p, mut sh) = setup("a\nb\nc\nd", 600.0, 300.0);+ for (para, st) in sh.text.as_mut().unwrap().paragraphs.iter_mut().zip(starts) {+ para.props.bullet = Some(Bullet::AutoNum { scheme: "arabicPeriod".into(), start_at: *st });+ }+ let l = lay(&p, &sh, 600.0, 300.0);+ let bullets: Vec<_> = l.runs.iter().filter(|r| r.cells.is_empty()).collect();+ assert_eq!(bullets.len(), expect.len());+ for (b, n) in bullets.iter().zip(expect) {+ let want = measure_str(&b.face, b.size, &format!("{n}.")).1.iter().map(|g| g.0).collect::<Vec<_>>();+ assert_eq!(b.glyphs.iter().map(|g| g.0).collect::<Vec<_>>(), want, "expected {n}. for starts {starts:?}");+ }+ };+ check(&[1, 2, 3, 4], &[1, 2, 3, 4]);+ // The same start on every paragraph still counts on from it (a whole box numbered at once).+ check(&[3, 3, 3, 3], &[3, 4, 5, 6]);+ // A later paragraph with a new start begins a fresh run.+ check(&[1, 1, 7, 7], &[1, 2, 7, 8]);+}