Skip to content

fix: beats added after a deletion lose measure structure - #597

Closed
FelipeDefensor wants to merge 2 commits into
TimeLineAnnotator:0.6.5from
FelipeDefensor:bug/v064-beat-pattern-delete
Closed

fix: beats added after a deletion lose measure structure#597
FelipeDefensor wants to merge 2 commits into
TimeLineAnnotator:0.6.5from
FelipeDefensor:bug/v064-beat-pattern-delete

Conversation

@FelipeDefensor

@FelipeDefensor FelipeDefensor commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Fixes the beat timeline losing its measure structure for every beat added after a deletion.

The bug

BeatTimeline.delete_components disables the component manager's compute_is_first_in_measure flag for the duration of the deletion, then re-enables a misspelled attribute:

self.component_manager.compute_is_first_in_measure = False
for component in list(reversed(components)):
    self.component_manager.delete_component(component)
self.component_manager.update_is_first_in_measure = True   # <- wrong attribute

The real flag stays False for the rest of the timeline's life. Every beat created afterwards takes the early-out in BeatTLComponentManager.create_component, so it skips recalculate_measures(), skips beat.is_first_in_measure = ..., and skips the BEAT_TIMELINE_MEASURE_NUMBER_CHANGE_DONE post — leaving the Beat.__init__ default is_first_in_measure = False. Result: short dashes everywhere, no measure numbers.

Deleting all beats made it worse. The recompute was guarded by if not self.is_empty:, so an emptied timeline kept a stale beats_in_measure / measure_numbers describing beats that no longer existed.

Not limited to deleting everything — any multi-beat deletion leaves the flag stuck, so beats added after a partial delete are equally broken.

The fix

Correct the attribute name, and move recalculate_measures() and the measure-number-change post out of the emptiness guard so an emptied timeline drops its stale measure data and the UI clears its labels.

Two regression tests in tests/timelines/beat/test_beat_timeline.py cover delete-all and delete-some followed by reinsertion. Full suite green: 1349 passed, 9 skipped, 2 xfailed, 2 xpassed.

Repro bundle

Bundle format per #580. Nothing repro-specific is committed — the fixture is generated into .repro/, which this PR adds to .gitignore.

One-time setup, and note that re-running a checkout after you have made local edits can throw them away:

gh pr checkout 597

Build the fixture, from this branch's checkout at the repo root:

mkdir -p .repro && printf '%s\n' \
  'metadata set-media-length 60' \
  'timelines add beat --name "Measures" --beat-pattern 4' \
  'components beat --tl-name "Measures" --time 1' \
  'components beat --tl-name "Measures" --time 2' \
  'components beat --tl-name "Measures" --time 3' \
  'components beat --tl-name "Measures" --time 4' \
  'components beat --tl-name "Measures" --time 5' \
  'components beat --tl-name "Measures" --time 6' \
  'components beat --tl-name "Measures" --time 7' \
  'components beat --tl-name "Measures" --time 8' \
  'components beat --tl-name "Measures" --time 9' \
  'components beat --tl-name "Measures" --time 10' \
  'components beat --tl-name "Measures" --time 11' \
  'components beat --tl-name "Measures" --time 12' \
  "save \"$PWD/.repro/beat-delete-reinsert.tla\" --overwrite" \
  | uv run --python 3.12 tilia --user-interface cli

That leaves you on a "Measures" beat timeline with beat pattern 4 and twelve beats — three full measures. It is media-free (media_path empty, media length set), so it opens with no prompt and no error.

See the bug, on the base ref:

FIXTURE="$PWD/.repro/beat-delete-reinsert.tla"
git worktree add --detach ../tilia-base 0.6.5
cd ../tilia-base
uv run --python 3.12 tilia "$FIXTURE"

See the fix, back in this branch's checkout:

uv run --python 3.12 tilia "$PWD/.repro/beat-delete-reinsert.tla"

Close with git worktree remove ../tilia-base. Same fixture file both times; only the app code differs.

On Windows, run these in PowerShell — Git Bash expands $PWD to an MSYS path (/c/Users/...) that the interpreter cannot open.

Acceptance criteria

  • Do: select all beats on "Measures" and delete them, then add beats back.
  • Was: every new beat drew as a short dash, no measure numbers appeared, and no beat was marked as starting a measure.
  • Should be: every 4th beat draws as a long dash and starts a new measure, with measure numbers 1, 2, 3, … as before the deletion.

What this bundle does not verify

The fix, the regression tests and the scenario all come from one author. The before/after pair shows that the scenario's behavior changed; it cannot show that the scenario is the bug as reported. The scenario and the criteria above are taken from the issue text rather than from the fix, but they exercise one path only: delete-all followed by clicking beats back in. Not covered — deleting a subset, Fill with beats as the reinsertion route, undo/redo across the deletion, and beat patterns other than 4. The regression tests cover delete-all and delete-some at the model level.

`BeatTimeline.delete_components` disabled the component manager's
`compute_is_first_in_measure` flag for the duration of the deletion, but
re-enabled a misspelled attribute (`update_is_first_in_measure`). The real
flag stayed `False` for the rest of the timeline's life, so every beat
created afterwards skipped `recalculate_measures()` and kept the default
`is_first_in_measure = False`: no measure numbers, no long dashes.

Deleting *all* beats made it worse. The recompute was guarded by
`if not self.is_empty:`, so an emptied timeline kept a stale
`beats_in_measure` / `measure_numbers` describing beats that no longer
existed.

Fix the attribute name, and move `recalculate_measures()` and the
measure-number-change post out of the emptiness guard so an emptied
timeline drops its stale measure data and the UI clears its labels.
A fix under review is verified by generating a .tla into .repro/ and opening it
on both the base ref and this branch. Nothing there is tracked, so keep it out
of git status rather than leaving the reviewer to notice it themselves.
@FelipeDefensor
FelipeDefensor force-pushed the bug/v064-beat-pattern-delete branch from 1a365b4 to 84c1bab Compare September 5, 2026 14:20
@FelipeDefensor

Copy link
Copy Markdown
Collaborator Author

Superseded by the combined 0.6.5 release branch, which carries this fix with one correction from review.

Renaming the misspelled restore to compute_is_first_in_measure = True fixed the reported bug but introduced a regression on the undo path: BeatTLComponentManager.restore_state suppresses recomputation across a whole delete-then-create pass and reaches delete_components in the middle of it, so forcing the flag back on re-enabled per-beat recomputation for every beat the restore re-creates. Measured on a 100-beat restore: recalculate_measures() went from 1 call to 101, and 0.010s to 0.078s, growing quadratically.

The release branch routes the suppression through a context manager that restores the value it found rather than a literal True, and restores it in a finally so a raising delete can no longer leave the flag off for the rest of the session. Two regression tests cover both, and both fail against this branch's version.

The repro bundle was removed as the commit message intended. Follow-up work for dev — the same context-manager treatment for the other six suppression windows, the equivalent staleness in crop() and clear(), and a duplicate recalculate_measures() in the beat timeline UI — is prepared separately.

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