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.

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

Fixes bug where clicking on the Activity Indicator doesn't play a note #5824

merged 2 commits into from Dec 7, 2020


Copy link

@IanCaio IanCaio commented Dec 5, 2020

After merging of #5581 a bug was unveiled, where an ambiguity on the arguments types of calls to the MidiEvent constructor would result in ADL picking the wrong option.

Discussing with others in Discord about approaches to fix the issue, @DomClark suggested using an enum class instead of a boolean on the last argument, replacing it from ignoreOnExport to source (representing the source of a MidiEvent). One of the arguments type from the second constructor was also changed from int to std::size_t to better represent it.

Those changes alone fixes the Activity Indicator. It's possible though that adding static_casts to the constructor calls could still be needed. For now I'm submitting the changes for further review and discussion.

	This attempts to fix a bug uncovered by LMMS#5581, where the wrong
MidiEvent constructor is selected due to ambiguity in the arguments that
are used in the call.
	Part of the solution is to use a enum class on the last
parameter instead of a boolean to avoid implicit conversions and a more
strict method selection through the ADL. One of the parameters type on a
constructor was also changed from int to std::size_t which more
accurately represents it.
	"ignoreOnExport" was replaced with "source", which for now can
be Source::Internal or Source::External, and its value is still used on
the processInEvent to define whether a MidiEvent should be ignored
during the song export.
Copy link

LmmsBot commented Dec 5, 2020

🤖 Hey, I'm @LmmsBot from and I made downloads for this pull request, click me to make them magically appear! 🎩




{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": ""}}, "build_link": ""}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": ""}}, "build_link": ""}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": ""}}, "build_link": ""}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": ""}}, "build_link": ""}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": ""}}, "build_link": ""}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": ""}}, "build_link": ""}]}, "commit_sha": "cd90121c798fe3be3f9475d56c4b7a2d81d682bb"}

Copy link

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

Looks good, and fixes the bug. There are a couple more things that could be done, but I'm not sure if it's worth doing them here:

  • m_data.m_sysExDataLen should be changed to a std::size_t, to match dataLen which is stored in it. However, it seems to be unused, so it doesn't matter too much right now.
  • Should all MidiEvents be constructed with the correct source? Currently everything is marked as external, except controller changes. Again, this is fine for now, since most messages go to processOutEvent where the source doesn't matter, but it might be worth correcting at some point. (I would probably make the default source Internal, and specify External in the MIDI backends, to keep the core code cleaner.)

@IanCaio IanCaio merged commit 53a733b into LMMS:master Dec 7, 2020
IanCaio added a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants