Skip to content

Conversation

@skalra4
Copy link
Contributor

@skalra4 skalra4 commented Apr 14, 2025

Resolves: #26233

Case for pickup measures with linked staves. If you switch the order of ascending rests, there may be an empty segment due to the longer rest/note, which was not being copied to the linked staff, which when created used the default staff configuration, causing a rest or note to be left behind, seemingly "creating a duplicate" and corrupting the score.

  • 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
Copy link
Member

So this PR fixes the issue by deleting those default rests during the clone process. But I was wondering: maybe those default rests should never have been created, if the first thing that will happen to them is that they are deleted. I suspect they are created when inserting the staff into the score, so before cloneStaff. Maybe you could look into that.

@skalra4
Copy link
Contributor Author

skalra4 commented Apr 18, 2025

I understand that. I have a question: Would you like this to be for every case where a staff member is cloned AND linked? I think the problem is that the functions being used are also what create default staffs, and I'm unsure how to introduce the condition whether or not to remove rests upon default initialization based on the actual time signature. This is my reasoning behind why I did it the way I did. If you have a better suggestion, I can try to implement it, but I can also try my best to do it on my own. However, I did consider this option from your original suggestion, but I couldn't figure out a way to compartmentalize and keep the two scenarios separate.

@cbjeukendrup
Copy link
Member

Score::undoInsertStaff has a createRests parameter, that controls whether default rests are created. It is true by default. Therefore, NotationParts::insertStaff, which is called via NotationParts::doAppendStaff, will always create rests. I think the solution would be to add a createRests parameter to NotationParts::insertStaff and NotationParts::doAppendStaff, and pass that on to undoInsertStaff. Then in NotationParts::appendLinkedStaff, where doAppendStaff is called, pass false to that parameter.

@cbjeukendrup
Copy link
Member

@skalra4 Something went wrong while rebasing. It looks like you first rebased, then pulled the unrebased version from your fork on GitHub, and then pushed. Instead, you should rebase, and then force-push. So:

git pull --rebase upstream master
git push -f

@skalra4 skalra4 force-pushed the 26233-fix_cloneStaff branch 2 times, most recently from c99db42 to 50ea827 Compare April 23, 2025 16:25
@skalra4
Copy link
Contributor Author

skalra4 commented Apr 23, 2025

I apologize for the messy working tree. Git still gets me, especially on big projects with many branches and changes.

@cbjeukendrup
Copy link
Member

No worries. It looks correct now!

@skalra4
Copy link
Contributor Author

skalra4 commented Apr 23, 2025

Great. Thanks so much for all the help. I appreciate and like the MuseScore dev community a lot!

@cbjeukendrup cbjeukendrup self-requested a review April 25, 2025 23:47
@cbjeukendrup cbjeukendrup force-pushed the 26233-fix_cloneStaff branch from 4704803 to 6c36322 Compare August 8, 2025 02:11
@cbjeukendrup
Copy link
Member

Sorry for the long silence around this PR! I've rebased it, and now that I was looking at it again, I decided to revert the 'old' fix, now that we have the new one with setting createRests to false; that should make the old fix unnecessary, and the old fix seems a bit risky to me so I'd rather not keep it for "just in case" reasons.

skalra4 and others added 7 commits August 8, 2025 20:50
Co-authored-by: Casper Jeukendrup <48658420+cbjeukendrup@users.noreply.github.com>
… staff makes score corrupted"

This reverts commit 74a8d85; that fix is possibly incorrect, and not necessary now that we are no longer creating those problematic rests anymore in the first place, so no need to remove them.
@cbjeukendrup cbjeukendrup force-pushed the 26233-fix_cloneStaff branch from 6c36322 to 7407cd7 Compare August 8, 2025 19:09
@zacjansheski
Copy link
Contributor

Tested and approved
#26233 FIXED

@cbjeukendrup cbjeukendrup merged commit c20fec8 into musescore:master Aug 8, 2025
13 checks passed
@skalra4
Copy link
Contributor Author

skalra4 commented Aug 13, 2025

Thank you so much for the learning experience. I want to contribute more to MuseScore in the future when I have the time!

@skalra4 skalra4 deleted the 26233-fix_cloneStaff branch August 13, 2025 22:55
@cbjeukendrup
Copy link
Member

You're always welcome!

@ghost ghost changed the title Fix #26233: Combination of pickup measure and linked staff makes scor… Combination of pickup measure and linked staff makes score corrupted Aug 14, 2025
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.

Combination of pickup measure and linked staff makes score corrupted

3 participants