Skip to content
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.

Sign up for GitHub

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

Romantext writer handle repeat bars and measure suffixes like m1a #1435

Merged
merged 3 commits into from
Sep 23, 2022
+52 −5
Merged

Romantext writer handle repeat bars and measure suffixes like m1a #1435

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bda60a3
write out repeat bars and measure suffixes like m1a
malcolmsailor Sep 22, 2022
50bde53
fix type annotation
malcolmsailor Sep 23, 2022
8de236e
Merge branch 'master' into pr/1435
mscuthbert Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 51 additions & 4 deletions music21/romanText/writeRoman.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@
'''

import fractions
import textwrap
import unittest

import typing as t

from music21 import bar
from music21 import base
from music21 import metadata
from music21 import meter
Expand Down Expand Up @@ -255,19 +257,45 @@ def prepSequentialListOfLines(self):
msg = f'further time signature change(s) unprocessed: {unprocessedTSs}'
self.combinedList.append(f'Note: {msg}')

measureNumberString = str(thisMeasure.measureNumber)
if thisMeasure.numberSuffix is not None:
measureNumberString += thisMeasure.numberSuffix

# RomanNumerals
measureString = '' # Clear for each measure

rnsThisMeasure = thisMeasure.getElementsByClass(roman.RomanNumeral)
if (isinstance(thisMeasure.leftBarline, bar.Repeat)
and thisMeasure.leftBarline.direction == 'start'):
measureString = rnString(measureNumber=measureNumberString,
beat=1.0,
chordString='||:',
inString=measureString,
)

for rn in rnsThisMeasure:
if rn.tie is None or rn.tie.type == 'start': # Ignore tied to Roman numerals
chordString = self.getChordString(rn)
measureString = rnString(measureNumber=thisMeasure.measureNumber, # Suffix?
measureString = rnString(measureNumber=measureNumberString,
beat=rn.beat,
chordString=chordString,
inString=measureString, # Creating update
)
if (isinstance(thisMeasure.rightBarline, bar.Repeat)
and thisMeasure.rightBarline.direction == 'end'):
# we want to put the repeat at the beat of the last roman
# numeral to avoid printing an unnecessary indication like
# 'b3' prior to the repeat
last_rn = thisMeasure[roman.RomanNumeral].last()
if last_rn is None:
beat = 1.0
else:
beat = last_rn.beat
measureString = rnString(measureNumber=measureNumberString,
beat=beat,
chordString=':||',
inString=measureString,
)

if measureString:
self.combinedList.append(measureString)
Expand Down Expand Up @@ -332,11 +360,14 @@ def rnString(measureNumber: int,
'''

if inString:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In definition of rnString, change type to t.Union[int, str]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it looks like you're concating int(measure.number) + measure.suffix only to separate them later. why not pass in the suffix separately?

I didn't look carefully enough at rnString() when this went in. It's quite inefficient. It seems to me that it really should be an object whose str gives the correct result so you don't need to keep passing in a the same string to concat to and read from over and over. But that's a later/larger refactor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all.
+1 for sorting out rnString.
Mea culpa for how it looks as it stands though I suspect it ended up that way as one of those solutions that seeks to please everyone ... but ends up half way between different solutions and ends up pleasing no one ;)
See #1393 for an alternative that removes it completely.
I'm happy to advance this sometime.

inStringMeasureNumber = int(inString.split(' ')[0][1:])
if inStringMeasureNumber != measureNumber:
inStringMeasureNumber = inString.split(' ')[0][1:]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it looks like you're concating int(measure.number) + measure.suffix only to separate them later. why not pass in the suffix separately?

Do you mean on this line? The only thing I changed was removing the cast to int. It doesn't split the suffix, it splits the inString on space and then slices the "m" from the beginning. As far as I can tell the suffix stays put once it is added.

I agree this function is clearly inefficient. It seems the obvious thing to do would be accumulate each "word" to a list and then do ' '.join when finished.

# inStringMeasureNumber was previously cast to int, but this fails on
# measures with suffixes ("m1a"). However, now we need to cast
# measureNumber to string in the following comparison.
if inStringMeasureNumber != str(measureNumber):
msg = f'The current measureNumber is given as {measureNumber}, but '
msg += f'the contextual inString ({inString}) refers to '
msg += 'measure number {measureNumber}. They should match.'
msg += f'measure number {measureNumber}. They should match.'
raise ValueError(msg)
else: # inString and therefore start new line
inString = f'm{measureNumber}'
Expand Down Expand Up @@ -542,6 +573,22 @@ def testTypeParses(self):
rn = roman.RomanNumeral('viio6', 'G')
RnWriter(rn) # and even (perhaps dubiously) directly on other music21 objects

def testRepeats(self):
from music21 import converter
rntxt = textwrap.dedent('''
Time Signature: 2/4
m1 ||: C: I
m2 V :||
m3 ||: I :||
m4 ||: I
m5a V :||
m5b I
''')
s = converter.parse(rntxt, format='romanText')
writer = RnWriter(s)
assert '\n'.join(writer.combinedList).strip().endswith(rntxt.strip())


# ------------------------------------------------------------------------------

def testRnString(self):
Expand Down