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

Add default setting for Merge matching rests in Staff properties. #25134

Merged

Conversation

rpatters1
Copy link
Contributor

@rpatters1 rpatters1 commented Oct 11, 2024

Resolves: #24983

Implements a new style setting for the default value of staves' Merge matching rests setting.

Screen Shot 2024-10-13 at 6 33 09 AM

The setting in Staff properties is now an AutoOnOff:

Screen Shot 2024-10-13 at 6 53 55 AM
  • Auto takes the setting from the Style setting
  • On force it on (same as checking the previous setting)
  • Off forces it off

Since the default value of the new Style setting is unchecked, imports of files created before this Style setting was available remain visually identical. If the staves were unchecked, they are converted to Auto. If the staves were checked, they are converted to On.

  • 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)

@rpatters1 rpatters1 marked this pull request as draft October 11, 2024 15:09
@rpatters1
Copy link
Contributor Author

I am changing this to Draft status, because I realized that file imports do not pick up the default. At a minimum, file imports from Midi files and MusicXML files should pick up the default. I can't seem to find a central add staff place that seems safe to add this where all paths would pick it up.

@rpatters1 rpatters1 marked this pull request as ready for review October 11, 2024 19:17
@rpatters1
Copy link
Contributor Author

rpatters1 commented Oct 11, 2024

I added initialization based on the setting to Factory::createStaff. This picks up all the imports that I tried without any need to modify them. The only situation where there was a slight issue is calling staff->init(sourceStaff). When the source staff is from the master score and the new staff is in an excerpt, the value put in by createStaff could be overwritten with the wrong value. Rather than risk a change to init, I simply overrode it with the correct value again in the 2 spots I identified with this problem.

Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

In this PR, the new style setting is only used to determine the value of "merge matching rests" for newly added staves, and not existing staves. I think that limits the usefulness of this option.

Personally I think it would be preferable to turn the existing staff property into an Auto / On / Off property, where Auto means "follow the score-wide setting". @oktophonie do you think this too?

This could be implemented in two ways:

  • Change the type of Staff::mergeMatchingRests from bool to AutoOnOff
  • Turn it into a property that is linked to the score-wide style setting; that means that the type stays bool, but modifying the value should be done via getProperty/setProperty, or rather undoChangeProperty to make it undoable; PropertyFlags are used to determine whether this staff should follow the global style setting (PropertyFlag::STYLED means follow the style; PropertyFlag::UNSTYLED means the property is overridden for this staff); use EngravingObject::initElementStyle in the constructor of Staff to set up the "connection".

src/engraving/dom/factory.cpp Outdated Show resolved Hide resolved
@rpatters1
Copy link
Contributor Author

AutoOnOff sounds like the way to go to me.

@rpatters1
Copy link
Contributor Author

AutoOnOff is a much better solution. Thank you for the suggestion. I think it's ready to be reviewed in this format, barring one of the checks failing.

src/engraving/style/styledef.h Outdated Show resolved Hide resolved
src/notation/internal/masternotation.cpp Outdated Show resolved Hide resolved
src/engraving/rw/write/twrite.cpp Outdated Show resolved Hide resolved
src/engraving/rw/read400/tread.cpp Outdated Show resolved Hide resolved
src/engraving/rw/read410/tread.cpp Outdated Show resolved Hide resolved
src/notation/view/widgets/editstaff.ui Outdated Show resolved Hide resolved
src/notation/view/widgets/editstaff.ui Show resolved Hide resolved
@rpatters1 rpatters1 force-pushed the default-merge-matching-rests-style branch from 8cf80cc to 5fa306a Compare October 16, 2024 14:56
@rpatters1
Copy link
Contributor Author

I rebased to the latest master and now this PR is getting a lot of test errors that do not seem related to my PR. Any idea what's going on?

Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

Final comments, I think!

src/engraving/rw/read400/tread.cpp Outdated Show resolved Hide resolved
src/engraving/rw/read410/tread.cpp Outdated Show resolved Hide resolved
src/notation/view/widgets/editstaff.cpp Outdated Show resolved Hide resolved
src/engraving/tests/scoreutils_tests.cpp Outdated Show resolved Hide resolved
src/engraving/tests/scoreutils_tests.cpp Outdated Show resolved Hide resolved
src/notation/internal/masternotation.cpp Outdated Show resolved Hide resolved
@rpatters1
Copy link
Contributor Author

Apparently, changing constants.h to 450 will entail a lot of additional changes in test files. That seems like it is significant scope creep for this PR. Can I leave it be for now?

@rpatters1
Copy link
Contributor Author

I backed out the non-comment change to contants.h. That should allow the run_tests GitHub action step to complete without throwing an error. It will mean that the dev branch will not correctly read the settings for AutoOnOff::OFF in files saved with the dev branch app until constants.h is updated to 450.

If you think the update should be part of the PR, I will need some guidance on what needs to change in the test files so that the run_tests don't fail.

Thank you for your due diligence. I am really impressed and grateful at the care you take.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Oct 16, 2024

Changing MSCVERSION etc. indeed requires basically all test scores to get changed, I'd rather leave that to the core team ;-)
Or to a script, like for f in $(find . -name '*.mscx' | xargs grep -l 'museScore version="4.40"'); do sed -i 's/museScore version="4.40"/museScore version="4.50"/g' $f; done (attention: eintirely untested)

Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me from a technical point of view!

@zacjansheski
Copy link
Contributor

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#24983 FIXED

@RomanPudashkin RomanPudashkin merged commit 24de343 into musescore:master Oct 18, 2024
11 checks passed
@rpatters1 rpatters1 deleted the default-merge-matching-rests-style branch October 19, 2024 13:56
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.

‘Merge matching rests’ default value as a staff style property
5 participants