-
Notifications
You must be signed in to change notification settings - Fork 453
One fix in m21ToXML.py:parseFlatElements() #1806
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
Changes from 3 commits
3525fc0
33a8a91
122842e
28fc8af
67458d0
2fcf874
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ | |
| from music21 import chord | ||
| from music21 import common | ||
| from music21.common.enums import AppendSpanners | ||
| from music21.common.numberTools import opFrac | ||
| from music21 import defaults | ||
| from music21 import duration | ||
| from music21 import dynamics | ||
|
|
@@ -3277,7 +3278,9 @@ def parseFlatElements( | |
| self.parseOneElement(obj, AppendSpanners.NORMAL) | ||
|
|
||
| for n in notesForLater: | ||
| if n.isRest and n.style.hideObjectOnPrint and n.duration.type == 'inexpressible': | ||
| if (n.isRest | ||
| and n.style.hideObjectOnPrint | ||
| and n.duration.type in ('inexpressible', 'complex')): | ||
| # Prefer a gap in stream, to be filled with a <forward> tag by | ||
| # fill_gap_with_forward_tag() rather than raising exceptions | ||
| continue | ||
|
|
@@ -3322,7 +3325,7 @@ def parseFlatElements( | |
| else: | ||
| # if necessary, jump to end of the measure. | ||
| if self.offsetInMeasure < firstPassEndOffsetInMeasure: | ||
| self.moveForward(firstPassEndOffsetInMeasure) | ||
| self.moveForward(opFrac(firstPassEndOffsetInMeasure - self.offsetInMeasure)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this part of the same bug fix or a separate bug fix -- it doesn't seem to have anything to do with complex or not complex types? Thus is it actually being tested by the new tests below? I don't think so, so please test it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated. I'll try to write a test for it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record: this is a bug I introduced back when I was implementing SpannerAnchors in the first place. I assumed the wrong thing about what offset should be passed to moveForward. The end result was that MusicXML write from SpannerAnchor-y scores was pretty busted. And I only noticed now because we're making SpannerAnchors now during MusicXML read, so the bug is triggered.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still working on this test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just pushed this new test. |
||
|
|
||
| self.currentVoiceId = None | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -695,6 +695,27 @@ def test_instrumentDoesNotCreateForward(self): | |
| self.assertTrue(tree.findall('.//note')) | ||
| self.assertFalse(tree.findall('.//forward')) | ||
|
|
||
| def test_complexHiddenRestDoesNotCrashButInsteadCreatesForward(self): | ||
| ''' | ||
| Complex hidden rests were raising an exception. Now they create a forward tag instead. | ||
| ''' | ||
| complexRest = note.Rest() | ||
| complexRest.quarterLength = 5.0 | ||
| complexRest.style.hideObjectOnPrint = True | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah -- these are hidden rests already -- okay -- that was an important part. |
||
| n = note.Note() | ||
| n.quarterLength = 1.0 | ||
| m = stream.Measure() | ||
| m.append(complexRest) | ||
| m.append(n) | ||
| p = stream.Part() | ||
| p.append(m) | ||
| s = stream.Score() | ||
| s.append(p) | ||
| tree = self.getET(s, makeNotation=False) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So all of this only applies if makeNotation is False?????? I'm flabbergasted -- tempted to remove it as an option. There is a reason why we make notation before showing -- to prevent crashes like this. Im really not interested in debugging anything else that comes forward only in makeNotation=False mode.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try to avoid fixing things specific to makeNotation=False in the future. Sorry about that! |
||
| self.assertEqual(len(tree.findall('.//forward')), 1) | ||
| self.assertEqual(len(tree.findall('.//note')), 1) | ||
| self.assertEqual(len(tree.findall('.//rest')), 0) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we test that the forward tag moves forward 5 beats? It's little things like this that help make sure bugs don't creep in in the future.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I'm on it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just pushed this test improvement. |
||
|
|
||
| def testOutOfBoundsExpressionDoesNotCreateForward(self): | ||
| ''' | ||
| A metronome mark at an offset exceeding the bar duration was causing | ||
|
|
||
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.
Greg -- 'complex' durations don't exist in m21ToXml unless makeNotation=False. I don't want fixes in here that cause different code paths for the two -- a complex duration in a stream at this point is an error.