Actually, you are preventing users from adding 2 staff-type-change at the same place, but what about corrupted scores that already have two at the same place?
I think it just doesn't read the second staff type change. But it's OK, since the two share the same information (because there's only one literal change of staff type).
EDIT: Yes, it just removes the second staff type change, so everything is fine :-)
That PR merely hides the problem and does nothing to fix it.
I haven't looked at the code relating to this problem, but is it possible that this is a wider problem that could still rear its head in other cases, and so actually fixing it would be a good idea?
Even if it isn't a wider problem, I still don't like the idea of there being any code anywhere that would crash on invalid input.
The primary cause of the crash is the previous operation of adding two staff type changes, which is invalid and meaningless. You can do this in 3.4.2 but the data of these two will be synced every time you change either of them, because the literal change of staff type only happens once (the list of staff type used internally only has one instance of the new staff type). If no more than one staff type change element is added to any measure, the relevant code all works out. I would in fact say that adapting the code to allow multiple staff type changes is wrong and causes unnecessary clutter or even further problems (if you don't do it right).
I wasn't suggesting adapting the code to allow multiple staff changes. I was suggesting that in addition to what you've fixed in your PR, we also add a sanity check somewhere to prevent a crash if the score somehow ever gets into this invalid state — or a similar one — again.
@Howard-C Didn't you said it got cleaned upon opening the score if there was two staff type changes at the same place?
Then it shouldn't get into the invalid state that @Spire42 mentionned, no?
Comments
I think we should prevent users from adding two staff type changes at the same tick, just like layout breaks.
EDIT: https://github.com/musescore/MuseScore/pull/5776 does this.
Well done, quickly done. :)
Actually, you are preventing users from adding 2 staff-type-change at the same place, but what about corrupted scores that already have two at the same place?
In reply to Actually, you are preventing… by ecstrema
I think it just doesn't read the second staff type change. But it's OK, since the two share the same information (because there's only one literal change of staff type).
EDIT: Yes, it just removes the second staff type change, so everything is fine :-)
So I'm gonna mark this as "won't fix" because preventing addition of multiple staff type change elements is obviously the better choice ;-)
But that PR would fix it, wouldn't it?
That PR merely hides the problem and does nothing to fix it.
I haven't looked at the code relating to this problem, but is it possible that this is a wider problem that could still rear its head in other cases, and so actually fixing it would be a good idea?
Even if it isn't a wider problem, I still don't like the idea of there being any code anywhere that would crash on invalid input.
It prevents the problem from happening, doesn't it?
In reply to That PR merely hides the… by Spire42
The primary cause of the crash is the previous operation of adding two staff type changes, which is invalid and meaningless. You can do this in 3.4.2 but the data of these two will be synced every time you change either of them, because the literal change of staff type only happens once (the list of staff type used internally only has one instance of the new staff type). If no more than one staff type change element is added to any measure, the relevant code all works out. I would in fact say that adapting the code to allow multiple staff type changes is wrong and causes unnecessary clutter or even further problems (if you don't do it right).
I wasn't suggesting adapting the code to allow multiple staff changes. I was suggesting that in addition to what you've fixed in your PR, we also add a sanity check somewhere to prevent a crash if the score somehow ever gets into this invalid state — or a similar one — again.
In reply to I wasn't suggesting adapting… by Spire42
This would involve searching for the amount of staff type change elements left. Not easy.
@Howard-C Didn't you said it got cleaned upon opening the score if there was two staff type changes at the same place?
Then it shouldn't get into the invalid state that @Spire42 mentionned, no?
In reply to @Howard-C Didn't you said it… by ecstrema
No it won't