-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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 for Mixer volume percentage labels are off by a factor of 100 #5661
Conversation
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
Windows
🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8671-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.694-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8671?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8669-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.694-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8669?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8670-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.694-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8670?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/a6h34vqc51716cd5/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35166231"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/v2nmnywspx7buto5/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35166231"}]}, "commit_sha": "083f7954a05b227af8e91aab6ba8fb6cbc9ee9f6"} |
It generally looks good. Let's wait for other developer's feedback. |
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.
Seconding PhysSong's comments, looks like a reasonably unintrusive fix otherwise.
…MMS#2340) Add m_conversionFactor to the AutomatableModelView. This factor will be applied to the model->value when displaying it in the contextmenu of the control for the reset and copy actions. The factor will be applied when copying the value to the clipboard. When pasting from the clipboard, the value will be divided by the factor. Remove the model->displayValue() calls when updating the reset/copy/paste action text labels as this gives e.g. in the Equalizer the wrong action text for the Frequency knobs. In the Fader class, remove the m_displayConversion variable but rather use the new m_conversionFactor variable. Rewire the setDisplayConversion() function to set the m_conversionFactor to 1.0 or 100.0. Faders in FxMixer show now the correct context menu. Copying and pasting values between faders or even volume knobs in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.
c517fc5
to
7822fc0
Compare
Additional commit to take into account the review comments concerning coding style of PhysSong. |
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.
LGTM. FYI, the changes to AutomatableModelView.cpp
will conflict with #5627.
Fixes conflict introduced by LMMS#5661 ("Fix for Mixer volume percentage labels are off by a factor of 100") on src/gui/AutomatableModelView.cpp.
Fixes conflict introduced by LMMS#5661 ("Fix for Mixer volume percentage labels are off by a factor of 100") on src/gui/AutomatableModelView.cpp. Note: This merge was force-pushed after a hard reset, because there were some issues with submodules being included on the last time I pushed the merge. That new merge is an attempt to fix that.
…MMS#5661) Add m_conversionFactor to the AutomatableModelView. This factor will be applied to the model->value when displaying it in the contextmenu of the control for the reset and copy actions. The factor will be applied when copying the value to the clipboard. When pasting from the clipboard, the value will be divided by the factor. Remove the model->displayValue() calls when updating the reset/copy/paste action text labels as this gives e.g. in the Equalizer the wrong action text for the Frequency knobs. In the Fader class, remove the m_displayConversion variable but rather use the new m_conversionFactor variable. Rewire the setDisplayConversion() function to set the m_conversionFactor to 1.0 or 100.0. Faders in FxMixer show now the correct context menu. Copying and pasting values between faders or even volume knobs in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.
(#2340)
Add m_conversionFactor to the AutomatableModelView. This factor will be applied to the model->value when displaying
it in the contextmenu of the control for the reset and copy actions. The factor will be applied when copying the value to
the clipboard. When pasting from the clipboard, the value will be divided by the factor.
Remove the model->displayValue() calls when updating the reset/copy/paste action text labels as this gives e.g. in the
Equalizer the wrong action text for the Frequency knobs.
In the Fader class, remove the m_displayConversion variable but rather use the new m_conversionFactor variable.
Rewire the setDisplayConversion() function to set the m_conversionFactor to 1.0 or 100.0.
Faders in FxMixer show now the correct context menu. Copying and pasting values between faders or even volume knobs
in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.
Replace PR #5654