fix #31571: no key signature upon adding a transposing instrument to a C score #1261
Conversation
|
Even with this fix, C keysig are not created on system after the first one. It causes at least one issue http://musescore.org/en/node/32441 |
|
Yes, good point. I definitely think we should fix the layout so this doesn't cause problems, but we should probably generate C keysigs for subsequent systems anyhow. |
|
I talked quickly with Werner today before he left to holidays again. His preference would be to not create keysig in the score if it's C major. I forgot to ask for the map... Personally, from a high level point of view, I believe it would make more sense to deal with C major like any other key signature. But I might be missing a crucial details. In any case, I will review and merge the other PR and wait for this one. |
|
Well, with my other PR #1285, we should be able to support no key signature as the official way of denoting C major at the beginning of a score and have everything work. I'd just need to redo this PR again, because it is still be the case that things are inconsistent and need updating. We would just have to do it the opposite of how I actually did (changing the places where we do generate a "C" key signature to not generate one instead of vice versa). On the positive side, I think that's probably a smaller change than mine, and likely a smaller impact on the tests. An interesting difference that is neither better nor worse necessarily - my original way, a blind user navigating the score would hear "C major" read aloud explicitly; with @wschweer method, he would have to deduce this from the lack of explicit key signature just like sighted users do. BTW, @vgstef, these discrepancies probably also have something to do with some of the layout issues in the Continuous View panel. One other thing to consider before going through with this: the request (from me and others) for an "atonal key signature" (looks like C major but doesn't transpose). In my mind I was thinking "no key signature" could continue to be a back door to get that functionality. But it would actually be pretty easy, I think, to add explicit support for atonal key signatures. Heck, if we get custom key signatures working again, I think that would give us atonal key signature for free - a custom key signature with nothing in it. Anyhow, I can redo this PR but won't get to it for a couple of days. Meanwhile, it is still the case that #1285 would be good to merge sooner, as it actually brings us closer to where we need to be, and the change we would be making here would just make layout worse is #1285 wasn't already in place. |
|
BTW, as far as just generally deciding which method makes more sense from a high level, I think the difference comes down to, do we want our internal representation to be more closely tied to the appearance of the score (in which case, no keysig for "C" makes sense, since there is no visual representation either) or to the semantics of the score (in which case, "C" is a key just lie any other and should be stored as such). FWIW, MusicXML takes the semantic view at least in this respect - it encourages an explicit C major key signature element at the start of a score in that key. |
|
I've made a new PR containing just the actual bug fix: #1306 The more I think about it, the more I think it does make sense to have initial key signatures for pieces in the key of "C" (and to reproduce this on subsequent systems). Otherwise, we end up doing a lot of special casing for this one particular key signature. Consider - do we remove the key signature when transposing into "C"? Do we constantly remove and recreate key signatures every time sometime presses the "Concert Pitch" button for a score whose key signature happens to be "C" for some instrument in one mode or the other? So I'm leaving this PR open as it does contain most of the necessary code to make this happen (I geuss we still need to look at the key signature of systems after the first). |
|
I wanted to bump this before it gets completely forgotten (I almost had). As things stand now, I don't know that there are user-visible bugs in this area - most of those have been fixed where necessary to work whether or not there is an explicit C major key signature. This one might turn out to have the same root cause: http://musescore.org/en/node/39121, but I assume that could be fixed the same way. There is also http://musescore.org/en/node/29931#comment-171886, which seems like it too could be related (the comment about key signatures not showing on subsequent systems). In any case, I believe things are still inconsistent. Sometimes there will be an explicit C major key signature at the begining of a score (or subsequent system), sometimes there will not be. We should either make a conscious choice to live with that or decide on one way or the other and try to make it happen consistently. |
|
The bug has been fixed in 96f60f4 |
|
That is true, of course. The rest of the code in this PR is what we'd need - or at least, a good part of what we'd need - should we ever decide we're tired of constantly special-casing C major key signatures. But it would make sense to take that part of the code out and make it a separate branch, plus fix the cases that were missed (key signature at the start of systems). I won't bother doing, though, that unless someone says we have decided to go ahead with the change. |
|
For what I understand @wschweer's policy will be as follow:
So again, my understanding is that we will keep on special-casing C major key signatures. |
|
Fair enough. I did see his comment to that effect the other day regarding tempo markings. I'm still not convinced we are 100% consistent with respect to this, but until it causes a problem (other than the ones we have already fixed and the one remaining one we know about with albums), I won't worry about it. |
The main fix here is in instrdialog.cpp. This is where I check to see if there is an explicit key signature in the map at tick 0 in the source staff (the staff used to seed the key signatures in the added instruments), and add it if not.
However, things were in a very inconsistent state in terms of when and where "C" key signatures were created. For instance, upon creation of a new score, initial "C" key signatures are inserted - and saved in the MSCX file - but these were not added to the staves' maps, which was the cause of the current bug. Then, upon load of a file with explicit "C" key signatures, the key signatures were thrown away, with the result that loading and immediately re-saving a score would strip off the "C" key signatures that had originally been created. And there is a visible difference between a score with no key signature versus one with a "C" key signature. A score with no key signature at all has the time signature too close to the clef (compared with 1.3, and also compared to how Gould shows it). The "C" key signatures were stripped off as part of the fix to avoid empty keysig segments, but it was not necessary to strip them off completely just to avoid empty segments. And unless we fix the layout for staves with no key signatures at all, it seems harmful to remove them. There is also another bug with transpose by key that results from staves with no key signature at all - spurious accidentals created as a result of the missing key signature in the original.
So in the end, I elected to standardize on having "C" key signatures explicit both in the score and in the staves' key maps. With my changes, the "C" key signatures are created when new scores are, they are added to the staves' maps, they are saved, and they are preserved upon reload. This solves the bug with adding new transposing instruments, it also solves the accidentals-on-transposition issue, and it also solves the time signature layout issue. And even if we do encounter a score with no key signature at all, I still create a key signature for newly added transposing instruments. So the current bug is fixed either way.
The fix required updating a lot of tests to account for the fact that "C" key signatures present in the test files are no longer stripped off on read (basically, undoing that part of the change from a couple of weeks ago). I verified that nothing else in the tests changed. The one test that appeared to show a slightly bigger change was just a fluke of how diff reports the results.