mirror of
https://github.com/storytold/wordcraft.git
synced 2026-10-21 11:14:04 -05:00
#2573: size fields never resize a picture just by showing it
Review follow-up (CodeRabbit on #2889): egui's DragValue clamps a value already outside its range when it is drawn and reports that as a change. The Size fields act on changes, so: - Picture Format's W/H resized a picture smaller than 1 pt (a 1×1 px spacer is 0.75 pt) or larger than 4000 pt as soon as the tab was shown, an edit and an undo step nobody asked for. The code before this PR had the same problem with its 4–2000 pt range. - The Size dialog's scale fields silently changed a picture enlarged past 10000% (a small icon made large), and its lengths a size outside 1–4000 pt, which OK then applied. The range now bounds only what is typed or dragged (`clamp_existing_to_range(false)`). Both regression tests fail without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZArmeAT48gF84aKj7Zx4C
This commit is contained in:
1 parent
9e257070b9
commit
d56a105f8b
2 files changed
+65
-4
No files matched your search
@@ -1244,19 +1244,22 @@ fn size_ui(app: &mut WordApp, ui: &mut Ui, f: &mut SizeForm) -> bool {
|
||||
let k = unit.pt_per_unit();
|
||||
// A length in the interface unit that takes any typed unit (`300 px` too), applied on Enter
|
||||
// so a half-typed number never resizes the other side; `None` until its value changes (while
|
||||
// typing, the field reports each keystroke as a change).
|
||||
// typing, the field reports each keystroke as a change). The range bounds what is typed or
|
||||
// dragged only: a value already outside it (a 1 px picture is 0.75 pt) is shown as it is, not
|
||||
// clamped, which egui would report as a change and so apply unasked.
|
||||
let length = |ui: &mut Ui, label: &str, pt: f32| {
|
||||
let name = ui.label(tl!(label));
|
||||
let was = pt / k;
|
||||
let mut v = was;
|
||||
let field = egui::DragValue::new(&mut v).speed(1.0 / k).range(1.0 / k..=4000.0 / k).measure(unit).max_decimals(2);
|
||||
let field =
|
||||
egui::DragValue::new(&mut v).speed(1.0 / k).range(1.0 / k..=4000.0 / k).clamp_existing_to_range(false).measure(unit).max_decimals(2);
|
||||
let changed = ui.add(field.update_while_editing(false)).labelled_by(name.id).changed();
|
||||
(changed && v != was).then_some(v * k)
|
||||
};
|
||||
let percent = |ui: &mut Ui, label: &str, was: f32| {
|
||||
let name = ui.label(tl!(label));
|
||||
let mut v = was;
|
||||
let field = egui::DragValue::new(&mut v).speed(1.0).range(1.0..=10_000.0).suffix(" %").max_decimals(1);
|
||||
let field = egui::DragValue::new(&mut v).speed(1.0).range(1.0..=10_000.0).clamp_existing_to_range(false).suffix(" %").max_decimals(1);
|
||||
let changed = ui.add(field.update_while_editing(false)).labelled_by(name.id).changed();
|
||||
(changed && v != was).then_some(v)
|
||||
};
|
||||
@@ -1903,4 +1906,28 @@ mod tests {
|
||||
h.state_mut().run("edit.undo", json!({})).unwrap();
|
||||
assert_eq!(selected_size(h.state()), (30.0, 15.0), "one undo step");
|
||||
}
|
||||
|
||||
/// #2573 review: a value outside a field's range (a 1 px picture enlarged past 10000%) is shown
|
||||
/// as it is. egui would otherwise clamp it on the first frame and report a change, which the
|
||||
/// form would take as an edit and OK would apply unasked.
|
||||
#[test]
|
||||
fn size_dialog_leaves_out_of_range_values_alone() {
|
||||
let mut a = picture_app(1, 1);
|
||||
a.run("picture.size", json!({"width": 100.0, "height": 100.0, "lockAspect": false})).unwrap();
|
||||
a.run("picture.size", json!({})).unwrap();
|
||||
let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1440.0, 900.0)).build_ui_state(
|
||||
|ui, app: &mut WordApp| {
|
||||
let ctx = ui.ctx().clone();
|
||||
app.logic(&ctx);
|
||||
app.ui(ui);
|
||||
},
|
||||
a,
|
||||
);
|
||||
h.run_steps(4);
|
||||
let Some(Dialog::Layout(d)) = &h.state().dialog else { panic!("the Size dialog is open") };
|
||||
let LayoutDialog::Size(f) = d.as_ref() else { panic!("size") };
|
||||
assert!((f.scale().0 - 100.0 / 0.75 * 100.0).abs() < 1.0, "13333%: {:?}", f.scale());
|
||||
assert_eq!((f.width, f.height), (100.0, 100.0), "the form keeps the size");
|
||||
assert_eq!(selected_size(h.state()), (100.0, 100.0));
|
||||
}
|
||||
}
|
||||
@@ -2568,7 +2568,14 @@ fn picture_format(app: &mut WordApp, ui: &mut Ui) {
|
||||
let name = ui.label(egui::RichText::new(tl!(label)).small());
|
||||
let was = pt / k;
|
||||
let mut v = was;
|
||||
let field = egui::DragValue::new(&mut v).speed(1.0 / k).range(1.0 / k..=4000.0 / k).measure(unit).max_decimals(2);
|
||||
// Not `clamp_existing_to_range`: egui would clamp a size outside the range (a 1 px
|
||||
// picture is 0.75 pt) and report it as a change, resizing the picture unasked.
|
||||
let field = egui::DragValue::new(&mut v)
|
||||
.speed(1.0 / k)
|
||||
.range(1.0 / k..=4000.0 / k)
|
||||
.clamp_existing_to_range(false)
|
||||
.measure(unit)
|
||||
.max_decimals(2);
|
||||
let r = ui.add(field.update_while_editing(false)).labelled_by(name.id).on_hover_text(tl!(
|
||||
"Type a size in any unit, such as 5 cm or 300 px. The other side follows, so the picture keeps its proportions."
|
||||
));
|
||||
@@ -3311,4 +3318,31 @@ mod tests {
|
||||
h.state_mut().run("edit.undo", json!({})).unwrap();
|
||||
assert_eq!(size(h.state()), Some((15.0, 7.5)), "one undo step");
|
||||
}
|
||||
|
||||
/// #2573 review: showing Picture Format never resizes a picture whose size is outside the
|
||||
/// fields' range (a 1×1 px spacer is 0.75 pt): egui clamps such a value by default and reports
|
||||
/// it as a change.
|
||||
#[test]
|
||||
fn picture_format_leaves_a_tiny_picture_alone() {
|
||||
let mut a = crate::WordApp::new(Session::new(wordcraft_doc::Document::new()), crate::Services::default());
|
||||
let img = image::RgbaImage::from_pixel(1, 1, image::Rgba([0, 0, 0, 255]));
|
||||
let mut png = Vec::new();
|
||||
img.write_to(&mut std::io::Cursor::new(&mut png), image::ImageFormat::Png).unwrap();
|
||||
a.run("insert.picture", json!({"data": wordcraft_engine::cmd::insert::base64_encode(&png)})).unwrap();
|
||||
a.run("ui.tab", json!({"tab": "Picture Format"})).unwrap();
|
||||
let size = |a: &crate::WordApp| wordcraft_engine::cmd::objects::selected(&a.session).and_then(|(_, o)| o.frame().map(|(w, h, _)| (w, h)));
|
||||
assert_eq!(size(&a), Some((0.75, 0.75)));
|
||||
let undo = a.session.undo_depth();
|
||||
let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(2560.0, 900.0)).build_ui_state(
|
||||
|ui, app: &mut crate::WordApp| {
|
||||
let ctx = ui.ctx().clone();
|
||||
app.logic(&ctx);
|
||||
app.ui(ui);
|
||||
},
|
||||
a,
|
||||
);
|
||||
h.run_steps(6);
|
||||
assert_eq!(size(h.state()), Some((0.75, 0.75)), "shown, not resized");
|
||||
assert_eq!(h.state().session.undo_depth(), undo, "no undo step");
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user