-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 #307623: Y offsets change when stave space is changed #6315
Conversation
Looking forward to testing this, thanks for following up! I see how this address the undo issue nicely, but right now I’m not understanding how this actually fixes the problem at hand, does it not still result in calls to Element::spatiumChanged just as before? |
Oh wait, I think I know - the issue was only with offsets of styled elements, probably we were updating the offset before the style. |
I guess other usages of |
6e29ff8
to
b2b2e6f
Compare
This PR has been updated to address @dmitrio95's review. Deleting that one line allowed me to remove the entire switch statement around it. It is not necessary to guard against |
b2b2e6f
to
b7a5472
Compare
My latest push adds back a comment that I had removed by mistake when removing the switch statement. Or rather, it leaves the comment untouched. |
Just tested, seems to work as advertised, nice job! In my investigation yesterday, though, I found a few things we still aren't scaling properly. Line thickness, for instance (whether styled or not). Also offsets on slurs & ties. I'm assuming this means a few element-specific spatiumChanged() functions are missing some stuff. And like I said, a few are missing corresponding locaSpatiumChanged() functions (or have them but they don't do the job properly. I'd recommend merging this as is but then maybe addressing those other more specific issues in a separate PR (which I'm playing around with now). |
Fixes a few cases where scaling still is not performed properly because an individual element is not scaling itself properly or its parent is scaling it multiple times (ties). In the case of pedal, the scaling problem happens on add, because the line width was never listed as a styled property.
See #6318. As mentioned, that PR can either be merged after this one, or the commit can be incorporated into this PR, it doesn't matter to me). I didn't test every single possibility to see what else might be missing, but nothing else I tried had problems. Except, as mentioned, with respect to staff as opposed to score scaling. Several other elements are not handling localSpatiumChanged properly (or at all). I will let someone else worry about that, though. |
Fix #307623: Y offsets change when stave space is changed
Resolves: https://musescore.org/node/307623.
This reverts #5699 and calls
Score::spatiumChanged()
from withinChangeStyleVal::flip()
. Not only does it now do the right thing on undo, but it solves the rest of the problem that was introduced recently.