From a090fb3f24661b31ba566094a595217b94a48ae3 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 11 Aug 2026 11:43:42 +0200 Subject: [PATCH] fix(validate): fix overflowing text, and stop sampling frames that never render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups the previous changes made possible but deliberately left out of their own scope, since both alter what `validate` does to a user's file. **`--fix` answers `ContentOverflowsBox` on text.** That arm did nothing because growing the box, shrinking the font and shortening the copy are all legitimate and produce different results — picking one was not the validator's call. `style.text-autofit` (#170) removed the dilemma: it states the author's intent without touching the declared box or the content, so nothing written by hand is overwritten. Same risk category as the two fixes already accepted, both of which also change the render. Scoped to `text`/`gradient_text`, the two components whose painters implement the field. Writing it anywhere else would be a no-op an author could reasonably read as a fix, which is worse than leaving the violation visible — a test pins that a `table` is left alone and nothing is claimed as applied. **`--strict-anim` stops at `freeze_at`.** Since #164 every render path clamps there, so samples beyond it evaluate transforms at instants the video cannot contain. It was reporting violations that cannot happen, which blocks a correct scenario and sends a generator "fixing" what was never wrong. Bounding the sample list rather than clamping each timestamp afterwards also avoids generating a run of identical post-freeze samples. `scene_duration` still reaches `BuildAnimationCtx` unchanged, so duration-relative effects keep their real window (contract from PR #27) — only the sampling ceiling moves. The mirror test matters as much as the new one: the same fixture without a freeze must still be caught, or the bound would be silencing real overflow instead of removing an unreachable sample. --- .../rustmotion-cli/src/commands/geometry.rs | 60 +++++++++++- .../rustmotion-cli/src/commands/validate.rs | 97 +++++++++++++++++-- 2 files changed, 149 insertions(+), 8 deletions(-) diff --git a/crates/rustmotion-cli/src/commands/geometry.rs b/crates/rustmotion-cli/src/commands/geometry.rs index 293058c..ac78b6e 100644 --- a/crates/rustmotion-cli/src/commands/geometry.rs +++ b/crates/rustmotion-cli/src/commands/geometry.rs @@ -1310,8 +1310,22 @@ pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec { + // Growing the box, shrinking the font and shortening the copy + // are all legitimate answers with very different visual + // outcomes, so this arm used to do nothing rather than pick + // one. `style.text-autofit` removed that dilemma for the two + // components whose painters implement it: it declares the + // author's intent ("this must fit") without touching the + // declared box or the content, so nothing the author wrote is + // overwritten or lost — the same risk category as the two + // fixes above, both of which also change the render. + // + // Scoped to `text`/`gradient_text` deliberately. Every other + // component ignores the field, so writing it there would be a + // no-op the author could reasonably read as a fix, which is + // worse than leaving the violation to them. + let kind = target.get("type").and_then(|t| t.as_str()); + if matches!(kind, Some("text") | Some("gradient_text")) { + if let Some(style) = target + .as_object_mut() + .and_then(|o| o.get_mut("style")) + .and_then(|s| s.as_object_mut()) + { + if !style.contains_key("text-autofit") { + style.insert("text-autofit".into(), serde_json::Value::Bool(true)); + applied += 1; + } + } + } + } ViolationKind::ViewportOverflow | ViolationKind::AnimatedTextOverflow - | ViolationKind::ContentOverflowsBox | ViolationKind::ContentOverflowsCard => { // Position/size clamping is too risky to auto-fix without - // losing intent — leave it for the user. (ContentOverflowsBox - // /ContentOverflowsCard specifically: growing the box/card, - // shrinking the font, or shortening the copy are all - // legitimate fixes with very different visual outcomes — not - // ours to pick.) + // losing intent — leave it for the user. } } } @@ -327,6 +351,67 @@ mod tests { } } + fn overflow_box_violation(path: &str) -> GeometryViolation { + GeometryViolation { + kind: ViolationKind::ContentOverflowsBox, + ..unwrappable_violation(path) + } + } + + /// `ContentOverflowsBox` had no fix because growing the box, shrinking + /// the font and shortening the copy are all legitimate and pick + /// different outcomes. `text-autofit` states the intent instead, without + /// overwriting anything the author declared. + #[test] + fn fix_declares_text_autofit_on_an_overflowing_text() { + let mut json: serde_json::Value = serde_json::from_str(NARROW_CARD_JSON).unwrap(); + let path = "views[0].scenes[0].children[0].children[0]"; + let applied = apply_fixes(&mut json, &[overflow_box_violation(path)]); + assert_eq!(applied, 1, "expected exactly one fix applied"); + + let target = navigate(&mut json, path).expect("path resolves"); + assert_eq!( + target.get("style").and_then(|s| s.get("text-autofit")), + Some(&serde_json::Value::Bool(true)) + ); + // The declared box and the content are what the author wrote; a fix + // that rewrote either would be picking one of the outcomes this arm + // exists to avoid picking. + assert!( + target.get("content").is_some(), + "content must be untouched: {target}" + ); + } + + /// Every component other than `text`/`gradient_text` ignores the field. + /// Writing it there would look like a fix while changing nothing, which + /// is worse than leaving the violation visible. + #[test] + fn fix_leaves_overflowing_components_that_cannot_autofit_alone() { + let json_src = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ "duration": 1.0, "children": [ + { "type": "table", "headers": ["a"], "rows": [["b"]], + "style": { "width": "40px", "font-size": 40 } } + ]}] + }"##; + let mut json: serde_json::Value = serde_json::from_str(json_src).unwrap(); + let path = "views[0].scenes[0].children[0]"; + assert_eq!( + apply_fixes(&mut json, &[overflow_box_violation(path)]), + 0, + "a table cannot autofit, so nothing should be claimed as fixed" + ); + let target = navigate(&mut json, path).expect("path resolves"); + assert!( + target + .get("style") + .and_then(|s| s.get("text-autofit")) + .is_none(), + "must not write a field this painter ignores: {target}" + ); + } + /// C1: `apply_fixes` must never write `style.wrap` (not a `CssStyle` /// field — writing it drops the whole component at the next parse /// because `CssStyle` is `deny_unknown_fields`). It must instead remove