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.

Sign up for GitHub

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

Jump to bottom

Fix: Issue with scaling of dynamics #18860 #18920

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Fix: Issue with scaling of dynamics #18860 #18920

merged 1 commit into from
Sep 26, 2023

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Aug 4, 2023

Resolves: #18860

@cbjeukendrup
Copy link
Contributor

Interesting solution; especially a lot simpler than what I did in #15772. However, it might be preferable to specify the style conversion function in the call to buildPropertyItem, to keep it nearby the conversion function for setting the property.

@mike-spa
Copy link
Contributor Author

mike-spa commented Aug 4, 2023

Interesting solution; especially a lot simpler than what I did in #15772. However, it might be preferable to specify the style conversion function in the call to buildPropertyItem, to keep it nearby the conversion function for setting the property.

Well in this case the style conversion function is just the same as the property conversion: it's just *100 or /100. In fact I wondered if there's a way to reuse the same conversion function that we're passing to builPropertyItem, but I couldn't come up with a sensible way

@mike-spa mike-spa force-pushed the fix#18860Issuewithscalingofdynamics branch from b77d398 to 3766268 Compare August 16, 2023 09:20
@mike-spa mike-spa force-pushed the fix#18860Issuewithscalingofdynamics branch from 3766268 to acbdfb6 Compare September 19, 2023 10:13
@cbjeukendrup
Copy link
Contributor

There are some other properties that have similar issues with similar fixes:

  • the MAG property in FretDiagramSettingsModel
  • the SCALE property in TimeSignatureSettingsModel

Maybe it'd be nice to add those to this PR.

@mike-spa mike-spa force-pushed the fix#18860Issuewithscalingofdynamics branch from acbdfb6 to ea304e8 Compare September 19, 2023 16:42
@mike-spa
Copy link
Contributor Author

@cbjeukendrup all done!

cbjeukendrup
cbjeukendrup reviewed Sep 20, 2023
View reviewed changes
src/inspector/models/notation/timesignatures/timesignaturesettingsmodel.cpp Outdated
},
[this](const mu::engraving::Sid sid, const QVariant& newValue) {
updateStyleValue(sid, QSizeF(newValue.toDouble() / 100, m_verticalScale->value().toDouble() / 100));
emit requestReloadPropertyItems();
});

m_verticalScale = buildPropertyItem(mu::engraving::Pid::SCALE, [this](const mu::engraving::Pid pid, const QVariant& newValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the vertical component of the property needs this callback too

Copy link
Contributor Author

@mike-spa mike-spa Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

also done 👍

@mike-spa mike-spa force-pushed the fix#18860Issuewithscalingofdynamics branch from ea304e8 to 4ff3646 Compare September 20, 2023 06:18
@mike-spa mike-spa force-pushed the fix#18860Issuewithscalingofdynamics branch from 4ff3646 to 3661584 Compare September 25, 2023 12:46
@cbjeukendrup cbjeukendrup force-pushed the fix#18860Issuewithscalingofdynamics branch from 3661584 to e6457fb Compare September 26, 2023 01:53
@mike-spa mike-spa merged commit a9ba583 into musescore:master Sep 26, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oktophonie oktophonie oktophonie approved these changes

@cbjeukendrup cbjeukendrup cbjeukendrup approved these changes

@RomanPudashkin RomanPudashkin Awaiting requested review from RomanPudashkin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with scaling of dynamics
3 participants
@mike-spa @cbjeukendrup @oktophonie