lmms
lmms copied to clipboard
LMMS
Fixes #6398: Colour of Master bus on FX Mixer retains when switching project
:robot: Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! :tophat:
macOS
Windows
- Windows 32-bit:
lmms-1.3.0-alpha-msvc2017-win32.exe(build link) - Windows 64-bit:
lmms-1.3.0-alpha-msvc2017-win64.exe(build link)
:robot:
{"platform_name_to_artifacts": {"macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/7a2d0730-7048-4264-bea2-3e134d323f53/artifacts/0/lmms-1.3.0-alpha.1.221+g8fe731fc9-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17739?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://ci.appveyor.com/api/buildjobs/leo0sa49xlkmwv2c/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44118529"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/kcwdgvxx7cnjafo3/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44118529"}]}, "commit_sha": "6d8d0a587cccd58a02137f95e3193032f92c4db9"}
Optional should be available by now though, I believe we already bumped to the required C++ version.
Optional should be available by now though, I believe we already bumped to the required C++ version.
True, but the we have redundant information in std::optional and QColor about validity.
Optional should be available by now though, I believe we already bumped to the required C++ version.
True, but the we have redundant information in
std::optionalandQColorabout validity.
I don't think two validity information represent the same thing. Invalid QColor may mean not only we haven't set a value yet, but also we set it to an invalid value manually for some reason.
I think they are equivalent in our current code. Do you see any use case in our code where an invalid MixerChannel::m_color and a not-set MixerChannel::m_color should be handled differently?
Do you see any use case in our code where an invalid
MixerChannel::m_colorand a not-setMixerChannel::m_colorshould be handled differently?
No for now, but if we allow setting colors by name, for example, m_color might be set to an invalid color. In that case, however, it might be good to treat invalid colors as not set.
In my opinion, a single (possibly invalid) member of type QColor doesn't imply that having no color is an acceptable state. An optional makes it clear that we expect there to be situations where there's no color, and it's not necessarily an error.
In my opinion, a single (possibly invalid) member of type QColor doesn't imply that having no color is an acceptable state. An optional makes it clear that we expect there to be situations where there's no color, and it's not necessarily an error.
Look like the team is for using std::optional then. @spechtstatt do you think you could fix that? It's the last comment of this PR which has to be resolved. Alternatively, someone else could code it for you.
I feel like the switch to std::optional is a bit out of scope for what is otherwise a tiny change. There's a bunch of other modernisation to be done across the codebase, and I think it would fit better there. If the author is happy to add it here, then I won't object, but I think it's overcomplicating things to require it here.
Well - I am somehow with @DomClark in that it may be too much for this tiny bug fix but I tried anyway :-)
Not sure if I made it right though. Especially when calling the color selection dialog.
@spechtstatt The use of std::optional looks correct, but shouldn't it replace m_hasColor?
Oh sorry.
I switched my Computer recently and I probably forgot to change it in the qt creator settings.
@spechtstatt The use of
std::optionallooks correct, but shouldn't it replacem_hasColor?
@spechtstatt Could you address that with the indentation fixes(revert to tabs)?
@allejok96: you mean the commit message? Yes: I was repeating the issue title again like in the first commit which was probably not the best idea.
@spechtstatt There is still one outstanding comment from PhysSong. Can you please try to fix it (or tell us if someone else should fix it)?
@spechtstatt Do you plan to continue this PR (solving the last open comments), or is it necessary that someone of the other devs takes over your PR?
Hi Johannes,
unfortunately, I currently have no time to work on the PR and this will continue for a while.
So, yes. I would be happy if someone could take over.
According to Veratil and Dom, this can now be merged, so I will merge it now.