Project migration refactor - #34873
Conversation
These migrations must happen to every <4.0 score, so they should go in our usual score compatibility files. These migrations were skipped for vtests, leading to scores looking different in the test and in the application.
It is already called in `MscLoader::loadMscz`, so no need to call it in individual reader implementations.
It serves no purpose and meta tags are not the place for `mscVersion`.
|
@cbjeukendrup sorry - I only remembered #32847 halfway through this. I've cherry picked a couple of the most relevant commits. Please feel free to review! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change moves legacy engraving resets from project migration into compatibility processing. Older scores now reset element positions and cross-staff beams through Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The compatibility migration introduces no verified merge-blocking behavior change. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cbjeukendrup
left a comment
cbjeukendrup
left a comment
There was a problem hiding this comment.
Thanks for notifying me! Yes, this looks good to me. One small tidiness comment.
The other two commits from #32847 are probably also still worth porting, but that can be done in a different PR.
|
|
||
| int mscVersion = score->mscVersion(); | ||
|
|
||
| if (score->mscVersion() < 300) { |
There was a problem hiding this comment.
| if (score->mscVersion() < 300) { | |
| if (mscVersion < 300) { |
(same for 206)
Also, maybe it is nice to keep the version checks in some order, like oldest migrations first or last.
| double doubleBarDistance = styleAbsolute(mu::engraving::Sid::doubleBarDistance); | ||
| doubleBarDistance -= styleAbsolute(mu::engraving::Sid::doubleBarWidth); | ||
| set(Sid::doubleBarDistance, doubleBarDistance / sp); | ||
| double endBarDistance = styleAbsolute(mu::engraving::Sid::endBarDistance); | ||
| endBarDistance -= (styleAbsolute(mu::engraving::Sid::barWidth) + styleAbsolute(Sid::endBarWidth)) / 2; | ||
| set(Sid::endBarDistance, endBarDistance / sp); | ||
| double repeatBarlineDotSeparation = styleAbsolute(mu::engraving::Sid::repeatBarlineDotSeparation); | ||
| static std::shared_ptr<IEngravingFontsProvider> engravingFonts | ||
| = muse::modularity::globalIoc()->resolve<IEngravingFontsProvider>("engraving"); | ||
| double dotWidth = engravingFonts->fontByName(value(Sid::musicalSymbolFont).value<String>().toStdString())->width( | ||
| mu::engraving::SymId::repeatDot, 1.0); | ||
| repeatBarlineDotSeparation -= (styleAbsolute(mu::engraving::Sid::barWidth) + dotWidth) / 2; | ||
| set(Sid::repeatBarlineDotSeparation, repeatBarlineDotSeparation / sp); |
There was a problem hiding this comment.
These styles are re-calculated relative to their last value every time this function is called. I believe the function may be called multiple times (e.g. for 3.x scores, it would be called once while reading the legacy-style-defaults-vX.mss file, and then again while reading the score style - or more if there are user styles, I guess), so the values would accumulate. Before, these were only calculated and overwritten once during project migration.
Not sure what the best solution would be, maybe moving it out of the MStyle::read, to after the entire score is read (e.g. EngravingCompat)? Or adding some sort of guard?
| set(Sid::tiePlacementChord, TiePlacement::INSIDE); | ||
| } | ||
|
|
||
| if (mscVersion < 400 && !MScore::testMode) { |
There was a problem hiding this comment.
applyCompatStyleVals is being called by ReadStyleHook::readStyleTag (in read302 and later), but I believe Read206 and Read114 read style through their own functions so they may not get these migrations (the function is called when reading the legacy-style-defaults-vX.mss file, so effect may vary depending on whether the styles were explicitly set in the score or not?)
|
|
||
| bool EngravingCompat::resetAllElementsPositions(MasterScore* score) | ||
| { | ||
| for (Score* score : score->scoreList()) { |
There was a problem hiding this comment.
Nitpick but may be worth changing the name of this scope's score so it's not the same as the function parameter (same in line 454).
| Transaction& tx = score->transactionManager()->currentOrDummyTransaction(); | ||
| Reset::resetAllPositions(tx, score); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
May be intended, but this doesn't seem to ever return false (same in resetAllCrossBeams). Maybe needRelayout can be explicitly set i doPostLayoutCompatIfNeeded instead.
5 participants
These migrations must happen to every <4.0 score, so they should go in our usual score compatibility files. These migrations were skipped for vtests, leading to scores looking different in the test and in the application (which was causing a lot of headaches when working on offsets migrations).
The vtests should now match what we see when scores are opened in MuseScore.