feat: make every timeline kind creatable from the CLI - #575
Open
FelipeDefensor wants to merge 4 commits into
Open
feat: make every timeline kind creatable from the CLI#575FelipeDefensor wants to merge 4 commits into
FelipeDefensor wants to merge 4 commits into
Conversation
AudioWaveTimeline.refresh() caps its amplitude divisions at the playback area width, but only QtUI ever served that request. Any headless caller of refresh() therefore died on NoReplyToRequest, which the CLI catches and reports as a raw traceback under "CLI error", leaving a timeline with no components behind. Serve the same default QtUI starts at, so an audiowave timeline built without a GUI has the resolution it would have had with one.
`choices` and `KIND_STR_TO_TLKIND` were the same key set written twice, and `KIND_SPECIFIC_ARGS` was a third hand-written arg-to-kind mapping alongside them. Keeping three tables in sync by hand is what let #476 happen: a kind reachable everywhere else in the app was simply missing from one of the lists, with nothing to catch it. Derive `choices` from the kind table, and validate optional arguments against the per-kind kwargs table instead of a separate mapping, so a kind that gains or loses a kwarg gets the right validation for free. This also lifts the kind table out of `add()`, where it was rebuilt on every invocation. Behaviour is unchanged; the existing tests cover it.
Closes #476. Every kind the app can create is now reachable from the CLI, by full name or abbreviation. Slider stays out on purpose: it comes with the file and can't be deleted. Harmony accepts no --height. It computes its own from level_height and visible_level_count, and passing one raises TypeError inside HarmonyTimeline.__init__, so the flag is rejected rather than dropped. Audiowave, by contrast, is an ordinary Timeline as far as height goes, so --height applies to it normally. PDF needs a file to point at, which the GUI collects through a dialog that has no CLI equivalent. Take it as --path instead, and reject the kind up front when it's missing rather than letting PdfTimeline fail on a required positional. The audiowave tests load media before adding the timeline: without it, refresh() bails out on the unreadable file and produces a hidden, empty timeline that still satisfies a name-and-type assertion.
#476 was not a logic error but a bookkeeping one: a kind reachable everywhere else in the app was missing from the CLI's tables, and nothing noticed. Compare those tables against Timeline.subclasses() so the next kind added to the app fails here until it is wired up, instead of quietly going missing again.
4 tasks
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.
Summary
Makes every timeline kind the app can create reachable from
timelines add, and fixes the reason the audiowave kind couldn't have worked there.Supersedes #483, which added
harmonyandaudiowaveagainst a base that has since moved on ~209 commits (theTimelineKindenum it was written against no longer exists). This is a fresh implementation ondev.Closes #476.
What changed
audiowavewas broken before it was exposed.Timelines.create_timelinecallsAudioWaveTimeline.refresh()for new audiowave timelines, andrefresh()caps its amplitude divisions atGet.PLAYBACK_AREA_WIDTH— a request onlyQtUIever served. Adding one from the CLI therefore raisedNoReplyToRequest, whichCLI.runcatches and reports as a raw Python traceback under "CLI error", leaving a timeline with no components behind. With no media loaded it instead failed earlier and produced a hidden, empty timeline. There was no input for which the command worked.The CLI now serves that request with the same default
QtUIstarts at, so an audiowave timeline built headless has the resolution it would have had in the GUI.New kinds.
harmony/har,audiowave/aud, andpdf.sliderstays out on purpose — it comes with the file and can't be deleted. With those, all eight creatable kinds are available.PDF takes
--path. The GUI collects the file through a dialog with no CLI equivalent, so the path comes from a flag instead, and the kind is rejected up front when it's missing rather than lettingPdfTimelinefail on a required positional.--heightis now checked rather than dropped. Harmony computes its height fromlevel_heightandvisible_level_count, and passing one raisesTypeErrorinsideHarmonyTimeline.__init__— so the flag is rejected with a message. Audiowave is an ordinaryTimelinein this respect, so--heightapplies to it normally.One kind table instead of three.
choicesandKIND_STR_TO_TLKINDwere the same key set written twice, withKIND_SPECIFIC_ARGSa third hand-written arg-to-kind mapping alongside them.choicesis now derived from the kind table, and optional arguments are validated against the per-kind kwargs table, so a kind that gains or loses a kwarg gets the right validation for free. That commit is behaviour-preserving and the pre-existing tests cover it.On #476 specifically
The issue wasn't a logic error but a bookkeeping one: a kind reachable everywhere else in the app was simply missing from one list, and nothing noticed. Adding the missing kinds fixes today's symptom; the last commit compares the CLI's tables against
Timeline.subclasses()so the next kind added to the app fails there until it's wired up.Test plan
pytest— full suiteload-media example.mp3thentimelines add audiowave/harmony/pdfall create cleanly, no tracebackGet.PLAYBACK_AREA_WIDTHfix (assert 0 > 0, thenassert_no_error) — they're real regression guards, not name-and-type assertions that a hidden empty timeline would also satisfypre-commit runon all changed filesNot in scope
Nothing stops a user creating N audiowave timelines, each re-decoding the same media. That's identical in the GUI, so it's a pre-existing question about
create_timelinerather than something this change introduces. Happy to open it separately if it's worth a guard.