Fix for Mixer volume percentage labels are off by a factor of 100

[DigArtRoks]

(#2340)

Patch the default context menu and take into account variable m_displayConversion to multiply/divide by 100.

Reset action: update the text with initValue multiplied by 100 if required.
Copy action: update the text with value multiplied by 100 if required. Replace the default copy slot by one
which also multiplies by 100 if required.
Paste action: Replace the default paste slot which divides by 100 if required.

Faders in FxMixer show now the correct context. Copying and pasting values between faders or even volume knobs
in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.

[DigArtRoks]

[LmmsBot]

🤖 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

• Linux (AppImage): lmms-1.2.2.691-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.2.2.691-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.2.2.691-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.2.2-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.2.2-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.2.2.691-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8454-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.691-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8454?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://8455-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.691-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8455?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8453-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.691-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8453?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/w1xl5e7tjpxe4y0r/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/34828615"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1x9lqgcneyii9tdp/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/34828615"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://8451-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.691-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8451?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "3910b235292583ff10dd6c9cbac21119ea9f29f6"}

[IanCaio]

IMHO this PR fixes a problem but uses the wrong approach. I'll point some of the things on the approach that concern me and give an alternative in the end:

• Magical numbers are used to access the actions on the patch method. Magical because they are not directly related to the ordering of the actions, meaning that if we change anything in the ordering of the default actions in AutomatableModelView::addDefaultActions we would break this code.
• I'm not sure it's any advantage to store the values as percentages in the clipboard. As far as we display them as percentages, what is improved by storing them as the number in percentages instead of float values equivalent to the percentages? It's even somewhat wrong because we don't store the unit in the clipboard, only the value. So if we copy 50%, for example, we have 50 on the clipboard, but we can't know it's a percentage. If we paste it in another model view that accepts floats but have another unit different than a percentage we would paste the wrong value. Mathematically 0.5 is the real float value of 50%, so I believe that's what should be stored.
• I don't think patching the default actions is better than fixing the way it's built. It looks somewhat odd to have a patch inside the code, like we didn't fix the "faulty" method but wrote another method to fix it.

So my alternative solution that would address those issues is actually changing the AutomatableModelView class instead. That's the one that creates the context menu actions in the first place. When percentages are used, as in the case of Fader, a unit of % is set to the model view so it can be written after the value on the hint texts. You could simply change AutomatableModelView::addDefaultActions to check if the unit being used is %, and if it's, multiply the displayed value by 100. Think you would save lines of code and get rid of those issues at the same time!

[DigArtRoks]

Thanks for your remarks. To be honest, I've considered them all before creating the pull request. I explored several paths and came out on this one as the least impact on the codebase.

The real problem is that parameters like the volume or panning in the track are expressed as %, but their underlying model uses 0 - 200 for volume and -100 to 100 for pan. The faders in the Fx mixer though use 0.0 to 2.0, but require *100 when they need to be displayed as %.

Today the AutomatableModelView just displays the float as is and adds the % sign for the volume and pan controls. However this fails for the FxFaders (see the original issue #2340). So that explains the fixing of the reset and copy labels in the context menu. I inserted the storing *100 into the clipboard for consistency reasons. E.g. you copy the fader setting (displayed with this fix as e.g. 90%). When rightclicking on a volume control, it displays overthere as paste 90% instead of 0.9%. Vice versa for the paste operation on faders.

As the Fader class had already the variable m_displayConversion to fix the *100 conversion for the floating hint text and for numerical entering the value, I thought to keep this thing further local in the Fader class.

Keeping it local required the magical numbers. I agree it is not clean, but not too bad either IMHO. I've seen worse in other projects. As long as the menu stays static it is fine. Restructuring the menu requires once an update of the indices. I could not easily figure out how to get back the actions to be patched. Well actually I can, but that requires storing pointers in the AutomatableModelView for the reset/copy/paste actions, so that they could be picked up. But that seemed to me a bridge too far.

Your suggestion is ok if all other parameters like Volume and Pan in a track also would store their value in the 0.0 to 1.0 range. What would you suggest to get around this? Would it be a good solution to move the m_displayConversion variable from the Fader class into the AutomatableModelView? I dwelled on it, but rejected it as it impacts all models for a problem in the Fader class only (AFAIK).

Looking forward to your opinion.

[IanCaio]

You're completely right, I didn't know the problem was this deep and other model-views that are displayed as percentages used the whole percentage numbers in their models instead of floats. I didn't mean your solution was bad, but if the AutomatableModelView fix was possible it would be cleaner and easier to maintain. As you said that would require we to change the volume and pan models as well, probably master volume too?

I still think that's a mess we will have to deal with at some point: defining whether we will store percentages as whole percentages or their equivalent floats and changing every required model to do it. Since the FxMixer is the only one that uses floats maybe it'll be easier to change it to use percentages instead of changing everything else to use floats, though personally I think floats were more appropriate.

I'll dig more into the code trying to come up with ideas or at least figure out how complicated it would be to change the models. If others agree about your approach as a quick fix I'm cool with it, but would still suggest that we add the deeper fixes to the models to our milestone and document the changes from this PR so we can reverse it when the models are fixed.

[DigArtRoks]

I created a new PR #5661 with an alternative implementation. I close this one as it was not that well.