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/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))?;

crates/engine/src/cmd/mod.rs

@@ -394,8 +394,14 @@ pub(crate) fn color_value(v: &Value) -> Option<deckcraft_model::ColorRef> {
             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());
     }
 }

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())

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.

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<Hit> {
     let x = xfrm_of(&st.doc, &st.selection, sh);
     let inv = x.affine().inverse();