feat(api): support the full time-signature model#321
Open
webern wants to merge 1 commit into
Open
Conversation
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.9% | 6448 / 7974 |
| Functions | 67.7% | 2224 / 3284 |
| Branches | 49.8% | 5542 / 11132 |
Core HTML report | API HTML report
Commit 302722164546a53f08e0194c3f6204a040bb3c5b.
gen-quality
|
webern
force-pushed
the
claude/eloquent-turing-us0rtv
branch
from
96c10f3 to
16274e6
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.0% | 6481 / 8004 |
| Functions | 67.8% | 2228 / 3285 |
| Branches | 49.8% | 5570 / 11180 |
Core HTML report | API HTML report
Commit 156117869514f0a8463e5a473ccf4c0c7291398b.
gen-quality
|
webern
force-pushed
the
claude/eloquent-turing-us0rtv
branch
from
16274e6 to
1cd06bf
Compare
webern
commented
Jul 8, 2026
| // The unscoped <time> (applies to all staves). staffIndex is INDEX_UNSPECIFIED here. | ||
| TimeSignatureData timeSignature; | ||
|
|
||
| // Normally empty: one entry per staff-scoped <time number="N">. Each entry's staffIndex is the |
Owner
Author
There was a problem hiding this comment.
I think this is fine and it works. It's just that the comment is a bit hard to understand. It should be something like this:
Only use this is if you need different time signatures on different staves.
Furthermore, we should not put a staff index on the time signature structure. Instead this should be a map or a vector of pairs naming the staff index and the time signature override that goes with it.
| namespace api | ||
| { | ||
|
|
||
| // -------- enums (unspecified-first, per mx::api convention) -------- |
Owner
Author
There was a problem hiding this comment.
Don't make separator comments like this.
| common, | ||
| cut, | ||
| singleNumber | ||
| singleNumber, |
Owner
Author
There was a problem hiding this comment.
I don't think this should be a list of symbols but then have this one item that's not exactly a symbol but rather a marker that tells you (I guess?) look somewhere else for the single number that is the symbol?
|
|
||
| // beats/beatType are free-form strings by design: they may hold compound values ("3+2"), | ||
| // decimals ("1 + 2.5"), or compound denominators ("8 + 16"). | ||
| struct TimeSignatureComponent |
Owner
Author
There was a problem hiding this comment.
I don't like this class because to me this looks like what a "normal" time signature should be. This is basically the one that I would want to interact with 99% of the time, but it's called a "component".
| // accessors never throw. The orthogonal display/scoping attributes are plain public fields. | ||
| // | ||
| // Defaults to an implicit, simple 4/4. | ||
| class TimeSignatureData |
Owner
Author
There was a problem hiding this comment.
I don't want this class buried under all this new stuff. I'd rather have all the new, complicated stuff in a separate file. Name TBD. TimeSignatureX where X is something like
Components, Support, Items, I don't know. Something to get this garbage interface out of the way of the simpler use cases.
| return std::holds_alternative<MeteredTimeSignature>(mySignature); | ||
| } | ||
|
|
||
| bool isSenzaMisura() const |
Owner
Author
There was a problem hiding this comment.
This interface is a disaster. I mean the whole thing is terrible. We need to try again.
I think we need to have TimeSignatureData for the simple use case, and something like ComplexTimeSignature for everything else.
Then maybe there's something like a TimeChoice class that switches between the two.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.9% | 6481 / 8009 |
| Functions | 67.8% | 2228 / 3285 |
| Branches | 49.8% | 5568 / 11186 |
Core HTML report | API HTML report
Commit d5a425752ce17d92fa004a46cd7bdbccc17179b4.
gen-quality
|
Redesign the time-signature api around a TimeChoice that switches
between the simple case (the ordinary N/D you want 99% of the time) and
a complex case that quarantines everything unusual. The simple type and
the complex machinery live in separate headers, so ordinary 4/4 work
never has to look at the complex surface.
Public shape (src/include/mx/api/):
- TimeSignatureData.h: TimeFraction (shared beats/beat-type leaf) and
TimeSignatureData { TimeSignatureSymbol symbol; TimeFraction fraction; }
where the symbol is the narrow {unspecified, common, cut}.
- ComplexTimeSignature.h: the full ComplexTimeSymbol vocabulary,
TimeSeparator, TimeRelation, InterchangeableTimeSignature,
MeteredTimeSignature, and ComplexTimeSignature -- itself a
{ metered, senzaMisura } choice mirroring the spec's one real fork.
- TimeChoice.h: the { simple, complex } switch, carrying the
whole-<time> attributes (isImplicit, display).
Guarantees:
- TimeChoice::complex auto-collapses a simple-equivalent meter (one
fraction, no interchangeable, no separator, a symbol the simple case
can express) back to simple, so a plain N/D can never hide in the
complex arena and there is one canonical representation.
- The two symbol enums do not overlap on the unusual values:
single-number/note/dotted-note exist only in the complex vocabulary,
so they cannot appear on the simple type.
Per-staff signatures move to std::map<int, TimeChoice>
staffTimeSignatures on MeasureData (keyed by staff index; no staff
index baked into the signature type).
Choice-type logic (factories, accessors, equality, auto-collapse) lives
in TUs under src/private/mx/api/, keeping the headers declaration-only.
impl: TimeReader translates each core::Time to a TimeChoice;
PropertiesWriter::writeTime regenerates all shapes and takes the staff
index separately; MeasureReader/MeasureWriter route by staff scope and
carry per-staff overrides forward; Converter gains narrow/full symbol,
separator, and relation tables.
Four corpus files newly pass the api round-trip and are pinned
(ly11d/e composite, ly11f symbols, ly11h senza-misura).
webern
force-pushed
the
claude/eloquent-turing-us0rtv
branch
from
1cd06bf to
0a3855c
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.1% | 6565 / 8093 |
| Functions | 68.1% | 2250 / 3306 |
| Branches | 49.8% | 5637 / 11317 |
Core HTML report | API HTML report
Commit 52649b0a7f0c21fdcba3401fb3ba170b6c92396b.
gen-quality
|
webern
changed the title
webern
changed the title
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.
Human Summary
This is now on its third design. A simple version of time signatures and a complex, MusicXML spec complete version, coexist in a choice-like object called TimeChoice. The idea is to hide away the, really rather difficult complexities of time signatures unless you really need it.
Summary
Replaces the flat single-pair
TimeSignatureDatawith a plain data struct that models every MusicXML time shape while keeping the 99% case a one-liner (setSimple("4", "4")to author,beats()/beatType()to read).The shape follows the proposal in #268:
TimeSignatureDataholds a publicstd::vector<TimeSignatureComponent>— one pair is the ordinary simple case, more than one is a composite/additive meter like 2/4 + 3/8 (a compound numerator like 3+2/8 remains a single pair). The rare cases are quarantined in twostd::optionalfields per the api presence/absence conventions:senzaMisura(std::optional<std::string>) — engages unmetered music; the string is the display glyph (usually empty, sometimes "X"). When set, components/symbol/interchangeable are ignored by the writer.interchangeable(std::optional<InterchangeableTimeSignature>) — the alternate meter of a dual pair like 3/4 (6/8), with its own components, relation, symbol, and separator.Also new: per-staff time signatures (
time number="N") via a normally-emptyMeasureData::staffTimeSignaturesvector using the samestaffIndexconvention asKeyData(the singularMeasureData::timeSignatureremains the unscoped case); the widened symbol vocabulary (note,dotted-note— previously dropped); and theseparatorattribute.Implementation:
TimeReadernow translates everytimeelement in a measure (previously it read only the first pair of the first element and silently dropped everything else);MeasureReaderroutes by staff scope and carries per-staff overrides forward alongside the singular value (Cursorgains the carried per-staff state).PropertiesWriter::writeTimeregenerates all shapes symmetrically;MeasureWriteremits one element per non-implicit entry.Convertergains bidirectionalTimeSymbol/TimeSeparator/TimeRelationtables (corenormal()folds into apiunspecified).This is a breaking change to the public api: the
beats/beatTypestring fields are replaced bycomponents(withsetSimpleand first-pairbeats()/beatType()conveniences), and the unusedisEqualTomethod is removed. Call-site migration is mechanical (see the migrated fixtures in this diff).Testing
TimeSignatureApiTest.cpp: composite, senza-misura with/without glyph, interchangeable (composite alternate, relation/symbol/separator), widened symbols, separator, per-staff routing + carry-forward, and nested-field equalitymake testpasses (4800 assertions in 389 test cases) + all examplesmake test-api-roundtrippasses, 163/163 pinnedroundtrip-baseline.txt:ly11d_TimeSignatures_CompoundMultiple,ly11e_TimeSignatures_CompoundMixed,ly11f_TimeSignatures_SymbolMeaning,ly11h_TimeSignatures_SenzaMisura(discovery mode: zero regressions)CMAKE_UNITY_BUILD=ON, batch size 0) clean;make checkcleanNotes:
rpatters1/timesigs_composite-ref.musicxmland the syntheticinterchangeable/senza-misura/time-relationfiles now round-trip their time signatures correctly but remain blocked by unrelated diffs (amusic-fontwhitespace normalization; the api always emittingdivisionswhich those minimal synthetic files omit).References