Regression: Removing the content of measures after deletion of the time signature causes corruption/crash
2.1 R.C / Windows 10
First reported here: https://musescore.org/en/node/188046
1) "Untitled" score
2) Enter some notes in the first measures (not really necessary)
3) Delete the time signature
4) Remove the content (Del only, not Ctrl + Del) of the first measure, then the second, the third, etc.
Result: corruption which leads to crash
Comments
Not the same issue with the 2.0.3. Investigation.
In February...
Somewhere between February 20 et 21...
- This nigthly works: fa8fdd3
- Not this one: 82110fc
Problems for loading intermediate nightlies (due to Qt5 widgets.dell missing)
So, maybe the second mentioned nightly/commit?
For fix (one of mine!): #167416: Cut a measure with triplets after inserting a new measure causes corruption/crash
So 3 commits being the possible culprit
82110fc
8620022
d2cc02b
I vote for the first one ?
Me too, the others are very unlikely. But I don't quite understand what's wrong with it.
The difficulty of fixing #167416: Cut a measure with triplets after inserting a new measure causes corruption/crash seems to have been not to break #29796: Crash on deleting all notes from part with mmrests; corruption on undo when doing same in score again.
So in master I guess c2ef50f646 is equally broken
Simplier steps:
1) "Untitled" score
2) Delete time signature
3) Ctrl + A
4) Del
Result: Corruption
Am I correct in understanding this only applies when deleting the *initial* time signature in a score? I can't reproduce a problem any other way.
It seems the problem has do do with this part of the code specifically:
https://musescore.org/en/node/188456
We've already cleared the contents of the measure - deleting the full measure rests. Now we're looping through the measures adding new rests, stopping when we get to the end of selection, but we only do this if we can figure out what time signature we are in. Otherwise we keep skipping this section of the code.
Probably we need to execute this section of code anyhow. Maybe just checking ts before using it get the value of f, but setting f to 4/4 if ts is zero. Or maybe there is some other way we are supposed to know the time signature. Or maybe we borrow the code from cmdFullMeasureRest(), which seems to work fine (it doesn't rely on knowing the time signature)?
Another corruption happens for similar reasons if the actual duration of the measure differs from what the time signature says. The same section of code highlighted above adds a rest of length equal to the time signature rather than what would actually be needed to fill the measure.
So I'm thinking, why mess with time signatures at all? Why not just use m->len()? Partial answer: we need to do the right thing in the presence of local time signatures. But probably there is a better way of dealing with all this.
In your comment #9, the link is not related to the section of code you talk about.
Yikes, you are right! I meant this:
https://github.com/musescore/MuseScore/blob/2.1/libmscore/edit.cpp#L247…
So, definitely related to the first commit shown by your investigation earlier. That section of code had long (?) been disabled because it had issues. The change in question re-enabled that section of code and fixed some of the issues, but there are more. I am continuing to investigate to see if I can come up with a safe fix.
FWIW, in 2.0.3, it works OK if the actual duration differs from nominal, *or* if there is a local time signature involved. But if you have *both* going on, you also get corruption. Not just from this operation, other things fail too. See #189741: Corruption on operations involving measures with local time signature and modified actual duration, which I just filed.
I think for now we need to not support the combination of local time signature and altered actual durations, as there are probably lots of problems that would result. Apparently we detect this if the change in actual duration is already ion palce when we attempt to add the local time signature, but the reverse is not true.
Anyhow, I'll attempt a fix for the deletion issue that does work even if we have both situations going on, but I also think for now we should disable the change in actual duration if there is a local time signature.
Here's my attempt at a fix. Pretty straightforward, but handle all cases I could throw at it. We still need to check all the other issues connected to this section of code, and any other stress testing anyone cares to do:
https://github.com/musescore/MuseScore/pull/3147
I've tested all the cases in #167416: Cut a measure with triplets after inserting a new measure causes corruption/crash and in #29796: Crash on deleting all notes from part with mmrests; corruption on undo when doing same in score, and my fix seems good for all.
Fixed in branch 2.1, commit cc75dab328
fix #188051: corruption on delete with no/modified time signature
Fixed in branch 2.1, commit a6df2ff581
Merge pull request #3147 from MarcSabatella/188051-delete-corrupts
Fix #188051: corruption on delete with no/modified time signature
It's merged and a nightly should be available soon. It would be good if we could test as many cases as possible.
Automatically closed -- issue fixed for 2 weeks with no activity.
This was never merged for master so the problem still exists. Same fix hopefully applies, we'll see...
Werner had actually fixed the original problem in a separate commit:
https://github.com/musescore/MuseScore/commit/f1b27da088a9a67d59baa11c7…
But his fix only addressed the case of no time signature at all, not the case of a modified one (actual duration != nominal). as per my comment in https://musescore.org/en/node/188051#comment-700151
So I'm going ahead and applying my fix from 2.x to master as well.
https://github.com/musescore/MuseScore/pull/3834
Fixed in branch master, commit 3c81c05947
fix #188051: corruption deleting contents of measure with modified duration
Automatically closed -- issue fixed for 2 weeks with no activity.