Merge pull request #4213 from DCaioP/fix/decimal-comma-in-unit-fields

Fix #3837: unit fields read a decimal comma as a decimal point

contributors/people.toml

@@ -159,3 +159,8 @@ display_name = "Aditya Sisodiya"
 real_name = "Angelo Bernardini"
 display_name = "Angelo Bernardini"
 sync_github_name = false
+
+[people.DCaioP]
+real_name = "Caio Pontes Magalhães"
+display_name = "Caio Pontes"
+sync_github_name = false

crates/ui-egui/src/dialogs/tests_modal.rs

@@ -703,3 +703,26 @@ fn printer_mark_labels_are_distinct_from_language_names() {
     assert_eq!(save_pdf::choice_label(cs, "language", "Japanese"), crate::i18n::tr(cs, "Japanese"));
     assert_eq!(save_pdf::choice_label(crate::i18n::Lang::EN, "marks.kind", "Japanese"), "Japanese");
 }
+
+/// In a document in millimeters, an offset typed with a decimal comma (`0,1`) is taken as 0.1 mm
+/// instead of going back to the value the field had, and Offset Path opens on it again (#3837).
+#[test]
+fn offset_path_takes_a_decimal_comma() {
+    let (ctx, mut app) = (ctx(), app_with_selection());
+    app.run("document.setUnits", json!({"units": "Millimeters"})).unwrap();
+    app.run("select.all", json!({})).unwrap();
+    crate::menus::invoke(&mut app, "object.path.offsetPath", json!({}));
+    settle(&ctx, &mut app);
+    // The dialog focuses its first field, the offset: type as a user does, then Tab commits it.
+    frame(&ctx, &mut app, vec![Event::Text("0,1".into())]);
+    frame(&ctx, &mut app, vec![Event::Key { key: egui::Key::Tab, physical_key: None, pressed: true, repeat: false, modifiers: Default::default() }]);
+    let texts = frame(&ctx, &mut app, vec![]);
+    let mm = vectorcraft_doc::Unit::Millimeters.to_pt(0.1);
+    let offset = app.ui.dialog.as_ref().unwrap().f64("offset", 0.0);
+    assert!((offset - mm).abs() < 1e-9, "0.1 mm: {offset} pt");
+    assert!(texts.iter().any(|(t, _)| t == "0.1 mm"), "the field shows 0.1 mm: {texts:?}");
+    confirm(&mut app).unwrap();
+    crate::menus::invoke(&mut app, "object.path.offsetPath", json!({}));
+    let offset = app.ui.dialog.as_ref().unwrap().f64("offset", 0.0);
+    assert!((offset - mm).abs() < 1e-9, "reopened on 0.1 mm: {offset} pt");
+}

crates/ui-egui/src/widgets.rs

@@ -343,6 +343,28 @@ pub(crate) fn typed_unit(ctx: &egui::Context, unit: Unit) -> Unit {
     if bare_points && unit == Unit::Picas { Unit::Points } else { unit }
 }
 
+/// `text` typed in a numeric field, with a decimal comma read as a point: `0,1 mm` is `0.1 mm`
+/// (#3837). Text with a point keeps its commas, so `1,000.5` is still refused rather than read
+/// with two decimal separators, and so does a number that reads as thousands ([`thousands`]).
+fn decimal_point(text: &str) -> std::borrow::Cow<'_, str> {
+    if text.contains(',') && !text.contains('.') && !thousands(text) { text.replace(',', ".").into() } else { text.into() }
+}
+
+/// Is `text` a number written with a thousands comma, as `1,000` or `12,500 mm`: one to three
+/// digits (not a lone 0), a comma, exactly three digits, then at most a unit? Read as a decimal it
+/// would be a thousand times too small, so the field refuses it instead (`0,125` and `1,25` are
+/// decimals).
+fn thousands(text: &str) -> bool {
+    let number = text.trim().trim_start_matches('-');
+    let Some((whole, rest)) = number.split_once(',') else { return false };
+    let (part, unit) = rest.split_at(rest.find(|c: char| !c.is_ascii_digit()).unwrap_or(rest.len()));
+    (1..=3).contains(&whole.len())
+        && whole.bytes().all(|b| b.is_ascii_digit())
+        && !whole.starts_with('0')
+        && part.len() == 3
+        && unit.trim().chars().all(char::is_alphabetic)
+}
+
 /// A recessed numeric field showing `value` (points) in `unit`. Returns the new value (points)
 /// when the user commits (Enter / focus loss / Shift+Enter, which keeps the focus outside dialogs).
 /// Supports unit suffixes and arithmetic; a number without a unit is in `unit` ([`typed_unit`]: in
@@ -387,14 +409,15 @@ fn unit_field(
     let (mut buf, commit, resp, rect) = recessed_text(ui, id, &shown, width, 1, label, name);
     select_all_on_focus(ui, &resp, &buf);
     // ↑/↓ and the wheel step in the field's unit; a drag on its label scrubs it.
-    let read = typed_unit(ui.ctx(), unit);
+    let typed = typed_unit(ui.ctx(), unit);
+    let read = |b: &str| typed.parse(&decimal_point(b));
     let bounds = unit.from_pt(*bounds.start())..=unit.from_pt(*bounds.end());
-    let stepped = step_value(ui, &resp, &mut buf, &bounds, (|b: &str| Some(unit.from_pt(read.parse(b)?)), |v| unit.format(unit.to_pt(v))), None);
+    let stepped = step_value(ui, &resp, &mut buf, &bounds, (|b: &str| Some(unit.from_pt(read(b)?)), |v| unit.format(unit.to_pt(v))), None);
     let scrubbed = scrub::field(ui, id, rect, &buf, value.map(|v| unit.from_pt(v)), STEP_DECIMALS, &bounds);
     let commit = commit && buf != shown;
     ui.data_mut(|d| d.insert_temp(id, buf.clone()));
     if commit {
-        read.parse(&buf).or_else(|| if share_of_value { share_of(&buf, value?) } else { None })
+        read(&buf).or_else(|| if share_of_value { share_of(&decimal_point(&buf), value?) } else { None })
     } else {
         stepped.or(scrubbed).map(|v| unit.to_pt(v))
     }
@@ -933,6 +956,7 @@ fn bounded_field(
     // The suffix (and %, °) may follow any operand: `45*2°`, `50% / 2`.
     let read = |buf: &str| {
         let suffix = suffix.trim();
+        let buf = decimal_point(buf);
         let bare = if suffix.is_empty() { buf.to_string() } else { buf.replace(suffix, "") };
         vectorcraft_doc::parse_number(&bare.replace(['%', '°'], ""))
     };
@@ -2589,7 +2613,14 @@ pub fn opt_field_named(ui: &mut Ui, id: impl std::hash::Hash, name: Option<&str>
     select_all_on_focus(ui, &resp, &buf);
     // ↑/↓ and the wheel step a value (an empty field stays empty); a drag on its label scrubs it.
     let lengths = 0.0..=f64::INFINITY;
-    let stepped = step_value(ui, &resp, &mut buf, &lengths, (|b: &str| Some(unit.from_pt(unit.parse(b)?)), |v| unit.number(unit.to_pt(v))), None);
+    let stepped = step_value(
+        ui,
+        &resp,
+        &mut buf,
+        &lengths,
+        (|b: &str| Some(unit.from_pt(unit.parse(&decimal_point(b))?)), |v| unit.number(unit.to_pt(v))),
+        None,
+    );
     let scrubbed = scrub::field(ui, id, framed.response.rect, &buf, value.map(|v| unit.from_pt(v)), STEP_DECIMALS, &lengths);
     let focused = ui.memory(|m| m.has_focus(id));
     ui.data_mut(|d| {
@@ -2598,7 +2629,7 @@ pub fn opt_field_named(ui: &mut Ui, id: impl std::hash::Hash, name: Option<&str>
     });
     if (resp.lost_focus() || kept) && buf != shown {
         let s = buf.trim();
-        if s.is_empty() { Some(None) } else { typed_unit(ui.ctx(), unit).parse(s).map(Some) }
+        if s.is_empty() { Some(None) } else { typed_unit(ui.ctx(), unit).parse(&decimal_point(s)).map(Some) }
     } else {
         stepped.or(scrubbed).map(|v| Some(unit.to_pt(v)))
     }
@@ -3224,6 +3255,33 @@ mod step_to_multiple_tests {
     }
 }
 
+#[cfg(test)]
+mod decimal_point_tests {
+    use super::*;
+
+    /// A decimal comma reads as a point, in a length, with arithmetic and in a percentage; text
+    /// with a point keeps its commas and is refused as before, and so does a thousands comma
+    /// (#3837).
+    #[test]
+    fn a_decimal_comma_reads_as_a_point() {
+        let mm = |s: &str| Unit::Millimeters.parse(&decimal_point(s)).map(|pt| Unit::Millimeters.from_pt(pt));
+        for (text, want) in [("0,1", 0.1), ("0,1 mm", 0.1), ("0,1mm", 0.1), ("-0,5", -0.5), ("1,5+0,25", 1.75), ("0.1", 0.1)] {
+            let got = mm(text).unwrap_or(f64::NAN);
+            assert!((got - want).abs() < 1e-9, "{text}: {got}");
+        }
+        assert_eq!(mm("1,000.5"), None, "a point and a comma");
+        assert_eq!(mm("1,2,3"), None, "two commas");
+        for thousand in ["1,000", "12,500 mm", "-1,000", "999,999mm"] {
+            assert_eq!(mm(thousand), None, "{thousand} reads as thousands: refused, not a thousand times too small");
+        }
+        for (text, want) in [("0,125", 0.125), ("1,25", 1.25), ("1,0000", 1.0), ("1,500+2", 3.5)] {
+            let got = mm(text).unwrap_or(f64::NAN);
+            assert!((got - want).abs() < 1e-9, "{text} is a decimal: {got}");
+        }
+        assert_eq!(share_of(&decimal_point("12,5%"), 200.0), Some(25.0));
+    }
+}
+
 #[cfg(test)]
 mod menu_room_tests {
     use super::*;