-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix part rename not persisting for potential excerpts #31658
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
Conversation
Test that renaming a 'potential' excerpt (not yet in master's excerpts list) persists after save/reload. This test fails without the corresponding fix because: - Potential excerpts are not in master->excerpts() - mscsaver only saves excerpts in the master's list - So the renamed excerpt is not saved to disk
When renaming a 'potential' excerpt (shown in Parts dialog but not yet created), ChangeExcerptTitle now auto-adds it to master's excerpts list if not present. This ensures the excerpt gets saved to disk, fixing the bug where part names changed in the Parts dialog weren't saved if the part hadn't been opened as a tab. Fixes musescore#31656
The previous fix added the auto-add logic to ChangeExcerptTitle, but undoSetName() was returning early when score() is null (which happens for potential excerpts), never reaching ChangeExcerptTitle. Use m_excerpt->masterScore() instead of score() to get the master, allowing the rename to proceed and trigger ChangeExcerptTitle.
cbjeukendrup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal, because initialising an excerpt is an expensive operation, and it is wasteful to populate and store the entire excerptScore while we only really need to store the excerpt's name.
Instead, the solution to this and some other issues, should be to move the concept of "potential excerpts" to the engraving/dom level, instead of having it at the MasterNotation/PartListModel level.
Potential excerpts should just be Excerpt but with no Score yet. Such excerpts should be created for each instrument when the master score is initially created, and when new instruments are added.
They should be stored in the MSCZ file, even if no properties have been modified compared to the defaults. However, the file format for potential excerpts might need to be slightly different because of the absence of a Score. It should only store a name and a list of Parts.
This is a somewhat big refactor, but I think it's worth it. It would also result in the ability to delete potential excerpts without having them appear back next time you open the parts dialog, and it would make it easier to fix the problems with the reset functionality.
Let us know if you're happy to look into this!
|
Is it acceptable if older MS versions simply ignore or skip lightweight excerpts? The excerpt would be auto-regenerated from parts, losing only the custom name. If yes, what is the preferred approach?
The current PR approach uses full excerpts and is always backward compatible, but produces bigger files when part is not yet needed. Another option would be a breaking change. Older versions would fail to open files with lightweight excerpts, which means refusing to open any 4.7+ file. |
I investigated a little bit this approach, and it appears it's not feasible. From what I can see, older MS versions process each file in Excerpts/*.mscx as follows:
It looks like the old code doesn't have a "discard invalid excerpt" path - it either succeeds completely or fails the entire file load. |
|
We are prohibiting opening 4.7 files in older versions (like we did with all 4.x releases), so we don't need to worry about backwards compatibility. (Officially, that's not entirely in line with the SemVer philosophy, but we have decided to allow ourselves to do this anyway because MuseScore is still so much under development.) |
|
@cbjeukendrup I have a branch with the approach you described. Is it better to close this PR and open a new one from a clean master, or do you prefer that I revert the commits here and continue the discussion and review in this PR? |
|
A new fresh PR would be great! (UX on GitHub doesn't really improve when threads become longer...) |
|
Closing this PR to continue with the suggested implementation in #31738 |
Resolves: #31656
When renaming a part in the Parts dialog, the new name wasn't persisted to the .mscz file unless the part had been opened as a tab first.
Root cause: "Potential" excerpts shown in the Parts dialog are not in
masterScore()->excerpts()until explicitly created. Sincemscsaveronly saves excerpts in the master's list, renaming a potential excerpt had no effect.Solution:
ChangeExcerptTitlenow checks if the excerpt is in the master's list before renaming. If not, it callsinitAndAddExcerpt()to add it, ensuring it gets saved.