fix #74911: disable system dividers when generating parts #2198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think the failing tests are places where parts were being generated by the test and the references were expecting the generated scores not to have mmrests on, because formerly this only happened when going through the File / Parts dialog. Interestingly, the test then explicitly turns mmrests on, but only after verifying the score against a master that expected them off. So I think I will just want to update masters. Need to look at this more closely to be sure. |
MarcSabatella
force-pushed
the
74911-divider-parts
branch
from
8c43460
to
4e09711
Compare
| @@ -276,7 +276,7 @@ void ExcerptsDialog::createExcerptClicked(QListWidgetItem* cur) | |||
| e->setPartScore(nscore); | |||
|
|
|||
| nscore->setName(e->title()); // needed before AddExcerpt | |||
| nscore->style()->set(StyleIdx::createMultiMeasureRests, true); | |||
| //nscore->style()->set(StyleIdx::createMultiMeasureRests, true); | |||
There was a problem hiding this comment.
could you just delete this line?
|
Looks good. I agree for the tests. Once you checked, can you squash? |
MarcSabatella
force-pushed
the
74911-divider-parts
branch
from
4e09711
to
a6a6916
Compare
|
Done, and yes, I did verify the failing tests were "good" failures and fixed the masters. |
MarcSabatella
added a commit
that referenced
this pull request
Sep 2, 2015
fix #74911: disable system dividers when generating parts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just as parts should have concert pitch default to "off" and mmrests to "on" regardless of the setting in the score, dividers should default to "off".
It's a one-line change, but in implementing it, I noticed we set the concert pitch status and mmrest status in totally different places. While it's possible there is a good reason for this, I'm included to think it was an oversight because these were added at very different times.
The code for to disable concert pitch for parts was added relatively recently. Apparently the score setting for concert pitch was inherited by the part all the way back to before 1.0 (!) and it continued to be the case through most of 2.0 development. This was finally fixed not long before beta 1, by forcing concert pitch off in the Score copy constructor:
7c72079
Whereas the code for mmrests goes back to the beginning of the current history; this is done in the handler for the File / Parts dialog. However, it seems there was a problem with this that I fixed (also around the time of the beta 1) by moving the code to a spot earlier in the same function - basically to a point where it is effectively the same as if it had been done in the constructor:
6e66e12
I like having this done in the constructor rather than the dialog; that way it gets hit no matter how parts are generated (eg, if this can be triggered via command line, plugins, osc, etc). So for this PR I went ahead moved the mmrest setting there too for consistency. No ill effects I can see.