-
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 missing Gould arrow quartertone accidentals #6263
Conversation
Looks good to me. @lvinken , what do you think? I think you should rebase this to the 3.x branch though. As far as I can tell you can do that here on GitHub, via the Edit button Not sure what's up with Travis, seems to have hit a timeout, but the build and all the tests passed. |
97b2c22
to
9059e48
Compare
I've just double checked, those accidentals indeed do exist in MusicXML, see https://github.com/w3c/musicxml/blob/6e3a667b85855b04d7e4548ea508b537bc29fc52/schema/musicxml.xsd#L1415-L1418 |
case AccidentalType::SHARP2_ARROW_DOWN: s = "double-sharp-down"; break; | ||
case AccidentalType::SHARP2_ARROW_UP: s = "double-sharp-up"; break; | ||
case AccidentalType::FLAT2_ARROW_DOWN: s = "flat-flat-down"; break; | ||
case AccidentalType::FLAT2_ARROW_UP: s = "flat-flat-up"; break; | ||
case AccidentalType::SORI: s = "sori"; break; | ||
case AccidentalType::KORON: s = "koron"; break; | ||
default: |
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.
Maybe the default should be s = "other"
? While keeping the qDebug()
message, I guess.
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.
Don't think so. The other
value is supposed to go together with the @smufl
attribute.
See the answer bei @mdgood here: w3c/musicxml#263
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.
OK, then let's just add that?
Probably something for a different PR though
map["double-sharp-down"] = AccidentalType::SHARP2_ARROW_DOWN; | ||
map["double-sharp-up"] = AccidentalType::SHARP2_ARROW_UP; | ||
map["flat-flat-down"] = AccidentalType::FLAT2_ARROW_DOWN; | ||
map["flat-flat-up"] = AccidentalType::FLAT2_ARROW_UP; |
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.
Not sure though how or whether to handle an import of "other". Seems the current defaulting to AccidentalType::NONE
alone with a qDebug()
message is reasonable.
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.
As mentioned above, handling "other"
would only make sense, if we'd check the @smufl
attribute. So, for now, I think it's ok to ignore "other"
.
and default to "other" for all otherwise unknown accidentals. This change/commit will cause a merge conflict with musescore#6263
This change/commit will cause a merge conflict with musescore#6263
and fix some MusicXML todos This change/commit will cause a merge conflict with musescore#6263
and fix some MusicXML todos This change/commit will cause a merge conflict with musescore#6263
and fix some MusicXML todos This change/commit will cause a merge conflict with musescore#6263
Looks OK to me. Note that this should go in 3.x and master, I am not quite sure if automatic sync from 3.x to master is in place. |
Thanks. There is no automatic sync between 3.x and master |
and fix some MusicXML todos This change/commit will cause a merge conflict with musescore#6263
and fix some MusicXML todos This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accifental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accifental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accifental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
[MusicXML] Add missing Gould arrow quartertone accidentals
and fix some MusicXML todos and complete the accifental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accifental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accidental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accidental mappings This change/commit will cause a merge conflict with musescore#6263 (it actually includes it completly)
and fix some MusicXML todos and complete the accidental mappings This change/commit will cause a merge conflict with #6263 (it actually includes it completly)
This PR is a simple addition to allow import/export of the added Gould arrow quartertone accidentals in MusicXML 3.1.