Fixes #6398: Colour of Master bus on FX Mixer retains when switching project

[spechtstatt]

Fixes #6398

[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! 🎩

macOS

• macOS : lmms-1.3.0-alpha.1.221+g8fe731fc9-mac10.14.dmg (build link)

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)

🤖

{"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"}

[Monospace-V]

Can't replicate in any way. Seems fixed.

[zonkmachine]

Merge?

[JohannesLorenz]

The changes are OK. However, looking at the comment about m_hasColor in the header, it's suggested to use std::optional. Indeed, m_hasColor is always used together with m_hasColor, so (instead of using std::optional) we could remove m_hasColor and instead use QColor::isValid(), which returns false after using the default CTOR? This should reduce future errors, where m_hasColor and QColor::isValid() are not in sync.

[Spekular]

Optional should be available by now though, I believe we already bumped to the required C++ version.

[JohannesLorenz]

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.

[PhysSong]

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.

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.

[JohannesLorenz]

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?

[PhysSong]

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?

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.

[Spekular]

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.

[JohannesLorenz]

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.

[DomClark]

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.

[spechtstatt]

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.

[JohannesLorenz]

@spechtstatt The use of std::optional looks correct, but shouldn't it replace m_hasColor?

[spechtstatt]

Ok - I see - somehow like the Nullables in C#.

Yes - makes sense.

[PhysSong]

@spechtstatt It seems like somehow changed tabs to spaces.

[spechtstatt]

Oh sorry.

I switched my Computer recently and I probably forgot to change it in the qt creator settings.

[PhysSong]

@spechtstatt The use of std::optional looks correct, but shouldn't it replace m_hasColor?

@spechtstatt Could you address that with the indentation fixes(revert to tabs)?

[spechtstatt]

Yes of course.

[JohannesLorenz]

Only minor and very easy-to-solve comments. Feel free to ignore.

[allejok96]

Code looks good except for one warning during build

[allejok96]

Confused by the commit log, but it builds fine and my comments seem to be fixed. Haven't test run tho.

[JohannesLorenz]

Tested. Bug is fixed an no regressions.

[JohannesLorenz]

2 code reviews + 1 test review passed. Suggesting merge.

[spechtstatt]

@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.

[JohannesLorenz]

@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]

@JohannesLorenz: not sure how to fix this - any suggestions?

[Veratil]

Code style fixes and then we can merge :)

[JohannesLorenz]

@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?

[spechtstatt]

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.

[JohannesLorenz]

According to Veratil and Dom, this can now be merged, so I will merge it now.