Repository navigation
Conversation
|
Looks amazing. I still need to play around with this more, but one thing that bothered me is that the edit beat unit dialog gives me denominator and units per tap, instead of denominator and numerator. I think users will expect to just input the time signature as they would in a score. |
|
I am also find the dashed line that connect the time signature to the downbeat dash on hover unnecessary, as they will always be vertically aligned and close to each other (it does look pretty, though). |
|
I added a few small fixes for findings of a /code-review session. |
FelipeDefensor
left a comment
There was a problem hiding this comment.
I marking this as request changes so you get a notification about my questions.
|
Went with the design choice of not storing numerators becasue errors in typing would be very hidden if we omit something. That leave us with knowing how much each beat is worth. This pattern of beat units would also allow the user to tap a clave-stype rhythm (eg 2 dotted quarters + 1 quarter) and still get 4/4. Any measure that has the wrong number of beats now show up automatically with a guessed time signature, tied to the original beat that gave it its beat value (hence the dotted line). I'm thinking specifically of certain pop songs with 1 beat missing/added at the bridge, so the user can enter 4 as the beat pattern (for example) and just change that one bar later and still get all the right time signatures. |
|
🤖 Claude @FelipeDefensor, a review of this PR would help soon. Its rules for beat units and time signatures ( |
…assumed Before the first unit meters_of can read, the measures now take that unit's denominator and units, assumed, as desktop#621 (beat units and time signatures)'s loader gives them: it keeps a unit in the first measure (_ensure_first_beat_unit), copied from the first unit. With no unit at all, a measure of N taps is still N/4, assumed, as #621's default unit is. Either way measure 1 has starts_here true, as in TiLiA, and no beat_unit_id, since no unit sits on its beats. Tests cover a timeline whose first measures have no unit (or only one that can't be read) and a unit later, and one with no readable unit at all. Part of TimeLineAnnotator/project-management#213 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Claude Five changes to
Part of TimeLineAnnotator/project-management#247 |
|
🤖 Claude Rebased onto
Tests on the rebased head, Windows, Python 3.11:
|
…assumed Before the first unit meters_of can read, the measures now take that unit's denominator and units, assumed, as desktop#621 (beat units and time signatures)'s loader gives them: it keeps a unit in the first measure (_ensure_first_beat_unit), copied from the first unit. With no unit at all, a measure of N taps is still N/4, assumed, as #621's default unit is. Either way measure 1 has starts_here true, as in TiLiA, and no beat_unit_id, since no unit sits on its beats. Tests cover a timeline whose first measures have no unit (or only one that can't be read) and a unit later, and one with no readable unit at all. Part of TimeLineAnnotator/project-management#213 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move #621's beat unit rules into tilia_core.timemap, and compute each measure's meter
Copied from tilia/timelines/beat/units.py (desktop#621 at 07cb6d4), unchanged: the file is the same blob as there. Since a17bbd8, #621 refuses a unit longer than MAX_UNIT_LENGTH (32 characters) before Fraction() reads it, so a unit reads the same on every Python, and cuts it short in the message; MeasureMeter holds tuples, so it can be hashed; beat_unit_id is str | None, with a docstring for both copies; and quarter_length is exact for an int. The new TestParseUnits cases are ported. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
meters_of passes the units and beat values as tuples, as #621's _compute_measure_meters now does, so a meter can be hashed. The tests compare tuples, and check that meters hash and that quarter_length is exact for an int, as #621's do. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copied from tilia/timelines/beat/units.py (desktop#621 at 07cb6d4), unchanged: the file is the same blob as there. Since a17bbd8, #621 refuses a unit longer than MAX_UNIT_LENGTH (32 characters) before Fraction() reads it, so a unit reads the same on every Python, and cuts it short in the message; MeasureMeter holds tuples, so it can be hashed; beat_unit_id is str | None, with a docstring for both copies; and quarter_length is exact for an int. The new TestParseUnits cases are ported. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
meters_of passes the units and beat values as tuples, as #621's _compute_measure_meters now does, so a meter can be hashed. The tests compare tuples, and check that meters hash and that quarter_length is exact for an int, as #621's do. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…code shared_examples.json holds 18 beat timelines with the meters and quarter stamps that desktop#621's BeatTimeline gives them, at 07cb6d4, loaded as TiLiA loads a file. Among them are a timeline whose first measures have no beat unit, and one with no unit at all, which #621's loader gives a copy of the first unit, or the default, assumed. The expected values were computed by running #621's code, not written by hand; the time signature's text is #621's beat values written by format_time_signature, since #621 has none. test_timemap_shared.py compares build_rows' meters and time signatures with the file, and checks its layout. Comparing the quarter stamps waits for the time map's (TimeLineAnnotator/project-management#215), so that test is skipped. times_of is empty until the lookups add their cases. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BeatTLComponentManager carries two flags that callers switch off around a bulk operation and back on afterwards. Every one of those windows was a pair of bare attribute assignments that restored a literal True rather than the value found on entry, with no try/finally. That shape is what produced the bug fixed in the previous commit: a misspelled restore (update_is_first_in_measure) silently created a new attribute and left the real flag off for the rest of the timeline's life, and nothing in CI could see it -- the project runs black and ruff with E,W,F,B,I selected, none of which flag an assignment to an attribute that does not exist, and there is no type checker configured. Route all seven windows through suppressing_is_first_in_measure() and the new suppressing_metric_fraction_dict(). Both restore what they found and do so in a finally clause, so a raising body can no longer wedge a flag off. This is behaviour-preserving. Instrumenting every window over a full test run, only delete_components is ever entered with a flag already off (17 of its 32 calls, from restore_state); crop, clear, deserialize_components, restore_state, fill_with_beats and update_is_first_in_measure_of_subsequent_beats find True on entry every single time, so restoring the entry value instead of True cannot change what they do.
Both suppress compute_is_first_in_measure around a bulk deletion and then never recompute, so the timeline keeps a beats_in_measure and measure_numbers describing beats that no longer exist. crop() runs whenever the loaded media gets shorter and the user declines scaling. Twelve beats at pattern 4 cropped to 7.5s leaves 8 beats but still reports measure_count 3, beats_that_start_measures [0, 4, 8], and a metric-fraction dict holding the deleted beats. get_time_by_measure(3) then returns a time with no beat, which misplaces every by-measure CSV and MusicXML import, and "Distribute beats" on the last visible measure raises IndexError because the last-measure guard is fooled by the phantom third measure. Saving in that state serializes the phantom too. clear() is reached from CSV/MusicXML import into an existing beat timeline, from the "delete existing beats?" confirmation in Fill with beats, and from Timelines.clear_timelines. After it the timeline has no beats but still reports three measures, so the import or fill that follows adjusts a stale list instead of building from empty. Extract the recalculate/re-derive/notify trio the fixed delete path already performs into BeatTimeline.refresh_measures() and call it from crop, clear, delete_components and restore_state. Those four had four hand-written variants of the same sequence, differing in whether they posted and whether they guarded on emptiness; that divergence is how delete_components came to be missing its recomputation in the first place. fill_with_beats and deserialize_components are left alone: their sequences genuinely differ (no post, and a different post respectively).
BeatTimelineUI.on_delete_component called recalculate_measures() after super().on_delete_component, which routes unconditionally through BeatTimeline.delete_components -- and that now performs the recalculation itself. The override was a pure pass-through plus a duplicate. The duplicate is not cheap. recalculate_measures starts by clearing every cached metric position, so the second pass throws away the caches the first one just built and re-derives all of them; each cold Beat.metric_position resolves itself with components.index(beat), which copies the component list and scans it. That is O(n^2) per pass, paid twice on the most common delete path. Drop the override and inherit the base implementation.
_get_base_state appended every serializable attribute to the string it hashes twice, through two identical statements. The hash was still deterministic, so nothing misbehaved, but it doubled the work done on every is_file_modified() check and read as though the second line hashed something else. Hashes change, which is harmless: the saved-state baseline is taken again whenever a file is loaded. Part of #582 (item 3).
The docstring said the console logs from WARNING when dev.log_requests is off; the code uses ERROR. Since the same table maps WARNING to tilia.errors, this made it hard to tell whether a message logged with logger.error or tilia.errors.display would reach the console. It now also says when to use each. Part of #582 (item 4).
patch_ask_for_int_dialog patches QInputDialog.getInt the way patch_ask_for_string_dialog patches getText, so tests can answer Get.FROM_USER_INT prompts through the dialog instead of serving the request (#605).
The dialog and inspector spin boxes stopped at 128, but the model, CSV import and CLI accepted any positive denominator. Opening a unit with a larger one clamped it in the spin box, so pressing OK, or Enter in the inspector, silently rewrote it to 128. MAX_DENOMINATOR now lives in tilia.timelines.beat.units and validate_denominator enforces it on creation, on set_data, in set_beat_unit, and in CSV and CLI input.
Showing or hiding a beat timeline's time signatures, and the height a new beat timeline gets for them, were written out three times: in the context-menu command, in `timelines beat time-signatures` and in `timelines add beat`, plus the creation arguments the Qt UI passes. They now live in tilia.ui.timelines.beat_time_signatures, which is free of Qt so the CLI can use it.
CLAUDE.md asks for type hints on every parameter and return in production code. Ruff's ANN rules flagged 60 omissions on lines this branch adds: overridden element and timeline UI methods, dialog and preview constructors, the CLI parser setup functions and the get_additional_args_for_creation classmethods.
App.get_id returns component ids as strings, so the governing beat unit's id is a str at runtime; ints only appear in tests. The docstring now also says what the field holds in tilia_core's copy of units.py: the id of the beat that carries the governing unit, since tilia_core reads a .tla beat unit as part of its beat. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MeasureMeter is frozen but held its units and beat values in lists, so hash(meter) raised TypeError and a set of meters, or a cache keyed on one, couldn't be built; the lists could also still be changed in place. Both fields are now tuples. format_units takes any sequence, since the time signature labels pass it a meter's fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MeasureMeter.quarter_length computed beat_value * 4 / denominator, which is a float when beat_value is an int: quarter_length(1) under a denominator of 6 gave 0.666..., not 2/3. Beat values from the timeline are always Fractions, so nothing reached this yet. The value is now converted to a Fraction first, so the result is exact whatever it is given. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
parse_units passed each unit to Fraction(), and whether that reads a very long integer depends on the Python version: 3.10.7 and later refuse more than 4300 digits, earlier versions read any length. The same file then got different meters on different Pythons, and the error repeated the whole unit. A unit of more than 32 characters, far above anything written in notation, is now refused before Fraction() sees it, with the error any unreadable unit gets. The message shows at most its first 32 characters, followed by an ellipsis. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the first measure has no beat unit, as after adding a pickup before the first beat or when opening a file without one there, _ensure_first_beat_unit copies the next beat unit's denominator and units onto the first beat. It also copied that unit's assumed flag, so the measures before it read as set by the user. Nobody set a time signature on them: the copy is now always assumed, as the default unit it replaces would be. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…from The beat values, flags and quarter stamps in shared_examples.json come from desktop#621's code; its time_signature text is #621's values written by format_time_signature, which test_timemap_table.py checks against texts written by hand. The docstring said every expected value came from #621. The issues the tests cite get short names. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MeasureMeter.beat_unit_id was the BeatUnit component's own id, while tilia-core's copy of units.py, which reads a .tla beat unit as part of its beat, gives the id of that beat. It is now the beat's id here too: a beat holds at most one unit, so its id names the unit. Its three users change with it. _put_beat_unit uses the id as it is, `beat unit remove` deletes get_beat_unit_on_beat(...), and the time signature labels are keyed by the unit's beat. With the component's id gone, the CLI would otherwise have deleted the beat itself once the app uses the core's meters; its test now checks that no beat goes. The docstring gives the field one meaning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A unit of more than MAX_UNIT_LENGTH characters was refused with the error of any unreadable unit, "'1111…' is not a whole number or fraction such as 1/2.", even when it was a whole number. It now says what is wrong: "'1111…' is longer than 32 characters." Units within the limit get the same messages as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… is built MeasureMeter is frozen and its units and beat_values are typed as tuples, but it held tuples only because _compute_measure_meters passed them. Built from lists, a meter could not be hashed and compared unequal to the same meter built from tuples. It now turns both into tuples itself, in __post_init__. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
quarter_length passed its argument to Fraction() so that an int stays
exact, but Fraction() also reads text and floats: quarter_length("1/2")
returned a value, and quarter_length(0.1) the float's exact binary value.
Only an int is now made a Fraction, so text raises TypeError as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…code shared_examples.json holds 18 beat timelines with the meters and quarter stamps that desktop#621's BeatTimeline gives them, at 07cb6d4, loaded as TiLiA loads a file. Among them are a timeline whose first measures have no beat unit, and one with no unit at all, which #621's loader gives a copy of the first unit, or the default, assumed. The expected values were computed by running #621's code, not written by hand; the time signature's text is #621's beat values written by format_time_signature, since #621 has none. test_timemap_shared.py compares build_rows' meters and time signatures with the file, and checks its layout. Comparing the quarter stamps waits for the time map's (TimeLineAnnotator/project-management#215), so that test is skipped. times_of is empty until the lookups add their cases. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…from The beat values, flags and quarter stamps in shared_examples.json come from desktop#621's code; its time_signature text is #621's values written by format_time_signature, which test_timemap_table.py checks against texts written by hand. The docstring said every expected value came from #621. The issues the tests cite get short names. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Claude Rebased onto
Conflicts in the rebase: one, in Tests: after each commit,
|
Moved from tilia/timelines/beat/pattern.py (desktop#621), unchanged: the file is the same blob as at 0ee8ea4, 07cb6d4 and a17bbd8. It sits next to units.py, the other module from #621. The beat importer and adding a beat timeline will call it, and so will the app's beat timeline, its validators and the beat pattern dialog once the app uses the core's copy. Its tests move to test_timemap_pattern.py with only the import changed. Part of TimeLineAnnotator/project-management#270 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moved from tilia/timelines/beat/pattern.py (desktop#621), unchanged: the file is the same blob as at 0ee8ea4, 07cb6d4 and a17bbd8. It sits next to units.py, the other module from #621. The beat importer and adding a beat timeline will call it, and so will the app's beat timeline, its validators and the beat pattern dialog once the app uses the core's copy. Its tests move to test_timemap_pattern.py with only the import changed. Part of TimeLineAnnotator/project-management#270 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copied from tilia/timelines/beat/units.py (desktop#621 at 0ee8ea4), unchanged: the file is the same blob as there. Since 07cb6d4: beat_unit_id is the id of the beat that carries the governing unit, in the app as in the core, with one docstring; a unit longer than MAX_UNIT_LENGTH is refused as too long, not as not a number; MeasureMeter turns units and beat_values into tuples itself; and quarter_length makes only an int a Fraction. The new and changed cases of #621's tests are ported. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A unit could be any fraction of up to MAX_UNIT_LENGTH characters, such as 1/100000000000000000000000000001. Quarter stamps are exact sums of the beats' lengths, so a file whose measures each bring a new large denominator made the stamps' denominators grow by some 30 digits a measure: building them for a few thousand measures took seconds, and from about 150 measures on a stamp had more digits than Python writes as text. parse_units now refuses a fraction whose denominator, in lowest terms, is above MAX_DENOMINATOR (128), the limit of a beat unit's own denominator, with the message of any unreadable unit: "'1/129' is not a whole number or fraction such as 1/2." Whole numbers are unchanged. Every sum of units then has a denominator that divides one fixed number. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Claude
Why. A unit could be any fraction of up to 32 characters. Quarter stamps are exact sums of the beats' lengths, so a file whose measures each bring a new large denominator, such as The change, in
Tests, written first:
Runs:
Guarantee. With the new check neutralised, 6 of these 515 tests fail: the 4 refusals in The core's copy of Part of TimeLineAnnotator/project-management#247 |
Copied from tilia/timelines/beat/units.py (desktop#621 at e5da86a), unchanged: the file is the same blob as there (2ef3be7). Since 0ee8ea4, the commit copied before, parse_units refuses a fraction whose denominator, in lowest terms, is above MAX_DENOMINATOR (128), with the message of any unreadable unit. Whole numbers are unchanged. Sums of units, such as a row's ts_numerator and the running quarter stamps of #747 (quarter stamps), then have denominators that divide one fixed number, where units such as 1/(10**29 + i) made them grow without limit. The new cases of #621's tests are ported. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I agree with the model — this is only about input. The dialog knows the bar's tap count, so it could take a time signature ("6/8", or "2+3/4" for grouped taps) and convert it to units before saving; storage and the derived time signatures don't change. Units per tap would stay available for cases like |
|
I am not clear on what should happen to beat units when we delete a beat in the middle of a timeline. Take this example:
If I delete the selected beat, I would expect every time signature and downbeat dash to shift one measure to the right, instead we get this:
What is going on? This is the file: 621.zip |
Copied from tilia/timelines/beat/units.py (desktop#621 at 0ee8ea4), unchanged: the file is the same blob as there. Since 07cb6d4: beat_unit_id is the id of the beat that carries the governing unit, in the app as in the core, with one docstring; a unit longer than MAX_UNIT_LENGTH is refused as too long, not as not a number; MeasureMeter turns units and beat_values into tuples itself; and quarter_length makes only an int a Fraction. The new and changed cases of #621's tests are ported. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copied from tilia/timelines/beat/units.py (desktop#621 at e5da86a), unchanged: the file is the same blob as there (2ef3be7). Since 0ee8ea4, the commit copied before, parse_units refuses a fraction whose denominator, in lowest terms, is above MAX_DENOMINATOR (128), with the message of any unreadable unit. Whole numbers are unchanged. Sums of units, such as a row's ts_numerator and the running quarter stamps of #747 (quarter stamps), then have denominators that divide one fixed number, where units such as 1/(10**29 + i) made them grow without limit. The new cases of #621's tests are ported. Part of TimeLineAnnotator/project-management#247 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>


Lets the beat timeline know what a tapped beat is worth in notation, so it can show time signatures and convert between seconds and quarter notes. It also makes the beat pattern easier to type and editable after the timeline is created.
What users get
Beat patterns
10[4] 3 15[4]is ten bars of 4, one of 3, then fifteen of 4, and groups nest (2[4 3[3]]). Plain4 4 3still means one bar of each.Beat units and time signatures
1(4/4 tapped in 4),3(6/8 tapped in 2),1/2(2/2 tapped in 4),2+3(5/4 tapped as 2+3). It applies from its measure until the next change.Quarter stamps
BeatTimelineconverts between seconds and positions in quarter notes (get_quarter_stamp_by_time,get_time_by_quarter_stamp), as exactFractions. Stamps are computed, never stored, so correcting a beat unit later corrects every stamp. TiLiA has no repeats yet, so stamps count along the beats in the order they were tapped.Elsewhere
denominatorandunitscolumns; empty cells carry over from the row above.timelines beat pattern set,timelines beat unit set|remove|listandtimelines beat time-signatures show|hide.Commits
Each has its own rationale in its message.
fix: hash each timeline attribute once— Part of Stale measure bookkeeping in BeatTimeline crop/clear, duplicated hash statement, stale log.py docstring #582 (item 3).docs: correct the console log level in TiliaLogger's docstring— Part of Stale measure bookkeeping in BeatTimeline crop/clear, duplicated hash statement, stale log.py docstring #582 (item 4).test: add a patch for integer input dialogs—patch_ask_for_int_dialog, mirroringpatch_ask_for_string_dialog(Replace Serve(Get.FROM_USER_*) with modal-dialog patch helpers in tests #605).test: cover hand-set measures through crop and refill— extra cases for refactor: beat measure-recomputation suppression, and the staleness it was hiding #602's crop and clear fixes: a renumbered measure and a bar set by hand. Addspatch_fill_beat_timeline_dialog.refactor: share time signature glyphs between score and beat timelinesfeat: inspector fields that apply when editing finishes— new row kinds that report on Enter or focus-out instead of every keystroke, for values that aren't valid half-typed or need confirming.refactor: route element repositioning through TimelineUI— so a timeline can redraw items that belong to no element; no change for existing timelines.feat: let timelines.add.* take the values they would prompt for— as TESTING.md asks tests to do.refactor: take the saved-state baseline after every file load— what loading adds to a file (here, the first beat's beat unit) isn't counted as an unsaved change.feat: add parser for bracketed beat pattern syntaxfeat: beat patterns, beat units, time signatures and quarter stamps— the feature itself. Resolves Inital beat pattern cannot be changed later #279.test: answer beat timeline prompts through their dialogs— the beat UI tests that predate this branch now patch the dialogs instead of servingGet.FROM_USER_*(Replace Serve(Get.FROM_USER_*) with modal-dialog patch helpers in tests #605).For review
show_time_signatures, andbeat_patternis now stored as text. Files with a list pattern still load. Beat units are saved asBEAT_UNITcomponents. Opening an old file adds the first beat's beat unit but doesn't mark the file as modified.len(), indexing and iteration still mean beats. Deleting a beat moves its beat unit to another beat of the measure, or to the next measure's first beat.1+2+3over a 2-beat bar give1+2.Testing
The full suite passes locally on Windows / Python 3.12: 774 non-UI tests, plus 944 timeline UI and 633 other UI tests. New tests cover the pattern parser and dialog, beat units through every command that changes beats, the inspector prompts, CSV and CLI, undo/redo with
undoable(), and save and reopen, including a file saved by TiLiA 0.5.15.