Conversation
miiizen
force-pushed
the
hookHeightMigration
branch
from
45cfd3c to
1d30c63
Compare
mike-spa
left a comment
mike-spa
left a comment
There was a problem hiding this comment.
Would you mind moving the ornament cui note fix to a separate PR? I think the hook sign fix could potentially go in 4.7, but the cur note fix certainly not
| } | ||
| } | ||
|
|
||
| compat::CompatUtils::resetHookHeightSign(b); |
There was a problem hiding this comment.
Seems like there's code before the function call that does the same thing as the function?
| <HairPin> | ||
| <subtype>0</subtype> | ||
| <beginHookHeight>-1.9</beginHookHeight> | ||
| <endHookHeight>-1.9</endHookHeight> |
There was a problem hiding this comment.
These tests changes look quite weird: why are we suddenly adding hookHeight tags to hairpins?
There was a problem hiding this comment.
We've changed the hookHeight value by flipping it so it no longer matches the default. Hairpin's hook heights look like this by default pre 4.6
| } | ||
| volta->setOffset(PointF()); // ignore offsets | ||
| volta->setAutoplace(true); | ||
| CompatUtils::resetHookHeightSign(volta); |
There was a problem hiding this comment.
It makes me a bit uncomfortable to have to add this compat function to 20 different places across all the read module. Could we not move this to EngravingCompat::doPreLayoutCompat? It will perhaps slightly add to the reading time as we need to navigate and find these spanners, but it's well worth it IMO if it saves us from calling it so many times
There was a problem hiding this comment.
We could, but I'm keen to put as much compatibility code which doesn't require score context in the file reading code as possible. Otherwise we need to add the same compatibility fix for palette items in PaletteCompat, which is something we always forget to do and I'd prefer to avoid where possible. This was part of the motivation behind versioning the palette files.
I agree this is a bit messy though.
There was a problem hiding this comment.
Ah good point about the palettes, then yeah I think it's fine 👍
miiizen
force-pushed
the
hookHeightMigration
branch
from
1d30c63 to
0fc2d59
Compare
|
Tested and approved on Ubuntu 24.04.4 LTS. |
3 participants
This PR migrates hook height signs (<4.6) on file read to avoid an additional score layout.
It also fixes a bug in ornament cue note layout order covered up by this second layout.