From a0eeb341d2790be1056e472774cb379d8bf16566 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:09:38 -0400 Subject: [PATCH] Colour names, media ids after a paste, and pressing a table outside its cells (#1344 follow-ups) (#1348) * Colors: unknown preset and system colour names are rejected, and the writer saves black or windowText for them * Gaps: copying text and cell ranges, list AutoFormat's option and Backspace, and a timing-sensitive control-server test * Paste and Compare: media kept under a foreign id raises the next id, so a later picture gets an id of its own * Tables: with a cell range selected, a press on the table outside its cells selects and moves the table * Tables: a click on a selected table's grab band keeps the table selected instead of entering the nearest cell --- crates/color/src/lib.rs | 238 ++++++++++++++++++++++++++++++++++ crates/engine/src/cmd/edit.rs | 6 +- crates/engine/src/cmd/mod.rs | 12 ++ crates/engine/src/compare.rs | 6 +- crates/engine/src/tests.rs | 67 ++++++++++ crates/engine/src/tools.rs | 24 +++- crates/pptx/src/write/dml.rs | 25 +++- docs/gaps.md | 21 ++- 8 files changed, 387 insertions(+), 12 deletions(-) diff --git a/crates/color/src/lib.rs b/crates/color/src/lib.rs index fe02a975..0056078f 100644 --- a/crates/color/src/lib.rs +++ b/crates/color/src/lib.rs @@ -515,6 +515,244 @@ pub fn preset(name: &str) -> Option { Some(Rgba::rgb(c.0, c.1, c.2)) } +/// Every preset colour name a file may write (`ST_PresetColorVal`, with the later editions' aliases). +pub const PRESET_NAMES: [&str; 190] = [ + "aliceBlue", + "antiqueWhite", + "aqua", + "aquamarine", + "azure", + "beige", + "bisque", + "black", + "blanchedAlmond", + "blue", + "blueViolet", + "brown", + "burlyWood", + "cadetBlue", + "chartreuse", + "chocolate", + "coral", + "cornflowerBlue", + "cornsilk", + "crimson", + "cyan", + "darkBlue", + "darkCyan", + "darkGoldenrod", + "darkGray", + "darkGrey", + "darkGreen", + "darkKhaki", + "darkMagenta", + "darkOliveGreen", + "darkOrange", + "darkOrchid", + "darkRed", + "darkSalmon", + "darkSeaGreen", + "darkSlateBlue", + "darkSlateGray", + "darkSlateGrey", + "darkTurquoise", + "darkViolet", + "dkBlue", + "dkCyan", + "dkGoldenrod", + "dkGray", + "dkGrey", + "dkGreen", + "dkKhaki", + "dkMagenta", + "dkOliveGreen", + "dkOrange", + "dkOrchid", + "dkRed", + "dkSalmon", + "dkSeaGreen", + "dkSlateBlue", + "dkSlateGray", + "dkSlateGrey", + "dkTurquoise", + "dkViolet", + "deepPink", + "deepSkyBlue", + "dimGray", + "dimGrey", + "dodgerBlue", + "firebrick", + "floralWhite", + "forestGreen", + "fuchsia", + "gainsboro", + "ghostWhite", + "gold", + "goldenrod", + "gray", + "grey", + "green", + "greenYellow", + "honeydew", + "hotPink", + "indianRed", + "indigo", + "ivory", + "khaki", + "lavender", + "lavenderBlush", + "lawnGreen", + "lemonChiffon", + "lightBlue", + "lightCoral", + "lightCyan", + "lightGoldenrodYellow", + "lightGray", + "lightGrey", + "lightGreen", + "lightPink", + "lightSalmon", + "lightSeaGreen", + "lightSkyBlue", + "lightSlateGray", + "lightSlateGrey", + "lightSteelBlue", + "lightYellow", + "ltBlue", + "ltCoral", + "ltCyan", + "ltGoldenrodYellow", + "ltGray", + "ltGrey", + "ltGreen", + "ltPink", + "ltSalmon", + "ltSeaGreen", + "ltSkyBlue", + "ltSlateGray", + "ltSlateGrey", + "ltSteelBlue", + "ltYellow", + "lime", + "limeGreen", + "linen", + "magenta", + "maroon", + "medAquamarine", + "medBlue", + "medOrchid", + "medPurple", + "medSeaGreen", + "medSlateBlue", + "medSpringGreen", + "medTurquoise", + "medVioletRed", + "mediumAquamarine", + "mediumBlue", + "mediumOrchid", + "mediumPurple", + "mediumSeaGreen", + "mediumSlateBlue", + "mediumSpringGreen", + "mediumTurquoise", + "mediumVioletRed", + "midnightBlue", + "mintCream", + "mistyRose", + "moccasin", + "navajoWhite", + "navy", + "oldLace", + "olive", + "oliveDrab", + "orange", + "orangeRed", + "orchid", + "paleGoldenrod", + "paleGreen", + "paleTurquoise", + "paleVioletRed", + "papayaWhip", + "peachPuff", + "peru", + "pink", + "plum", + "powderBlue", + "purple", + "red", + "rosyBrown", + "royalBlue", + "saddleBrown", + "salmon", + "sandyBrown", + "seaGreen", + "seaShell", + "sienna", + "silver", + "skyBlue", + "slateBlue", + "slateGray", + "slateGrey", + "snow", + "springGreen", + "steelBlue", + "tan", + "teal", + "thistle", + "tomato", + "turquoise", + "violet", + "wheat", + "white", + "whiteSmoke", + "yellow", + "yellowGreen", +]; + +/// Every system colour name a file may write (`ST_SystemColorVal`). +pub const SYSTEM_NAMES: [&str; 30] = [ + "scrollBar", + "background", + "activeCaption", + "inactiveCaption", + "menu", + "window", + "windowFrame", + "menuText", + "windowText", + "captionText", + "activeBorder", + "inactiveBorder", + "appWorkspace", + "highlight", + "highlightText", + "btnFace", + "btnShadow", + "grayText", + "btnText", + "inactiveCaptionText", + "btnHighlight", + "3dDkShadow", + "3dLight", + "infoText", + "infoBk", + "hotLight", + "gradientActiveCaption", + "gradientInactiveCaption", + "menuHighlight", + "menuBar", +]; + +/// Whether `name` is a preset colour name a file may write. +pub fn is_preset_name(name: &str) -> bool { + PRESET_NAMES.contains(&name) +} + +/// Whether `name` is a system colour name a file may write. +pub fn is_system_name(name: &str) -> bool { + SYSTEM_NAMES.contains(&name) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index c7e2c81a..72061085 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -245,7 +245,11 @@ fn bring_media(cmd: &str, doc: &mut deckcraft_model::Presentation, media: &[deck let mut map = HashMap::new(); for m in media { match doc.media(m.id) { - None => doc.media.push(m.clone()), + None => { + // Keeps the foreign id, so later ids must start past it. + doc.media.push(m.clone()); + doc.next_id = doc.next_id.max(m.id.0); + } Some(x) if x.data == m.data => {} Some(_) => { let id = doc.add_media(&m.name, &m.content_type, m.data.as_ref().clone()).ok_or_else(|| bad(cmd, NO_ID_SPACE))?; diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index fcda1413..f0f1141f 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -394,8 +394,14 @@ pub(crate) fn color_value(v: &Value) -> Option { let mut c = if let Some(s) = o.get("scheme").and_then(Value::as_str) { ColorRef::scheme(deckcraft_color::SchemeSlot::from_xml(s)?) } else if let Some(name) = o.get("preset").and_then(Value::as_str) { + if !deckcraft_color::is_preset_name(name) { + return None; + } ColorRef { base: deckcraft_model::ColorBase::Preset { name: name.to_string() }, mods: vec![] } } else if let Some(name) = o.get("system").and_then(Value::as_str) { + if !deckcraft_color::is_system_name(name) { + return None; + } let last = deckcraft_color::Rgba::from_hex(o.get("last").and_then(Value::as_str)?)?; ColorRef { base: deckcraft_model::ColorBase::System { name: name.to_string(), last }, mods: vec![] } } else { @@ -657,5 +663,11 @@ mod tests { let system = color_value(&json!({"system": "windowText", "last": "#123456"})); assert_eq!(system, Some(ColorRef { base: ColorBase::System { name: "windowText".into(), last: Rgba::rgb(0x12, 0x34, 0x56) }, mods: vec![] })); assert_eq!(color_value(&json!({"system": "windowText"})), None, "a system color needs its last value"); + assert_eq!(color_value(&json!({"preset": "notAColor"})), None); + assert_eq!(color_value(&json!({"system": "notAColor", "last": "#000000"})), None); + for name in ["aliceBlue", "darkBlue", "ltSlateGrey"] { + assert!(color_value(&json!({"preset": name})).is_some(), "{name}"); + } + assert!(color_value(&json!({"system": "3dDkShadow", "last": "#000000"})).is_some()); } } diff --git a/crates/engine/src/compare.rs b/crates/engine/src/compare.rs index 5b7546c4..fa71dc1a 100644 --- a/crates/engine/src/compare.rs +++ b/crates/engine/src/compare.rs @@ -286,7 +286,11 @@ pub fn adopt(doc: &mut Presentation, rev: &Presentation, slide: &Slide, replacin let Some(m) = rev.media(id) else { continue }; match doc.media(id) { Some(x) if x.data == m.data => {} - None => doc.media.push(m.clone()), + None => { + // Keeps the revision's id, so later ids must start past it. + doc.media.push(m.clone()); + doc.next_id = doc.next_id.max(id.0); + } Some(_) => { let new = doc .add_media(&m.name, &m.content_type, m.data.as_ref().clone()) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index b7371fd2..770d341e 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -947,6 +947,34 @@ fn dragging_across_cells_selects_a_range_of_cells() { assert_eq!(cmd::table::cell_block(&t, (1, 1), (2, 2)), ((0, 0), (2, 2))); } +#[test] +fn pressing_a_table_outside_its_cells_with_a_range_selected_moves_the_table() { + let mut s = session(); + let id = table_at(&mut s, &[100.0, 100.0], json!([["a", "b"], ["c", "d"]])); + let drag = |s: &mut Session, kind, x: f64, y: f64| s.pointer(tev(kind, x, y)).unwrap(); + s.execute("text.edit", &json!({"id": id, "cell": [0, 0]})).unwrap(); + drag(&mut s, PointerKind::Down, 150.0, 120.0); + drag(&mut s, PointerKind::Drag, 250.0, 160.0); + drag(&mut s, PointerKind::Up, 250.0, 160.0); + assert_eq!(s.doc().unwrap().selection.cells, Some((id, (0, 0), (1, 1)))); + // Just left of the table's edge, where a press still hits the table but no cell. + drag(&mut s, PointerKind::Down, 98.0, 120.0); + assert_eq!(s.doc().unwrap().selection.cells, None, "the range ends"); + drag(&mut s, PointerKind::Drag, 148.0, 170.0); + drag(&mut s, PointerKind::Up, 148.0, 170.0); + let st = s.doc().unwrap(); + assert!(st.selection.text.is_none()); + assert_eq!(st.selection.shapes, vec![id]); + let x = st.shape(id).unwrap().xfrm.unwrap(); + assert!((x.x - 150.0).abs() < 10.0 && (x.y - 150.0).abs() < 10.0, "the table moves: {x:?}"); + // A click (no drag) there keeps the table selected and puts no caret in the nearest cell. + let (x0, y0) = (x.x - 2.0, x.y + 20.0); + drag(&mut s, PointerKind::Down, x0, y0); + drag(&mut s, PointerKind::Up, x0, y0); + let st = s.doc().unwrap(); + assert_eq!((st.selection.shapes.clone(), st.selection.text.is_none()), (vec![id], true)); +} + #[test] fn a_click_in_a_selected_table_enters_the_clicked_cell() { let mut s = session(); @@ -1970,6 +1998,45 @@ mod copy_remap { assert_eq!(img.get_pixel(img.width() / 2, img.height() / 2).0, [255, 0, 0, 255]); } + #[test] + fn pictures_inserted_after_a_paste_from_another_deck_get_media_ids_of_their_own() { + let mut a = Session::with_new(); + let mut b = Session::with_new(); + let (pa, ..) = populate(&mut a, b"deck a picture"); + // Deck a's picture uses a media id above its shape ids that deck b has not handed out yet. + { + let doc = Arc::make_mut(&mut a.doc_mut().unwrap().doc); + let high = MediaId(doc.next_id + 20); + doc.next_id = high.0; + doc.media[0].id = high; + let slide = Arc::make_mut(&mut doc.slides[0]); + let Some(ShapeKind::Picture { fill }) = slide.shapes.iter_mut().find(|x| x.id == pa).map(|x| &mut x.kind) else { panic!() }; + fill.media = high; + } + assert_eq!(picture_bytes(&a, &shape(&a, 0, pa)), b"deck a picture"); + select(&mut a, &[pa]); + a.execute("edit.copy", &json!({})).unwrap(); + b.clipboard = a.clipboard.clone(); + b.doc_mut().unwrap().selection.shapes.clear(); + let r = b.execute("edit.paste", &json!({})).unwrap(); + let pasted = ShapeId(r["ids"][0].as_u64().unwrap() as u32); + let mut inserted = vec![]; + for i in 0..12u8 { + let png = solid_png([i, 0, 0]); + let r = b.execute("insert.picture", &json!({"data": cmd::base64_encode(&png), "name": "a.png"})).unwrap(); + inserted.push((ShapeId(r["id"].as_u64().unwrap() as u32), png)); + } + assert_eq!(picture_bytes(&b, &shape(&b, 0, pasted)), b"deck a picture"); + for (id, png) in inserted { + assert_eq!(picture_bytes(&b, &shape(&b, 0, id)), png); + } + let d = b.doc().unwrap(); + let mut ids: Vec<_> = d.doc.media.iter().map(|m| m.id).collect(); + ids.sort(); + ids.dedup(); + assert_eq!(ids.len(), d.doc.media.len(), "media ids are unique"); + } + /// Slide 0 of `s` gets a table whose first cell, background and second cell's picture bullet /// each use a picture of their own, and a transition sound. `tag` starts the bytes of each. /// Returns the table. diff --git a/crates/engine/src/tools.rs b/crates/engine/src/tools.rs index a9c91f76..72393183 100644 --- a/crates/engine/src/tools.rs +++ b/crates/engine/src/tools.rs @@ -705,10 +705,13 @@ impl Session { } } Some(Hit::Shape { id, .. }) - if !(mods.shift || mods.cmd) && st.selection.cells.is_some_and(|c| c.0 == id && st.selection.shapes == [id]) => + if !(mods.shift || mods.cmd) + && st.selection.cells.is_some_and(|c| c.0 == id && st.selection.shapes == [id]) + && in_table_cell(st, id, p) => { // A press in a table whose cells are selected starts again in the cell under the - // pointer: a click puts the caret there and a drag selects another range. + // pointer: a click puts the caret there and a drag selects another range. A press + // on the table outside its cells selects and moves the table like any shape. let before = self.text_selection(); if self.edit_text_at(id, p)? { self.select(|_, sel| sel.cells = None)?; @@ -736,15 +739,17 @@ impl Session { let origs: Vec<(ShapeId, Xfrm)> = ids.iter().filter_map(|i| st.shape(*i).filter(|s| !s.locked).map(|s| (*i, xfrm_of(&st.doc, &st.selection, s)))).collect(); // A click (no drag) on text puts the caret there; unselected text boxes and - // placeholders take the caret straight away like PowerPoint. A click in a selected - // table puts the caret in the clicked cell. + // placeholders take the caret straight away like PowerPoint. A click in a cell of a + // selected table puts the caret there; a click on its grab band keeps the table selected. let table = st.shape(id).is_some_and(|s| matches!(s.kind, ShapeKind::Table(_))); // A click on the frame band of a text box selects the box whole, as PowerPoint // does, so it can be moved, duplicated or deleted; only the inside takes the caret. let on_frame = text && is_text_first(st, id) && st.shape(id).is_some_and(|s| edit_zone(&xfrm_of(&st.doc, &st.selection, s), p, tol) == EditZone::Frame); - let enter_text = (!toggle && !on_frame && ((text && (selected || is_text_first(st, id))) || (table && selected))).then_some((id, p)); + let enter_text = + (!toggle && !on_frame && ((text && (selected || is_text_first(st, id))) || (table && selected && in_table_cell(st, id, p)))) + .then_some((id, p)); let ids2 = ids.clone(); self.select(|_, sel| { sel.shapes = ids2; @@ -1990,6 +1995,15 @@ pub fn resize_box(o: &Xfrm, h: u8, d: Vec2, keep_aspect: bool, from_center: bool Xfrm { x: c.x - nw / 2.0, y: c.y - nh / 2.0, w: nw, h: nh, rot: o.rot, flip_h: fh, flip_v: fv } } +/// Whether `p` lies in a cell of table `id`, not in the grab band around it (`cell_at` would take +/// the nearest cell). +fn in_table_cell(st: &crate::DocState, id: ShapeId, p: Point) -> bool { + st.shape(id).and_then(|sh| cmd::table::table_frame(st, sh)).is_some_and(|f| { + let q = f.local(p); + (0.0..=f.width()).contains(&q.x) && (0.0..=f.height()).contains(&q.y) && f.cell_at(q).is_some() + }) +} + fn hit_shape(st: &crate::DocState, sh: &Shape, p: Point, tol: f64) -> Option { let x = xfrm_of(&st.doc, &st.selection, sh); let inv = x.affine().inverse(); diff --git a/crates/pptx/src/write/dml.rs b/crates/pptx/src/write/dml.rs index 09b298f8..518f8c4a 100644 --- a/crates/pptx/src/write/dml.rs +++ b/crates/pptx/src/write/dml.rs @@ -36,9 +36,10 @@ pub fn color(w: &mut W, c: &ColorRef) { let (tag, a) = match &c.base { ColorBase::Rgb { rgb } => ("a:srgbClr", A::new().a("val", rgb.hex())), ColorBase::Scheme { slot } => ("a:schemeClr", A::new().a("val", slot.xml_name())), - ColorBase::Preset { name } => ("a:prstClr", A::new().a("val", if name.is_empty() { "black" } else { name })), + // The reader keeps any name a file has; only names the schema allows are written. + ColorBase::Preset { name } => ("a:prstClr", A::new().a("val", if deckcraft_color::is_preset_name(name) { name } else { "black" })), ColorBase::System { name, last } => { - ("a:sysClr", A::new().a("val", if name.is_empty() { "windowText" } else { name }).a("lastClr", last.hex())) + ("a:sysClr", A::new().a("val", if deckcraft_color::is_system_name(name) { name } else { "windowText" }).a("lastClr", last.hex())) } }; let rgb_alpha = match &c.base { @@ -788,3 +789,23 @@ pub fn paragraph(w: &mut W, x: &mut Exp, o: &mut Out, p: &Paragraph, math: bool, pub fn has_math(tb: &TextBody) -> bool { tb.paragraphs.iter().flat_map(|p| &p.runs).any(|r| matches!(&r.kind, RunKind::Math { omml } if !omml.is_empty())) } + +#[cfg(test)] +mod tests { + use super::*; + use deckcraft_color::Rgba; + + #[test] + fn unknown_preset_and_system_names_save_as_black_and_window_text() { + let write = |base: ColorBase| { + let mut w = W::frag(); + color(&mut w, &ColorRef { base, mods: vec![] }); + w.s + }; + assert_eq!(write(ColorBase::Preset { name: "notAColor".into() }), r#""#); + assert_eq!(write(ColorBase::Preset { name: "darkBlue".into() }), r#""#); + let last = Rgba::rgb(0x12, 0x34, 0x56); + assert_eq!(write(ColorBase::System { name: "notAColor".into(), last }), r#""#); + assert_eq!(write(ColorBase::System { name: "3dLight".into(), last }), r#""#); + } +} diff --git a/docs/gaps.md b/docs/gaps.md index ee0a7735..c8dabe40 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -61,6 +61,12 @@ revision history. - **Impact:** long-session crash rate is sampled in CI; RSS is not measured directly. - **Estimate:** 1–2 h (RSS sampling per platform). +### A control-server timing test may fail under load +- **Evidence:** `silent_refused_peers_do_not_delay_the_next_refusal` in + `apps/deckcraft/src/control_server.rs` asserts that a refusal arrives in under 1 s, so it can + fail on a busy machine although the server behaves correctly. +- **Estimate:** under 1 h (a looser bound or a logical clock). + ## File formats Detail in [file-format-parity.md](file-format-parity.md). @@ -260,9 +266,17 @@ and starts at that effect. The pane has an advanced timeline with draggable bars `review.thesaurus` looks up the selected word (checklist row partial). AutoCorrect has PowerPoint's options (two initial capitals, sentence and day capitals, caps lock, fractions, an editable replace list), and AutoFormat superscripts ordinals (1st) and starts bulleted and - numbered lists (`* `, `1. `, Backspace puts the typed text back) as you type; no undo of a - single correction. -- **Estimate:** 1–2 h (bundle openly licensed dictionaries). + numbered lists (`* `, `1. `) as you type; no undo of a single correction. Automatic lists start + only while Replace text as you type is on, where PowerPoint has a separate AutoFormat option, + and Backspace right after one puts the typed lead back, which PowerPoint does not. +- **Estimate:** 1–2 h (bundle openly licensed dictionaries; under 1 h for the AutoFormat option). + +### Copying text and cell ranges +- **Evidence:** a copied text range carries no media, so a picture bullet in it is lost when + pasted into another deck; the writer saves a bullet whose picture is missing as `•`. A copied + range of table cells is tab-separated text, not a table, so pasting it makes text rather than + cells. +- **Estimate:** 2–4 h. ### Equations - **Evidence:** checklist row missing; PR #55 (OMML editor, +10.5k lines) is open. @@ -357,6 +371,7 @@ Detail in [localization-parity.md](localization-parity.md). | Date | Change | Summary | |---|---|---| +| 2026-10-25 | trivial | New entries: copying text and cell ranges (no media, no table), a control-server timing test that may fail under load; Proofing notes that automatic lists follow Replace text as you type and that Backspace puts the typed lead back | | 2026-10-24 | trivial | AutoFormat starts bulleted and numbered lists as you type (`* `, `-- `, `> `, `1. `, `a) ` and the rest), with an AutoCorrect option to turn it off | | 2026-10-22 | minor | Animation Pane ▸ Play From starts the preview at the selected effect; gap 10 keeps the effect gallery (7–13 h) | | 2026-10-24 | trivial | Columns dialog (number and spacing of text columns); dialog count 41 → 42 |