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 #138266: Merge matching rests in multiple voices #5896

Merged
merged 1 commit into from
Jun 6, 2020

Conversation

tobik
Copy link
Contributor

@tobik tobik commented Apr 3, 2020

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

Marc Sabatella thankfully prepared this functionality 5 years ago. I activated his code and added a staff property "Merge matching rests" which enables the functionality.

Merging rests are common to closed SATB scores. The other way to achieve this is to make all matching voice 2 rests invisible. But this is a error prone and time consuming procedure which is now automated.

  • 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

@tobik tobik mentioned this pull request Apr 3, 2020
8 tasks
@Jojo-Schmitz
Copy link
Contributor

I wonder whether this new setting should even be the default for some templates, like the Closed Score SATB ones. Maybe the 'open' SATB ones too, but I'm not so sure there.

@tobik
Copy link
Contributor Author

tobik commented Apr 3, 2020

Do you have any idea why this check is failing?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Apr 3, 2020

Yes, see the most recently merged PR. Then push again ;-)

@Harmoniker1
Copy link
Contributor

Use git rebase master to merge recent master updates, in this way there'll be no merge commits.

@tobik
Copy link
Contributor Author

tobik commented Apr 3, 2020

I probably ruined my git history by a reset!? And to be honest, I have no clue how to repair it...
A git pull --rebase musescore master got me a incoming commit from origin 138266-mergerests.
A git pull --rebase origin 138266-mergrests dropped the recent commits from musescore master.
Finally I did git pull origin 138266, git pull --rebase musescore master, git push origin 138266 and now there are again lots of unwanted commits in my PR.

I would appreciate some help

@AntonioBL
Copy link
Contributor

@tobik : I am definitely not a git expert, but you can try to do something like what is explained here:
https://stackoverflow.com/a/46921732
maybe you should create a backup branch in case there is something which goes wrong.
And remember to check the branch history via "git log" before pushing the branch (and you most probably should use "git push --force origin 138266-mergerests" when pushing your branch when everything is ok).
Ciao,
ABL

@tobik
Copy link
Contributor Author

tobik commented Apr 4, 2020

I finally managed to get the PR clean. Thank you all!

@tobik
Copy link
Contributor Author

tobik commented Apr 4, 2020

I wonder whether this new setting should even be the default for some templates, like the Closed Score SATB ones. Maybe the 'open' SATB ones too, but I'm not so sure there.

Should I commit changed templates?

@Jojo-Schmitz
Copy link
Contributor

Maybe in a 2nd commit. I'd like input from others in this, so maybe better also discuss it in the issue.

@anatoly-os
Copy link
Contributor

I like the changes and would apply them to 3.x. Could you please create a PR based on 3.x, not master?

@tobik tobik changed the base branch from master to 3.x June 3, 2020 19:26
@anatoly-os anatoly-os merged commit c13b580 into musescore:3.x Jun 6, 2020
@tobik tobik deleted the 138266-mergerests branch June 7, 2020 19:38
anatoly-os added a commit that referenced this pull request Aug 20, 2020
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