Stop sample previews on mouse release or browser focus loss

[Spekular]

This fix/workaround closes #5736.

I'll make a PR once this is merged that allows samples to keep playing after mouse release (depending on the users settings), because it seems that stopping samples on focus loss is enough to prevent the crash*. In other words, all that remains is some work to add/save/read the option. However, since this would increase the scope of the PR and review times, I'd like to get this merged first so the crash is fixed and nightly builds can go up.

*If there turns out to be a race condition I think we can either add a stopPreview call to the stop button code or fix it in the mixer.

[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.3-752%2Bg90e0440-linux-x86_64.AppImage (build link)

Windows

• Windows 64-bit: lmms-1.2.3-752%2Bg90e04400b-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.2.3-752%2Bg90e04400b-mingw-win32.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)
• 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)

macOS

• macOS : lmms-1.2.3-752%2Bg90e04400b-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10226-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg90e0440-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10226?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10229-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg90e04400b-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10229?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10227-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg90e04400b-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10227?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/0mvtwohjohhv2gm8/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36187111"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/pkqg7776n1s2hb6v/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36187111"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/lrunl6k92ov9r5vg/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36187112"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/46s1x1beoy1o6ijo/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36187112"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10228-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg90e04400b-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10228?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "90e04400b858e8e204aff46f0c806e064722be56"}

[IanCaio]

Small suggestion

Suggested change

	m_previewPlayHandle = NULL;
	m_previewPlayHandle = nullptr;

[superpaik]

Though it fixes the crash, it introduces a somehow annoying side effect (I think it's been explained in the #5736 thread): the loop continues to sound while the new one is playing, which is very confusing, and can be very annoying if the loop is long.
This side-effect doesn't happen when you move around samples with the keyboard, samples go on and off perfectly. Maybe looking at the code for how keyboard is managed can help.

[Spekular]

@superpaik The fact that samples (shorter than 3 seconds) continue after mouse release is intentional behavior, and would be very easy to remove. It does make some sense for samples like drums that they can be played just by clicking (rather than holding), but it can be unintuitive so IMO it might be a good idea to remove it.

I decided to investigate what other DAWs do when a sample is clicked:

• Ableton plays the sample until it finishes or another preview starts. Samples also stop when the browser loses focus
• FL plays the sample until it finishes or another preview starts. Samples also stop when the play or stop button is pressed

Another potential fix: Stop previews when the sidebar loses focus. Theoretically, if the preview is stopped by the sample browser before the stop button is pressed, the issue shouldn't occur. However, depending on the order in which the events are sent and processed the mixer clear function might end up being called before the sidebar calls stopPreview, so this feels like a risky fix (potential race condition).

[Spekular]

Or maybe the function hooked up to the stop button could tell the file browser to stop the preview before stopping the song. That should guarantee the order of the calls and prevent the crash.

[superpaik]

@Spekular I understand that the sample continue playing after mouserelease, it's a good and desired feature. But I think that when another sample is clicked, the one that is playing should stop (as you said others DAW do).
In fact, that is what happen when you move around samples with keyboard and not mouse (I believe this is something new -keyboard- in the master not available in the stable version (1.2.2).
I'm not very good at coding but on the keyPressEvent function, the first thing that is done is stop another samples from playing. I think that's what is needed inside mousePressEvent. I may be wrong, though.

[Spekular]

I've changed the method used to prevent the crash, so this PR now stops samples on mouse release. Note, I intend to add an option for those who prefer the sample to continue playing as soon as this is merged. In other words I don't think this PR needs a long debate about the preferred behavior, just a review of code quality and some testing to ensure that it works as described.

[Spekular]

Tested to fix the crash by myself and zonkmachine, and the code is rather simple, so I'll merge this.