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

[MU4] Fix #319079: Shift Selecting Last Word #7793

Merged

Conversation

ht-gong
Copy link
Contributor

@ht-gong ht-gong commented Mar 24, 2021

Resolves: https://musescore.org/en/node/319079

Original code only attempts to find the next ChordRest segment as the end marker for selecting a range, this fix adds the detection for the EndBarLine segment as a potential end marker, which resolves the issue.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Mar 25, 2021

Now just the commit message (the body) doesn't fit anymore.

Unfortunatly this change leads to a crash, if the start and end syllable are in different staves (and the end one is at the end of the score and in a higher staff), it dies of a failed assertion:

Q_ASSERT(staffEnd > staffStart && staffStart >= 0 && staffEnd >= 0 && staffEnd <= _score->nstaves());

Independant of whether we'd use the 1st or the 2nd proposed fix, or none at all.
Without it that selecting in the 'wrong' order doesn't work at all (not even if not including the last sylllable of a score), so this fix just uncovered another bug (but crashes on a debug build only).
Maybe we should remove or modify that assert?
I see nothing strictly illegal about staffEnd > staffStart not being the case.
But while removing that condition from the Q_ASSERT() fixes the crash, it still doesn't make such a selection work :-(

Anyway, this hasn't got anything to do with the issue this PR is fixing.

@ht-gong
Copy link
Contributor Author

ht-gong commented Mar 25, 2021

Now just the commit message (the body) doesn't fit anymore.

Unfortunatly this change leads to a crash, if the start and end syllable are in different staves (and the end one is at the end of the score and in a higher staff), it dies of a failed assertion:

Q_ASSERT(staffEnd > staffStart && staffStart >= 0 && staffEnd >= 0 && staffEnd <= _score->nstaves());

Independant of whether we'd use the 1st or the 2nd proposed fix, or none at all.
Without it that selecting in the 'wrong' order doesn't work at all (not even if not including the last sylllable of a score), so this fix just uncovered another bug (but crashes on a debug build only).
Maybe we should remove or modify that assert?
I see nothing strictly illegal about staffEnd > staffStart not being the case.
But while removing that condition from the Q_ASSERT() fixes the crash, it still doesn't make such a selection work :-(

Anyway, this hasn't got anything to do with the issue this PR is fixing.

Anything that I should do to correct the commit message? The updated commit message is only associated with my second commit.

Also did some tests around the issue mentioned above. Took a crack at a fix by adding this comparison in the Score::selectRange(Element* e, int staffIdx) function that reverses the two indexes if they don't satisfy idx2 > idx1:

            int idx1 = selectedElement->staffIdx();
            int idx2 = e->staffIdx();
            if (idx2 < idx1) {
                int temp = idx1;
                idx1 = idx2;
                idx2 = temp;
            }

Surprisingly, with some elementary testing, this seems to resolve the issue.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Mar 25, 2021

Your commit message is fine, but you should squash those 2 commits into just one (and keep the 2nd message).
Fixing that other issue might be something for a separate commit or even separate PR?
But yes, that additional fix indeed seems to work fine.

@ht-gong ht-gong force-pushed the issue-319079-shift-selecting-last-word branch from c1de600 to 46c1fba Compare March 25, 2021 09:08
Changed condition to only check for s1. s2 might be nullptr which marks end of score.
@ht-gong ht-gong force-pushed the issue-319079-shift-selecting-last-word branch from 46c1fba to 9058efa Compare March 25, 2021 09:15
Copy link
Contributor Author

@ht-gong ht-gong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, wrong button.

@ht-gong
Copy link
Contributor Author

ht-gong commented Mar 25, 2021

Your commit message is fine, but you should squash those 2 commits into just one (and keep the 2nd message).
Fixing that other issue might be something for a separate commit or even separate PR?
But yes, that additional fix indeed seems to work fine.

Committing the second fix now :D

Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me!

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 10, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 10, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793
@vpereverzev vpereverzev merged commit a7e4159 into musescore:master Apr 12, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 13, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 13, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 17, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 17, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 26, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 26, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 26, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 26, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 1, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 1, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 2, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 1
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 2, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 2
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 10, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 1
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 10, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 2
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 23, 2021
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 1
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 23, 2021
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 2
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
Changed condition to only check for s1. s2 might be nullptr which marks
end of score.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 1
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
Assert enforces ending staffidx to be higher than starting staffidx. This code reverses the two indexes if they don't satisfy idx2 >= idx1.

Backport of musescore#7793, resp. duplicate of musescore#7947, part 2
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.

5 participants