Corruption when expanding multimeasure rests after undoing the deletion of it

• Feb 22, 2016 - 01:38
Type
Functional
Severity
S2 - Critical
Status
closed
Project
MuseScore

Manjaro Linux, revision e666061 Nightly

1. New Score.
2. Press "M" for multimeasure rests.
3. Select the entire "measure" with the multimeasure rest.
4. Press "Delete".
5. Undo the deletion.
6. Press "M" again to expand the multimeasure rests.

Result:
Corruption. Measures that were part of the multimeasure rest appear empty.

Workaround:
Method 1. Changing time signature or deleting the old time signature that the measure had puts the rests back.
Method 2. If the first measure of all staves is corrupted, adding a (temporary) stave, and then following the first method works.

Discussion:
Same corruption happens when selecting a multimeasure rest and other notes and then deleting.
From the debug output, the problem seems to lie in the processing of cmd or AddElement::cleanup.

I would guess that multimeasure rests are not handled properly.
I suspect that it would be fine to handle the deletion of multimeasure rests like normal rests.

Side Note:
When deleting a multimeasure rest in debug mode, a line similar to
"Segment::add(Rest) there is already a Rest at 1:0:0(0) track 0. score 0x54e6e70"
appears.

Comments

• Feb 26, 2016 - 03:32

You're probably right about the commit that caused it. Might be the changes to createMMrests().

A brief investigation suggests the problem is that when we perform the delete (note: it has to be Delete, not Ctrl+Delete - it should look like nothing happened) we are trying to add rests to all the measures but there are already rests there (we get console messages from Segment::checkElement()). And I suspect the undo is removing those rests.

Reply

• Feb 26, 2016 - 19:40

Bottom line: the bug seems to be a result of this specific line in the commit mentioend above:

https://github.com/musescore/MuseScore/blob/a6ec8aa17069595db360a21c8de…

This seems a good change overall - it means the mmrest itself is now being stored with an actual duration rather than 0/1. But somehow it has the side effect of changing what happens when you try to delete it.

I can see what is happening but am not sure yet what to do about it.

Before the change, when deleting the mmrest, it would have a duration of 0/1 and so when we went to delete it, we basically wouldn't do anything. Now, we are deleting the mmrest and recreating it for the full duration, which ultimately has the effect of trying to add new full measures rests to the underlying measures. And since there are already rests there, the add does nothing but does create an undo entry, so undo removes the rest that was already there.

Assuming we want to keep having the mmrest have a real duration instead of 0/1 as it did before, I think we could probbably just modify the delete operation to essentially just skip mmrests as it seems it did before. But I worry there might be other places where we consciously or otherwise rely on the fact that mmrests had a duration of 0/1.

Reply

• Feb 27, 2016 - 23:00

I'm still working on this on and off, and finding it's more complicated. I am thinking that while the specific symptom we are seeing is new as of April, the underlying problem probably goes back further. cadiz1, can you try the following with an older build:

1) My First Score
2) Enter a quarter note into measure 5
3) "M" to enable mmrests
4) Ctrl+A to select all
5) Delete
6) Undo
7) "M" to disable mmrests

I am guessing that there will be corruption in this case, perhaps just in the first four measure, or in the first measure only, or perhaps the first 28 measures.

What I am finding is that the setRest() calls within the body of code I referenced above are using the *total* length of the selected region, and if you have only mmrest measures selected, the length was reported as 0 before the April change, so we'd never call setRest(). But if the selection includes mmrest measures along with other measures, then I think we'd still call setRest() and get the same type of corruption. If the loop through segments were using next1MM(), we'd probably just be counting measure 5 since the mmrests before and after reported a length of 0. But because the loop actually uses next1(), I think probably we'll be counting 28 measures and calling setRest() with that total length.

Assuming that is correct:

I think we really *do* need to call setRest(), otherwise the measures that were *not* part of mmrests originally will end up empty/corrupt. Since there is only a single call to setRest() for the entire selected region, I think maybe setRest() needs to be fixed to be smarter about mmrests. Either that or perhaps we change cmdDeleteSelection() to call setRest() for each measure rather than once for the whole selection - and then we can skip mmrests there.

Reply

• Feb 28, 2016 - 00:31

"I am guessing that there will be corruption in this case, perhaps just in the first four measure, or in the first measure only, or perhaps the first 28 measures."

True: corruption in the first four measures (image below produced with a Nightly of April 7, 2015: 475292c)
7 avril.jpg
And I get exactly the same result (corruption in the first four measures) by going back, month after month, until August 17, 2014.
With eg 01b6a81. Also with two others the same day.

Between May 19, 2014, and August 16, 2014 (eg with this one: 4f9dc38), I receive constantly a crash at the step #5 (Delete)

EDIT: further investigation needed, but the change occurs maybe here? https://github.com/musescore/MuseScore/commit/1886d6d9114bfc31c1e162123…
For fix: #29796: Crash on deleting all notes from part with mmrests; corruption on undo when doing same in score (I note that it is exactly the same steps until the step #5 !)

Reply

• Feb 28, 2016 - 15:51

Great, thanks, that helps a lot! I think also we'll find these problem exist only if the selection *begins* with an mmrest, probably going back to the August 2014 change at least (the crash before then might still happen, not sure).

I'm currently thinking the fix will actually be to go ahead and delete the rests from the underlying measures then add them back - in other words, do the same thing for mmrests that we do for regular measures. This way the change can be localized to cmdDeleteSelection() and thus not risk affecting anything else. If I'm not mistaken, we can accomplish this very easily by checking the start segment of rhte selection to see if it is in an mmrest, and switching to the corresponding real measure if so, keeping everything else in the code the same. Will check this later.

Reply