Fix xml serialization of fingering articulations #323
Conversation
music21/musicxml/m21ToXml.py
Outdated
| @@ -3858,7 +3858,7 @@ def noteheadToXml(self, n): | |||
| mxNotehead.set('color', color) | |||
| return mxNotehead | |||
|
|
|||
| def noteToNotations(self, n, notFirstNoteOfChord=False, chordParent=None): | |||
| def noteToNotations(self, n, pitchNumber, chordParent=None): | |||
| ''' | |||
There was a problem hiding this comment.
This needs a default value for pitchNumber.
There was a problem hiding this comment.
and again the name "pitchNumber" could mean too many different things.
music21/musicxml/m21ToXml.py
Outdated
| @@ -3099,7 +3099,7 @@ def objectAttachedSpannersToNotations(self, obj, objectSpannerBundle=None): | |||
|
|
|||
| return notations | |||
|
|
|||
| def noteToXml(self, n, addChordTag=False, chordParent=None): | |||
| def noteToXml(self, n, pitchNumber=0, chordParent=None): | |||
There was a problem hiding this comment.
pitchNumber is too vague as a variable name -- does it mean C=0, C#=1? is it midi number? it might be "noteIndexInChord" or something like that
music21/musicxml/m21ToXml.py
Outdated
| applicableArticulations = [] | ||
| fingeringNumber = 0 | ||
| for a in chordOrNote.articulations: | ||
| if type(a) is Fingering: |
There was a problem hiding this comment.
do not do type(x) checks. use isinstance -- otherwise subclasses fail. But better still is use the music21.base.Music21Object.classes or classSet interface -- it is much faster than isinstance.
music21/musicxml/m21ToXml.py
Outdated
| applicableArticulations.append(a) | ||
| fingeringNumber += 1 | ||
| elif pitchNumber == 0: | ||
| applicableArticulations.append(a) |
There was a problem hiding this comment.
what if there are more Fingering articulations than pitches? will they be lost?
There was a problem hiding this comment.
this is otherwise really good!
There was a problem hiding this comment.
yes they will be intentionally lost. I added that behavior to doc and test
music21/musicxml/m21ToXml.py
Outdated
|
|
||
| # only apply expressions to notes or the first note of a chord... | ||
| if pitchNumber == 0: |
There was a problem hiding this comment.
I still prefer setting up a separate variable that makes more clear what "pitchNumber == 0" means:
if isSingleNoteOrFirstInChord: even a wordy expression like this is helpful to show what is being tested.
music21/musicxml/m21ToXml.py
Outdated
| @@ -3858,7 +3858,7 @@ def noteheadToXml(self, n): | |||
| mxNotehead.set('color', color) | |||
| return mxNotehead | |||
|
|
|||
| def noteToNotations(self, n, notFirstNoteOfChord=False, chordParent=None): | |||
| def noteToNotations(self, n, pitchNumber, chordParent=None): | |||
There was a problem hiding this comment.
need tests to show that this works
|
Do we need corresponding changes in xmlToM21? I hate to have the capabilities of import become unbalanced from export. |
|
import already works as intended (the articulations of the chord are collected from every note) |
Unlike other articulations, fingers should not only be applied to the first note of a chord. Fingerings must be applied in orderd, one each to one note respectively. This commit fixes this behavior.
|
A new inspection was created. |
|
@mscuthbert I updated the pull request and believe I have now addressed all your review comments. |
|
there's one unused variable in there, but I can fix that. Congrats and THANK YOU! Approved! |
Unlike other articulations, fingers should not only be applied to the first note of a chord. Fingerings must be applied in orderd, one each to one note respectively. This commit fixes this behavior.