Undo of Change Instrument does not work

• Sep 21, 2015 - 18:05
Reported version
3.0
Type
Functional
Severity
S3 - Major
Status
closed
Project
MuseScore
Tags
3.0alpha
3.0alpha-06
undoredo
playback

Ubuntu 14.04, GIT commit: 699a5ee

1) my first score
2) add notes to measure 3
3) add Instrument text to measure 3
4) right click Instrument
5) Change Instrument
6) select Trombone
7) OK
8) play notes to verify change
9) undo
10) play notes to verify

Result: still sounds like trombone. Going to Change Instrument again, Trombone still shows as current instrument. A subsequent undo removes the Instrument text, so it's not just a matter of an extra item on undo stack. Nor does a save/reload fix it.

I discovered this working on #9352: Add ability to set transposition by range. I am trying to get the Change Instrument command to update the transposition for existing notes, and it's working fine except for the key signature, which requires me to have access to the old transposition. I'm trying to get the old transposition from the variable that supposedly stores the old instrument, but it is returning the new instrument instead. So whatever is going wrong with managing the undo is also interfering with my ability to get the transposition working correctly. I expect to fix this issue as part of my implementation for that. The undo command exists and is being executed, so hoepfully it's just a simple typo or something in the management of the original instrument.

Comments

• Oct 28, 2015 - 09:14

For the record, from https://github.com/musescore/MuseScore/pull/2228#issuecomment-142613036

"The cause of the issue is that we are setting the new instrument for the instrument change element before we create the undo object here:

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

So when ChangeInstrument::flip() tries to preserve the original instrument, it's too late - the new instrument is already there. So trying to access the old instrument - either to restore it on undo, or to access its transposition info when re-transposing key signatures - gives the wrong result."

Reply

• Feb 26, 2017 - 04:55

This is still an issue in 2.03 and in master. As mentioned, it not only affects the undo itself (which wouldn't be so bad) but also prevents key signatures from transposing correctly at the point of a change of instrument. It also prevents chord symbols from transposing correctly after an instrument change. Since I am trying to get the instrument change transposition to work in 2.1, it would be nice to get this all happening as well, but my recollection is that it was complicated to fix this, and Werner had indicated he would be looking at it. Since the undo code tends to be fragile, I'm not convinced it is worthwhile to try to fixing this for 2.1, nor do I think failure to fix this means I shouldn't continue to try to get my fix for #9352: Add ability to set transposition by range working with 2.1. I actually *do* have it working in my local branch; I'm just testing all the ancillary issues that were connected with it to see what the status of them is with 2.1.

Reply

• Feb 26, 2017 - 18:54

You can also get a crash in master if you add an instrument change to first chordrest of a score in C major (eg, my first score) then do Undo twice. An assertion failure, actually, trying to remove a non-existent key signature. Guessing it has to do with the fact that C major isn't a "real" key signature at the beginning of a score, because it works (doesn't crash) if the key is anything else. I would hope that fixing the basic undo issue would also fix the crash, but more investigation is needed.

Reply

• Apr 5, 2018 - 16:33

Indeed :-). Lots of water under the bridge since my initial work on this. Not sure what the current status is regarding all the interconnected issues with undo. There are also other issues (both in 2.x and master) due to the whole non-existent-C-major-keysig-at-beginning-of-score issue. Still feels like there needs to be some sort of architectural decision on how to proceed to avoid these sorts of issues.

Reply

• Jul 27, 2018 - 20:09

Finally on this. When I first implemented transposition for instrument changes, it was for master (see https://github.com/musescore/MuseScore/pull/2228), but it wasn't fully tested, and a number of things didn't work because either my code wasn't totally right or there were issues elsewhere. The issue at hand is one of the latter. When I ported my changes to 2.1 (see https://github.com/musescore/MuseScore/pull/3029), I fixed all of these issues I could, including this one. But it doesn't make sense to try to take the fix for this issue out of content - the other things I fixed along the way would still be broken. So I'm going a painstaking process of comparing the master and 2.1 versions of my PR to see what else I need to port. Bottom line should be that instrument changes will work a better overall.

Reply