Clear destination track before cloning to prevent corruption (#16896):

[CubikingChill]

rests already in dtrack would otherwise coexist with the cloned notes.

Resolves: #16896

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

[cbjeukendrup]

We can't call undo… functions inside the undo or redo method of an UndoCommand: the undo… method causes another UndoCommand to be pushed onto the stack, which should never happen while an UndoCommand's undo or redo is performed.

There are two solutions:

• replace the undoRemoveElement call with something that directly removes the element, not involving the undo stack, and undo that manually in CloneVoice::undo. That's likely not possible though.
• perform the undoRemoveElement calls outside CloneVoice::redo, before calling score->undo(new CloneVoice(…)).

[CubikingChill]

Let me know if the latest approach is good.

[cbjeukendrup]

On a superficial level, it looks correct now. I'll leave it to the team though to decide definitively whether this is the correct solution.

[cbjeukendrup]

Actually, it seems that I can still reproduce the corruption from #16896 (comment) after this PR, and in fact this PR does not address the root cause described there.

On a closer look, it turns out that there are more pre-existing problems with cmdExplode, specifically with CloneVoice. Its redo method is actually full of 'forbidden' calls to undo… methods. They are guarded by a first bool, but they are quite risky. Score::cloneVoice also seems to use a mix of undoable and non-undoable methods.
Anyway, so it looks like CloneVoice should not be an UndoCommand at all; the content of its redo method should be extracted. The operation will still be undoable, because the primitives called in CloneVoice::redo are all undoable on their own (undoRemoveElement, ...).

I suggest putting this PR on hold, until these bigger issues are resolved.

[cbjeukendrup]

See #32850

[CubikingChill]

Actually I ve read #32850 and I am quite determined to do the refactoring. Maybe I have overlooked some issues. I will let you know after there's some substantial progress.

[cbjeukendrup]

(To prevent confusion: I already did the refactoring I mentioned in the PR I mentioned, so we just need to wait for that to be merged)

[CubikingChill]

(To prevent confusion: I already did the refactoring I mentioned in the PR I mentioned, so we just need to wait for that to be merged)

OK great. I was on my mobile github app and I didn't realize it is a PR instead of an issue. I did read the need of refactor somewhere in this repo.

Do you think your refactoring will solve this issue?

[cbjeukendrup]

Although it does improve the situation somewhat, it does not fully resolve the corruption; in particular, it does not yet take into account 'ongoing' notes that start before the exploded range.

[CubikingChill]

Although it does improve the situation somewhat, it does not fully resolve the corruption; in particular, it does not yet take into account 'ongoing' notes that start before the exploded range.

I think you are more experienced than me in this codebase. Do you think I should continue working on this PR AFTER your PR?

[cbjeukendrup]

Yes, my PR moves a lot of stuff around, so anything that's done before my PR is merged would need to be redone because of conflicts. But indeed, after my PR is merged, you can definitely continue working on this issue.

What you can already do as a preparation step, is looking how it is done for pasting, because paste operations need to do something similar (cleaning the destination area before writing new notes/rests onto it). It would be interesting to see if anything from the pasting implementation can be reused in CloneVoices.

[CubikingChill]

step, is looking how it is done for pasting, because paste operations need to do something similar (cleaning the destination area

Great. Looking forward to your PR being merged. Will definitely look into the areas you suggested.