-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add support for section restart to MEI import #23022
Conversation
5b507f4
to
71780a2
Compare
2b47ee9
to
52be828
Compare
Measure* measure = dynamic_cast<Measure*>(m_score->measures()->last()); | ||
if (measure) { |
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.
The following would be more MuseScore-idiomatic:
Measure* measure = dynamic_cast<Measure*>(m_score->measures()->last()); | |
if (measure) { | |
MeasureBase* mb = m_score->measures()->last(); | |
if (mb->isMeasure()) { |
Alternatively, at the start of the method do:
MeasureBase* lastMeasureBase = !m_score->measures()->empty() ? m_score->measures()->last() : nullptr
and then check if (lastMeasureBase)
or lastMeasureBase && lastMeasureBase->isMeasure()
where appropriate (also at the bottom of the method)
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.
@cbjeukendrup Thanks, looks more elegant now. But unit test fail. Any idea?
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.
Ah, I think I see the potential problem: what's the last measure at the top of the method, might not be the last measure anymore at the bottom of the method. In between, this->readMeasure
for example might add another measure.
So, it looks like you inevitably need to query the last measure twice.
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.
Of course, you're right. I reverted the latter change, so now it should be good to go.
This PR adds support for MEI's
section@restart
to the MEI importer.(Also it removes an outdated comment I overlooked in #22943.)