Crash due to bad selection after undo of operation originally performed in linked score
Reported version
2.1
Type
Functional
Severity
S2 - Critical
Status
closed
Regression
No
Workaround
No
Project
2.0.3 3c7a69d AND 2.1 dev. 213632d (and current 3.0 dev. 95ee954/ Windows7
1) "My First Score"
2) Create parts -> New all -> Ok
3) Enter a note in main score first measure
4) View Part
5) Select range the MM rest (like image below)
6) Undo
Result: crash
Comments
Former issue. I notice a behaviour change on February 21, 2015.
- Four nightlies work on February 20, eg the latest one, or close: d9e3e93
- Not on February 21, eg daf802e and 41a0e13
After a first checking, nothing really obvious for locating the change.
I see that (but on February 20, so no): https://github.com/musescore/MuseScore/commit/41af30e3c3fd8e0d88e30effd…
The next day, this commit seems not directly related (ie the second nightly on February 21 mentioned just above), but I see the mention of "undo.cpp/MMrest", so? https://github.com/musescore/MuseScore/pull/1797/files
Fixed in branch 2.1, commit f30d2ad063
partially fix #151521: Undoing a multimeasure rest range selection in parts causes a crash
Fixed in branch master, commit ea2cb6428d
partially fix #151521: Undoing a multimeasure rest range selection in parts causes a crash
Partial? So I guess it should remain open till final fix
With f30d2ad, I get a crash with two additionnal steps :
1) "My First Score"
2) Create parts -> New all -> Ok
3) Enter a note in main score first measure
4) View Part
5) Select range the MM rest
6) Undo
Then:
7) Press "M"
8) Undo
Result: crash
A quick investigation suggests the problem with the selection is that we are trying to restore the selection of the original measure rest from the main score, but that rest doesn't exist in the part The rest itself seems to belong to one score, but the measure containing the rest belongs to a different score.
It's not about multimeasure rests specifically, nor is it about the selections being in different scores. I was confused because actually, only the score is having its selection restored. The selection in the part is not actually touched at all, and *that* is the problem: the thing that was selected at the start of the undo operation no longer exists.
I can reproduce versions of this problems lots of different ways, then. The key is to perform an action in the score that has the effect of adding an element to the part. Now switch to the part, select the element that just got added, and undo. The selection in the part is still the element that just got added, but of course it no longer exists. So you'll likely get a crash if you try to do something with the selection.
For example:
1) untitled score
2) generate parts
3) switch to part
4) disable mmrests (to show they are not relevant)
5) enter half note into measure 1
6) switch back to score
7) select the half rest after the half note
8) undo (thus removing the half note and half rest)
9) press "5"
Result: crash. After step 8, the originally selected half rest is removed, but it is still selected. Pressing "5" tries to change its duration to a quarter rest, and bad things happen.
Potentially each part has its own selection, and an undo might affect multiple parts, with some parts having their selections affected and others not. Fixing this correctly probably means modifying the SaveState::undo()/redo() code to save & restore not just the selection for the current score, but for all linked scores.
Perhaps a cheaper partial solution is to *clear* the selection on undo/redo, for all linked scores other than the one whose state is actually being restored?
I started wondering, how do avoid this problem on ordinary commands - not undo, but every edit.
Turns out we already do exactly what I proposed above as the "cheaper partial solution" - clearing selection for all linked scores other than the one we are are working on - as part of endCmd(). So basically, every ordinary thing you can ever do to a score will result in losing selection in all linked scores. For some reason I hadn't realized / remembered that.
So it might seem to make sense to do the same thing as part of endUndoRedo(). But this is tricky in that the undo stack is actually always managed by the root score, even though when we save/restore the selection on any given command, it is for the score we were editing.
Luckily, this suggests an easy way to do the more "correct" fix. Currently, we save the selection for the current score at the end of startCmd(), by creating a new SaveState element on the undo stack. In principle, all we need to do is do this for all linked scores. Initial testing is promising, but I'd need to do a lot more testing before going ahead with anything like that.
This is the "correct" fix alluded to above:
https://github.com/musescore/MuseScore/pull/3160
Again, right now we save state for the current score on every command; for linked scores we just clear the selection. On undo we restore the saved state. Works fine if we perform the undo while editing the same score we were editing when we performed the original command. But if we switch scores between the command and the undo, we restore the selection in the original score but not the one we are currently editing. Depending on what changed, that selection may no longer be valid.
So my fix is to just go ahead and save state for *all* linked scores on every command, and thus restore them for all linked score on every undo. Turns out to be very simple to do this.
Can this one still make it into 2.2?
From what I remember, I like my fix fine, but I'd definitely want people to do some stress testing - performing operations on scores with linked parts and undoing in various combinations of cases like "do in score, switch to part, undo" or "do in part, switch to score, undo" or "do in score, switch to part, do something else, switch back to score, undo, undo, switch back to part, redo" etc etc.
Selection is dropped. There is no crash in master.