Conversation
Add api::TieLetRing, a start-only structure on NoteData for laissez-vibrer ties, kept separate from the isTieStart/isTieStop pair. Parse a lone <tied type="let-ring"> in parseCurve and emit it in NotationsWriter, with reader/writer/round-trip unit tests.
webern
added
feature
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28487 / 36624 |
| Functions | 74.3% | 6349 / 8550 |
| Branches | 50.6% | 22632 / 44725 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 78.4% | 6046 / 7709 |
| Functions | 63.9% | 2051 / 3211 |
| Branches | 47.6% | 5120 / 10757 |
Core HTML report | API HTML report
Commit b5b8a034c5b81e6c75732f4e54e2c33ff80e66a1.
gen-quality
|
webern
commented
Jul 1, 2026
| // rather than by overloading NoteData's isTieStart / isTieStop. | ||
| struct TieLetRing | ||
| { | ||
| // Whether this note carries an lv (let-ring) tie. When false the remaining |
Owner
Author
There was a problem hiding this comment.
This doesn't quite make sense to me. We might need this, not so sure, but if we do maybe isLvSpecified. It seems like this would be cleaner without this boot at all.
The weirder part is why this would govern all the other attributes.
|
|
||
| // A laissez-vibrer / let-ring tie on this note (a lone <tied type="let-ring"> | ||
| // with no matching stop). Independent of isTieStart / isTieStop. See TieLetRing. | ||
| TieLetRing tieLetRing; |
Owner
Author
There was a problem hiding this comment.
Oh. I see why we have that bool above. Hmm. This is dumb. I think we need to either use std::optional for the first time or consider the IsSpecified/Optional class I opened an issue about. #249
Contributor
There was a problem hiding this comment.
In my experience, std::optional is a great (arguably the best) solution to this kind of problem for any type except an enum or a bool. For those types, std::optional is codesmell. Instead of optional, enums should provide an unspecified option (which should be the {} initialization default) and bools should be converted to 3-state enums (which this repo already does).
…LetRing> Per review feedback on #286: the isSpecified bool inside TieLetRing is awkward because presence of the lv tie should be expressed at the field level, not inside the struct. Change NoteData::tieLetRing from TieLetRing to std::optional<TieLetRing> and remove the now-redundant isSpecified member. Update the writer (has_value()), reader (assign directly), and tests (has_value() / remove parseTieLetRing_isSpecified test).
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28487 / 36624 |
| Functions | 74.3% | 6349 / 8550 |
| Branches | 50.6% | 22632 / 44725 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 78.4% | 6050 / 7713 |
| Functions | 63.9% | 2051 / 3211 |
| Branches | 47.6% | 5123 / 10761 |
Core HTML report | API HTML report
Commit 70107808535867d266ba7e6cd879edb75a5314d5.
gen-quality
|
CI failed fmt-check after the previous commit; this applies clang-format's line-collapse for the now-shorter TieLetRing() initializer list.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28487 / 36624 |
| Functions | 74.3% | 6349 / 8550 |
| Branches | 50.6% | 22632 / 44725 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 78.4% | 6050 / 7713 |
| Functions | 63.9% | 2051 / 3211 |
| Branches | 47.6% | 5123 / 10761 |
Core HTML report | API HTML report
Commit 065eb6b3aba2f1549be99057edf604e7d01f8df8.
gen-quality
|
Trim the musical explanation of what an lv/let-ring tie is (musicians already know) and keep only the non-obvious parts: the MusicXML mapping and why it's a separate struct instead of overloading isTieStart/isTieStop.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28487 / 36624 |
| Functions | 74.3% | 6349 / 8550 |
| Branches | 50.6% | 22632 / 44725 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 78.7% | 6101 / 7754 |
| Functions | 64.3% | 2091 / 3250 |
| Branches | 47.8% | 5182 / 10834 |
Core HTML report | API HTML report
Commit b4ddd789254a03a92e1d9c726ca88a504e6f23fc.
gen-quality
|
Contributor
|
I'm wondering if we should create a similar class like |
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
2 participants
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.
Summary
Adds support for laissez-vibrer (lv, or "let-ring") ties to mx::api.
An lv tie applies to a single note and has no matching stop, so it does not fit the existing
start/stop tie model. Rather than overloading
NoteData::isTieStart/isTieStop, it is modeledwith its own start-only structure,
api::TieLetRing(the shared visual attributes:PositionData,CurveOrientation, and optionalColorData), held inNoteData::tieLetRingas astd::optional<TieLetRing>. In MusicXML this is a lone<tied type="let-ring">notation; thesound-level
<tie>element has no let-ring type.parseCurveinCurveFunctions.hroutes a lone<tied type="let-ring">(
core::TiedType::letRing) intoNoteData::tieLetRing. Anif constexprguard keeps the branchout of the slur instantiation, whose type has no let-ring alternative.
NotationsWriteremits a<tied type="let-ring">from the field, carrying position,orientation, and color.
Testing
CurveFunctionsTest.cpp:parseTieLetRing_*(position, placement, color,orientation),
parseCurve_letRing_routesToTieLetRing,parseCurve_start_doesNotSetTieLetRing,writeAttributesFromTieLetRing_*, andtieLetRing_roundTrip.References