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 #222031: remove ties and spanners when removing induvidual notes within a chord #3564

Merged
merged 1 commit into from Jun 2, 2018

Conversation

Projects
None yet
2 participants
@dmitrio95
Copy link
Contributor

commented Mar 20, 2018

In the current version of MuseScore removing individual notes within a chord behaves differently compared to removing the whole chord. While this can be a little surprising from the user's point of view, such a behavior may also lead to crashes like the one described in this bug report.

This pull request is aimed to handle individual notes removal in a way more similar to the case of removal of the whole chord, i.e. remove properly ties and spanners connected to the note being removed and restore the correct previous configuration when undoing the removal.

@dmitrio95 dmitrio95 force-pushed the dmitrio95:bugfix/flute_0_crash branch from a48d0df to 1b8679d Mar 21, 2018


void Note::connectTiedNotes()
{
if (tieBack()) {

This comment has been minimized.

Copy link
@anatoly-os

anatoly-os Apr 2, 2018

Contributor

Is it possible that tieBack() returns nullptr? If it is, whether it is a corrupted logic or usual case?
If it is a corrupted logic, it is better not to check nullptr since it allows finding the corrupted place faster thanks to the crash. It is better for development to have crashes on invalid program state, than checking nullptr and skipping such corrupted program state.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 2, 2018

Author Contributor

Yes, note may have only a forward tie or no ties at all, and in those cases tieBack() will return nullptr. I see nothing wrong about it.

This comment has been minimized.

Copy link
@anatoly-os

anatoly-os Apr 2, 2018

Contributor

Is calling connect/disconnect valid in the case of nullptr ties?

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 2, 2018

Author Contributor

Yes, the purpose of these member functions is just to ensure that all ties that should be connected to the given note will be properly connected (or disconnected) with this note and those notes that should be bound to it with the ties. The note is not required to have any of those ties, and if it doesn't have them then these functions will just do nothing.

In fact, these functions are based on the code that was placed in undo.cpp and chord.cpp to handle ties connection/disconnection when adding/deleting a single note. I moved it to separate functions to make note deletion process more unified (which was the purpose of the whole commit) and reduce code duplication, and made some changes to make it suitable for more general case that that of deletion and addition of the note to the chord. Although my "generalization" may be not so good, the discussed checks on existence of backward and forward ties were present in the previous version of this code too (see, e.g., this), so I have not added anything when assuming that the note in the considered cases may have just one tie or no ties.

@anatoly-os anatoly-os merged commit be5d711 into musescore:master Jun 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@anatoly-os

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2018

@dmitrio95 there are conflicts when manually merging the fix to 2.3. Could you please create the separate PR to 2.3 for this fix?

@dmitrio95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2018

@anatoly-os OK, I will prepare a new PR when I have a time.

@anatoly-os

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2018

Thank you!

@dmitrio95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

@anatoly-os Just have opened #3712 with this patch ported to 2.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.