Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #210116: Delete all and Undo duplicates the clef #3700

Closed

Conversation

mattmcclinch
Copy link
Contributor

@anatoly-os
Copy link
Contributor

anatoly-os commented Jun 1, 2018

Personally, I like it. @wschweer could you please review?

@anatoly-os anatoly-os requested a review from wschweer June 1, 2018 15:57
@mattmcclinch
Copy link
Contributor Author

I replaced more occurrences of Measure::add() and Segment::add() with Score::undoAddElement(). I could produce the same error after trying to delete a courtesy key signature, and I got warnings after trying to delete bar lines.

@wschweer
Copy link
Contributor

wschweer commented Jun 8, 2018

It should not be possible to delete a generated element. For the system header the clef segment should not be deleted if empty but set disabled. It will be reenabled later by layout when generating a new clef. So no undo/redo record is necessary for the clef segment. This also avoids confusion on undo.
I have another patch which tries to address this and at least fixes the clef problem.

@Jojo-Schmitz
Copy link
Contributor

So this pr could be closed, right?

@mattmcclinch
Copy link
Contributor Author

@wschweer, thank you for your comments. I have tested your solution, and I would like to make the following notes.

  1. Since it is possible for an element to lose its “generated” status, it is still possible to delete an element that was originally generated automatically, by first making it invisible (for example). Perhaps it would be helpful to add a “modified” flag, and keep generated elements “generated”. That way, it would be impossible to delete a required element, even if it was modified after being generated.

  2. In Score::undoRemoveElement() in libmscore/edit.cpp, when deciding whether the segment should be disabled or removed, perhaps a better test would be
    if (s->header() || s->trailer())
    instead of
    if (s->isHeaderClefType()).
    I can still cause a crash related to this issue, but this change seems to fix it.

@wschweer
Copy link
Contributor

I changed undoRemoveElement() as you suggested.

@wschweer wschweer closed this Jun 14, 2018
@mattmcclinch mattmcclinch deleted the 210116-undo-delete-clef branch September 5, 2018 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants