Skip to content

feat: make every timeline kind creatable from the CLI - #575

Open
FelipeDefensor wants to merge 4 commits into
devfrom
fix/476-cli-all-timeline-kinds
Open

feat: make every timeline kind creatable from the CLI#575
FelipeDefensor wants to merge 4 commits into
devfrom
fix/476-cli-all-timeline-kinds

Conversation

@FelipeDefensor

Copy link
Copy Markdown
Collaborator

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 harmony and audiowave against a base that has since moved on ~209 commits (the TimelineKind enum it was written against no longer exists). This is a fresh implementation on dev.

Closes #476.

What changed

audiowave was broken before it was exposed. Timelines.create_timeline calls AudioWaveTimeline.refresh() for new audiowave timelines, and refresh() caps its amplitude divisions at Get.PLAYBACK_AREA_WIDTH — a request only QtUI ever served. Adding one from the CLI therefore raised NoReplyToRequest, which CLI.run catches 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 QtUI starts at, so an audiowave timeline built headless has the resolution it would have had in the GUI.

New kinds. harmony/har, audiowave/aud, and pdf. slider stays 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 letting PdfTimeline fail on a required positional.

--height is now checked rather than dropped. Harmony computes its height from level_height and visible_level_count, and passing one raises TypeError inside HarmonyTimeline.__init__ — so the flag is rejected with a message. Audiowave is an ordinary Timeline in this respect, so --height applies to it normally.

One kind table instead of three. choices and KIND_STR_TO_TLKIND were the same key set written twice, with KIND_SPECIFIC_ARGS a third hand-written arg-to-kind mapping alongside them. choices is 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 suite
  • Real CLI, not just tests: load-media example.mp3 then timelines add audiowave/harmony/pdf all create cleanly, no traceback
  • The audiowave tests fail without the Get.PLAYBACK_AREA_WIDTH fix (assert 0 > 0, then assert_no_error) — they're real regression guards, not name-and-type assertions that a hidden empty timeline would also satisfy
  • The coverage test fails when a kind is removed from the table
  • pre-commit run on all changed files

Not 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_timeline rather than something this change introduces. Happy to open it separately if it's worth a guard.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant