Beat timeline with offset

[minamouse]

Summary

An option to add an offset to the beat timeline start time. It's a check box with a text box that is only enabled when selected, and it is paired with one of the existing beat fill options.

Because this is modifying some of the same code as the BPM PR, I could combine them if that makes more sense, but for now keeping them separate.

Window

Example after adding

[FelipeDefensor]

Thanks for this. A very useful addition and the implementation works. It does create some easy-to-fix problems with some of the fill methods though.

Two things need fixing before this can merge:

1. CI is currently red. Five existing tests fail on this branch (they pass on main) — see the inline notes on the signature and the tuple unpack.
2. The offset shifts the beats but not the extent they're spread over, so the trailing beats land past the end of the media and get silently dropped. On a 100 s file, "100 beats" with a 10 s offset produces 91 beats and no error.

The rest are smaller consistency points. Happy to help with any of them. Note that our PRs are rebased and them fast-forwarded against dev, so you might encounter some merge conflicts.

Disclaimer: the review was very much AI-assisted, hence the size of the comments.

[FelipeDefensor]

offset has no default, so every existing caller breaks. tests/ui/timelines/score/test_musicxml.py:39 still calls fill_with_beats(beat_tl.FillMethod.BY_AMOUNT, 10) and now fails with:

TypeError: BeatTimeline.fill_with_beats() missing 1 required positional argument: 'offset'

offset: int | float = 0 is exactly the old behaviour, so it keeps every caller working and makes the feature purely additive.

Optional: add a -> None as return value hint too.

[FelipeDefensor]

Both branches still size themselves from the full duration while the times are shifted by offset, so the run overruns the end of the media:

• BY_AMOUNT spaces at duration / value, so the last beat sits at offset + duration - duration/value
• BY_INTERVAL still iterates floor(duration / value) times

Anything past duration is rejected by validate_time_is_inbounds, and since the create_component return is discarded the user just gets fewer beats than they asked for. Measured on this branch: duration 100s, 100 beats, offset 10 -> 91 beats, no error. Same for interval 1s + offset 10.

Deriving both branches from the region actually being filled fixes it and removes the duplication:

span = duration - offset
if span <= 0:
    return

if method == BeatTimeline.FillMethod.BY_AMOUNT:
    count, step = value, span / value
else:
    count, step = math.floor(span / value), value

reasons = []
for i in range(count):
    component, fail_reason = self.create_component(
        ComponentKind.BEAT, offset + i * step
    )
    if not component:
        reasons.append(fail_reason)

With offset=0 this is identical to the current behaviour, so the existing tests stay green.

Heads up: dev has since rewritten this function with a @long_operation("Creating beats...") decorator and per-iteration progress posts, so this will need reshaping onto that structure if the PR gets rebased.

[FelipeDefensor]

Widening the result to a 4-tuple means the four Serve stubs in tests/ui/timelines/beat/test_beat_timeline_ui.py (lines 522, 534, 545, 555) now raise ValueError: not enough values to unpack (expected 4, got 3). All four TestFillWithBeats tests fail — including test_reject_delete_existing_beats, since the unpack happens before the confirmation dialog.

Each stub needs a fourth element, e.g. (..., BeatTimeline.FillMethod.BY_AMOUNT, 100, 0).

[FelipeDefensor]

setRange(0, 1000) is the one input here not tied to the media — the line four above does it right:

self._by_interval_edit.setRange(0.01, get(Get.MEDIA_DURATION))

The hardcoded bound is wrong in both directions: a 75-minute recording can't be offset past 1000s, and a 30s clip accepts an offset of 900. The second case is the bad one — on_beat_timeline_fill calls timeline.clear() before filling, so every beat gets rejected and the user's existing beats are gone with nothing to replace them and no error shown. (Undo recovers it, but nothing signals that anything went wrong.)

setRange(0, get(Get.MEDIA_DURATION)) closes this off at the widget.

[FelipeDefensor]

The box starts disabled but pre-filled with 10, so ticking the checkbox and pressing OK applies a 10-second offset the user never typed.

setValue(0) is the safe default — and once it's 0, the checkbox has nothing left to do: a spinbox reading 0 already means "no offset", which is exactly what the if self._with_offset_prompt.isChecked() else 0 ternary substitutes. Dropping the checkbox, the toggled.connect, and the enabled/disabled bookkeeping would leave get_result reading self._with_offset_value.value() and one fewer widget to keep in sync. Your call, but it's a nice simplification.

[FelipeDefensor]

This is the only user-facing string in the dialog that doesn't come from tilia/ui/strings.py. The idea is to centralize strings there to allow for internatinalization soon.

A BEAT_TIMELINE_WITH_OFFSET_OPTION constant plus setText(...) (like lines 67 and 71) matches the rest. Small casing note too: the sibling options are lowercase ("with", "with beats spaced by").

[FelipeDefensor]

This is identical to BEAT_TIMELINE_BY_INTERVAL_SUFFIX on the line above.

You can reuse the existing constant, or promote a shared SECONDS_SUFFIX for both.