fix(validate): fix overflowing text, and stop sampling frames that never render - #172
Merged
Conversation
…ver render 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two follow-ups that #170 and #171 made possible but deliberately left out of their own scope, since both change what
validatedoes to a user's file.--fixnow answersContentOverflowsBoxon textThat arm did nothing, for a good reason spelled out in its own comment: growing the box, shrinking the font and shortening the copy are all legitimate and produce very different results, so picking one was not the validator's call.
style.text-autofit(#170) removed the dilemma. It states the author's intent — "this must fit" — without touching the declared box or the content, so nothing written by hand is overwritten or lost. That puts it in the same risk category as the two fixes already accepted (white-spaceremoved,auto_scrollforced), both of which also change the render.Scoped to
textandgradient_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 — worse than leaving the violation visible. A test pins that atableis left alone and that nothing is counted as applied.--strict-animstops sampling atfreeze_atSince #164 every render path clamps at
scene.freeze_at, so any sample past it evaluates transforms at an instant the video cannot contain.--strict-animwas reporting violations that cannot happen — a false positive that blocks a correct scenario and sends a generator "fixing" something that was never wrong.Bounding the sample list rather than clamping each timestamp afterwards also avoids generating a run of identical post-freeze samples, so a frozen scene costs less to validate rather than more.
scene_durationstill reachesBuildAnimationCtxunchanged: 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 rather than removing an unreachable sample. Both are in.
Verification
cargo test --workspace: 25 targets, 1037 tests, 0 failurescargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warnings: cleanClippy earned its keep here: it caught a duplicated
#[test]attribute I had left above an existing one, which had been silently running the new test twice.