From 205ca7dfc0de2f6d61ac49954942dbf5f0ba9c23 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 24 Oct 2026 08:23:17 -0700 Subject: [PATCH] Text: the Columns dialog sends only the fields that changed With several text boxes selected, the dialog shows the first one's values. OK sent both Number and Spacing, so changing one of them also gave every box the first box's value of the other. OK now sends only the fields that differ from what the dialog showed, and with neither changed it closes without an edit. --- crates/ui-egui/src/columns_dialog_tests.rs | 59 ++++++++++++++++++++++ crates/ui-egui/src/dialogs.rs | 54 +++++++++++--------- 2 files changed, 90 insertions(+), 23 deletions(-) diff --git a/crates/ui-egui/src/columns_dialog_tests.rs b/crates/ui-egui/src/columns_dialog_tests.rs index fa6b677f..7737031c 100644 --- a/crates/ui-egui/src/columns_dialog_tests.rs +++ b/crates/ui-egui/src/columns_dialog_tests.rs @@ -12,6 +12,24 @@ fn harness() -> Harness<'static, SlideApp> { let mut s = Session::with_new(); s.execute("shape.insert", &json!({"preset": "rect", "rect": [80.0, 80.0, 320.0, 160.0], "text": "Hello"})).unwrap(); let _ = s.execute("text.exit", &json!({})); + harness_of(s) +} + +/// Two text boxes with different columns, both selected: 2 columns 9 pt apart, and 3 columns +/// 36 pt apart. +fn two_boxes() -> Harness<'static, SlideApp> { + let mut s = Session::with_new(); + let mut ids = Vec::new(); + for (y, count, spacing) in [(80.0, 2, 9.0), (300.0, 3, 36.0)] { + ids.push(s.execute("shape.insert", &json!({"preset": "rect", "rect": [80.0, y, 320.0, 160.0], "text": "Hello"})).unwrap()["id"].clone()); + let _ = s.execute("text.exit", &json!({})); + s.execute("format.columns", &json!({"count": count, "spacing": spacing})).unwrap(); + } + s.execute("edit.select", &json!({"ids": ids})).unwrap(); + harness_of(s) +} + +fn harness_of(s: Session) -> Harness<'static, SlideApp> { let app = SlideApp::new(s, Services::default()); let mut h = Harness::builder().with_size(egui::vec2(1440.0, 900.0)).with_step_dt(1.0 / 60.0).build_ui_state( |ui, app: &mut SlideApp| { @@ -30,6 +48,47 @@ fn columns_of(h: &Harness<'_, SlideApp>) -> (Option, Option) { (body.columns, body.col_spacing) } +fn columns_of_each(h: &Harness<'_, SlideApp>) -> Vec<(Option, Option)> { + let st = h.state().session.active().unwrap(); + st.selected_shapes().iter().map(|sh| sh.text.as_ref().map(|t| (t.body.columns, t.body.col_spacing)).unwrap_or_default()).collect() +} + +/// Opens the dialog on `h`'s selection, types `fields` into it and presses OK. +fn ok_with(h: &mut Harness<'_, SlideApp>, fields: &[(&str, &str)]) { + h.state_mut().run("format.columns", json!({})).unwrap(); + h.run_steps(2); + let d = h.state_mut().dialog.as_mut().unwrap(); + for (key, value) in fields { + d.fields.insert((*key).into(), (*value).into()); + } + h.get_by_label("OK").click_accesskit(); + h.run_steps(2); + assert!(h.state().dialog.is_none(), "status {:?}", h.state().status); +} + +/// With text boxes whose columns differ, the dialog shows the first one's values, and a field +/// left as shown does not overwrite the others' own values. +#[test] +fn a_field_left_as_shown_keeps_each_text_box_s_own_value() { + let mut h = two_boxes(); + assert_eq!(columns_of_each(&h), [(Some(2), Some(9.0)), (Some(3), Some(36.0))]); + ok_with(&mut h, &[("count", "4")]); + assert_eq!(columns_of_each(&h), [(Some(4), Some(9.0)), (Some(4), Some(36.0))], "Number only"); + + let mut h = two_boxes(); + ok_with(&mut h, &[("spacing", "0.5")]); + assert_eq!(columns_of_each(&h), [(Some(2), Some(36.0)), (Some(3), Some(36.0))], "Spacing only"); +} + +#[test] +fn ok_with_nothing_changed_closes_without_an_edit() { + let mut h = two_boxes(); + let n = h.state().session.active().unwrap().history.undo.len(); + ok_with(&mut h, &[]); + assert_eq!(h.state().session.active().unwrap().history.undo.len(), n); + assert_eq!(columns_of_each(&h), [(Some(2), Some(9.0)), (Some(3), Some(36.0))]); +} + fn field(h: &Harness<'_, SlideApp>, key: &str) -> Option { h.state().dialog.as_ref().and_then(|d| d.fields.get(key).cloned()) } diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 3e994589..9ecbdd5c 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -2664,31 +2664,35 @@ fn grid_options(app: &mut SlideApp, ui: &mut Ui, d: &mut Dialog) -> bool { cancel } -/// The Columns dialog's fields as `format.columns` params: Number and Spacing (inches, sent in points). -/// A Spacing left as the dialog showed it sends the text box's own spacing, unrounded. -fn columns_value(d: &Dialog, shown_spacing: &str) -> Result { - let count = - field_of(d, "count").trim().parse::().ok().filter(|n| (1..=16).contains(n)).ok_or("Number must be a whole number from 1 to 16")?; - let typed = field_of(d, "spacing"); - if typed == shown_spacing - && let Some(pt) = d.params.get("spacing").and_then(Value::as_f64).filter(|v| v.is_finite()) - { - return Ok(json!({"count": count, "spacing": pt})); +/// The Columns dialog's changed fields as `format.columns` params: Number and Spacing (inches, +/// sent in points). A field left as the dialog showed it is left out, so each selected text box +/// keeps its own value; `None` when neither field changed. +fn columns_value(d: &Dialog, shown_count: &str, shown_spacing: &str) -> Result, String> { + let mut p = serde_json::Map::new(); + let count = field_of(d, "count"); + if count != shown_count { + let n = count.trim().parse::().ok().filter(|n| (1..=16).contains(n)).ok_or("Number must be a whole number from 1 to 16")?; + p.insert("count".into(), json!(n)); } - let spacing = typed - .trim() - .trim_end_matches("in") - .trim_end_matches('"') - .trim() - .parse::() - .ok() - .filter(|v| v.is_finite() && (0.0..=22.0).contains(v)) - .ok_or("Spacing must be from 0 to 22 inches")?; - Ok(json!({"count": count, "spacing": spacing * 72.0})) + let spacing = field_of(d, "spacing"); + if spacing != shown_spacing { + let inches = spacing + .trim() + .trim_end_matches("in") + .trim_end_matches('"') + .trim() + .parse::() + .ok() + .filter(|v| v.is_finite() && (0.0..=22.0).contains(v)) + .ok_or("Spacing must be from 0 to 22 inches")?; + p.insert("spacing".into(), json!(inches * 72.0)); + } + Ok((!p.is_empty()).then_some(Value::Object(p))) } /// Columns: the number of columns in the text box and the space between them. OK applies one -/// `format.columns` call; Cancel leaves the text box as it was. +/// `format.columns` call with the fields that changed, or none when neither did; Cancel leaves +/// the text box as it was. fn columns_dialog(app: &mut SlideApp, ui: &mut Ui, d: &mut Dialog) -> bool { let count = d.params.get("count").and_then(Value::as_u64).unwrap_or(1).to_string(); let spacing = num_text(d.params.get("spacing").and_then(Value::as_f64).unwrap_or(0.0) / 72.0); @@ -2700,8 +2704,12 @@ fn columns_dialog(app: &mut SlideApp, ui: &mut Ui, d: &mut Dialog) -> bool { } let (ok, cancel) = buttons(ui, "OK"); if ok { - match columns_value(d, &spacing).and_then(|p| app.run("format.columns", p)) { - Ok(_) => return true, + let applied = columns_value(d, &count, &spacing).and_then(|p| match p { + Some(p) => app.run("format.columns", p).map(|_| ()), + None => Ok(()), + }); + match applied { + Ok(()) => return true, Err(e) => { d.fields.insert("error".into(), e); return false;