-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MusicXML: add support for numerals #24174
base: master
Are you sure you want to change the base?
Conversation
411fb64
to
453f510
Compare
Backport of musescore#24174
Backport of musescore#24174
Backport of musescore#24174
Backport of musescore#24174
Backport of musescore#24174
Backport of musescore#24174
Backport of musescore#24174
0867810
to
e93caa0
Compare
Backport of musescore#24174
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I've left a couple of requests to do with roman numerals.
String numeralRoot = m_e.readText(); | ||
String numeralRootText = m_e.attribute("text"); | ||
// TODO analyze text and import as roman numerals | ||
ha->setHarmonyType(HarmonyType::NASHVILLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We appear to have just switched the problem around - previously nashville numbers were imported as roman numerals, now roman numerals are imported as nashville numbers. It would be great if we could recognise numerals here, as we've lost export/import consistency for numerals with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not entirely true, because the numeral
element has been ignored until here, the old behavior for function
hasn't changed. But I will add a basic check for Roman numerals in the text attribute.
m_xml.tag("numeral-root", { { "text", textName } }, "1"); | ||
m_xml.endElement(); | ||
// only check for major or minor | ||
m_xml.tag("kind", textName.at(0).isUpper() ? "major" : "minor"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could modify the code you've written below to read inversions to write them here.
15c137f
to
a3ac0c5
Compare
Backport of musescore#24174
This PR adds support for Nashville number system in MusicXML import and export and adds basic support for Roman numerals.
Relates to #19508.