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

Add option to continue sidebar previews when mouse released #5787

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

Spekular
Copy link
Member

Adds a new option to the (incredibly crowded) General>UI section of our preferences that allows clicked samples in the sidebar to play to completion/interruption. With this setting enabled, sample previews will not stop on mouse release, but will still stop when:

  • Another sidebar item gains focus (another preview starts, cursor moves to folder)
  • The sidebar loses focus (another area is clicked on)
  • The sample finishes playing

Thanks to changes in #5764 (stopping previews on focus loss, specifically), this should no longer cause the crash described in #5736. Please test this! Somewhat related: #5427

Milestoning for 1.3 since 1.2 had this option... ish.

@Spekular Spekular added this to the 1.3 milestone Nov 14, 2020
@LmmsBot
Copy link

LmmsBot commented Nov 14, 2020

🤖 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

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10539-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bg6597161-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10539?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://10535-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bg65971610d-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10535?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10536-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bg65971610d-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10536?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/f4v7xu8fiue789om/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36364947"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/k03u5ldip47es1sg/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36364947"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10537-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bg65971610d-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10537?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "889c312eacb3d7820a95f871705b9c9ba24ba27f"}

@superpaik
Copy link
Contributor

It works perfectly as described. Only one minor thing. When you said (regarding scenarios to stop sample from playing): "Another sidebar item gains focus (another preview starts, cursor moves to folder)", does it means that if I click (or open/close) on a folder (not a sample) the current sample should stop? It doesn't. Personally I think that's a better behaviour than stop it, but I don't know if this is intended or a side-effect.
No crashes found (related to #5736 )
It saves and loads the configuration setting.

@Spekular
Copy link
Member Author

When you said (regarding scenarios to stop sample from playing): "Another sidebar item gains focus (another preview starts, cursor moves to folder)", does it means that if I click (or open/close) on a folder (not a sample) the current sample should stop? It doesn't.

I was thinking about the arrow keys when I said that, so I forgot about the mouse. Thanks for pointing it out!

Personally I think that's a better behaviour than stop it, but I don't know if this is intended or a side-effect.

It wasn't intended, but I'm not sure if I should change it or not. I'll test a bit in LMMS and other DAWs and see what feels natural.

Also something I realized yesterday and was planning to fix as soon as I got the time: preset previews will likely play forever with this setting on, so I need to restore the previously existing logic that ensured they always turn off on mouse-up.

@Spekular
Copy link
Member Author

Latest commit restores proper behavior for preset previews, so they stop when the mouse is released as they should. Nothing else should be different.

Both Ableton and FL let previews continue when folders are opened/closed, and it doesn't feel bad or unintuitive to me, so I'm leaving that as is.

@Spekular Spekular added the needs code review A functional code review is currently required for this PR label Nov 17, 2020
Copy link
Contributor

@Veratil Veratil left a comment

Choose a reason for hiding this comment

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

Nothing stands out to me, it looks good. I haven't built and tested, but the logic is sound.

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

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

Will later try to download the build and test, but as far as the code goes LGTM!

@Spekular
Copy link
Member Author

Quoting myself and IanCaio from Discord:

do you still intend to test #5787 or should I go ahead and merge it?

[..] I didn’t test it yet but I did review the code and it all looked good, since others have tested and had no issues it’s alright by me if you want to merge it now

@Spekular Spekular merged commit ed9abe5 into LMMS:master Nov 24, 2020
@Spekular Spekular deleted the preview-preference branch November 24, 2020 20:50
@Spekular Spekular removed the needs code review A functional code review is currently required for this PR label Dec 2, 2020
IanCaio pushed a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
* Add option to continue sidebar previews when mouse released

* Cancel non-sample previews regardless of setting
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
* Add option to continue sidebar previews when mouse released

* Cancel non-sample previews regardless of setting
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Add option to continue sidebar previews when mouse released

* Cancel non-sample previews regardless of setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants