Keyboard shortcuts to preview/add sounds from sidebar #5427

Spekular · 2020-03-23T21:27:44Z

Now ready for testing and review

The file browser now behaves as follows:

Pressing arrow keys, space, or enter/return will cancel any running previews
Pressing up/down moves to the previous/next item
- If the new item is a sample, it will automatically preview
- If the key is held down and auto-repeats, previews are suppressed
Holding space previews the current item. The preview ends when space is released.
Enter/return sends the current item to the song editor as an instrument
- Samples can be sent to a new sample track instead by holding shift
- Holding control sends to the BB editor instead
- Samples can't be sent as sample tracks to the BB editor (wontfix), so ctrl+shift does nothing
RMB context menu gets fake headers, however legibility isn't ideal

~~Todo:~~

~~Sustained presets shouldn't play indefinitely. Only play presets while left arrow held?~~ Done
~~Shift + right arrow to add a sample to a new (empty) sample track in song editor~~ Done
~~Ctrl + shift + right arrow to add a sample to a new (empty) sample track in BB editor~~ Not feasible, nor particularly useful.

~~Code quality is iffy at best right now, among other things I did some questionable refactoring in the context menu code.~~ The commit "DRY up code and ..." vastly improves this

LmmsBot · 2020-03-23T21:34:39Z

🤖 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-736%2Bgebe30f8-linux-x86_64.AppImage (build link)

Windows

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

macOS

macOS : lmms-1.2.3-736%2Bgebe30f88f-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://9239-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-736%2Bgebe30f8-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9239?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://9240-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-736%2Bgebe30f88f-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9240?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://9242-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-736%2Bgebe30f88f-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9242?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/rm3ukilko9fvn74l/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35551388"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/f5fudlp3mrbvncep/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35551388"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://9243-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-736%2Bgebe30f88f-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9243?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "9d60c0ea58f99493b255d9a5e3232281b755a581"}

src/gui/FileBrowser.cpp

Spekular · 2020-09-26T15:29:51Z

IMHO this should close #5671. The proposal there isn't an improvement as far as I'm concerned, and with this merged it makes even less sense.

Veratil

The nullptr comment scopes across the whole review.

src/gui/FileBrowser.cpp

Spekular · 2020-09-26T19:22:25Z

FYI, CircleCI mac failure looks unrelated (it fails to fetch homebrew).

Spekular · 2020-09-29T15:39:01Z

The latest commit is a bit of a shot in the dark, the overhead from locking/unlocking more often and changing m_previewFileItem might cancel out the benefit of not hogging the lock as long (not to mention the fact that potentially neither of those are the issue). That being said, as far as I can tell it shouldn't introduce threading issues.

Previously: The code that I extracted into previewFileItem locked the preview mutex immediately, and only unlocked before returning. The code that I extracted into stopPreview was also completely wrapped in lock/unlock. Thus, whatever function entered the lock most recently would be the one that took effect.
Now: previewFileItem and stopPreview immediately lock and then set m_previewFileItem before unlocking. Whatever function entered the lock most recently decides the state of m_previewFileItem, which in turn decides what action will take effect.

If this commit works, I'll add comments clarifying that access to m_previewFileItem and m_previewPlayHandle should be protected by m_pphMutex (which should maybe be renamed to m_previewMutex?). If it fails or someone has objections, I'll revert the changes affecting when locks are aquired/released, but leave the use of QMutexLocker.

Spekular · 2020-09-29T16:21:43Z

Testing doesn't reveal any apparent difference, which isn't surprising. In hindsight it's pretty obvious that making the preview lock in FileBrowser available slightly faster wouldn't help, but it was an easy thing to check.

DomClark

Regarding m_pphMutex, couldn't it be removed altogether? From what I can tell, it's only ever locked from the UI thread, so there's nothing for it to protect against.

src/gui/FileBrowser.cpp

include/FileBrowser.h

src/gui/FileBrowser.cpp

Spekular · 2020-09-30T18:52:10Z

Regarding m_pphMutex, couldn't it be removed altogether? From what I can tell, it's only ever locked from the UI thread, so there's nothing for it to protect against.

My understanding is that it prevents several calls created via UI input from interfering with each other. Is this not necessary?

For example, if you very quickly tap the spacebar, couldn't this happen without locks?

previewFileItem gets as far as checking m_previewPlayHandle != nullptr
stopPreview sets the play handle to nullptr
We end up calling call Engine::getMixer()->addPlayHandle() with a nullptr argument

Also, as far as I can tell C++ makes no guarantees that a variable is in a valid state when read in unsynchronized code. Isn't this alone a good reason to avoid concurrent access?

Spekular · 2020-09-30T18:55:46Z

Ah wait, you mean that all the modifications to the play handle originate from UI events and thus run on the UI thread, so there's no concurrency happening. That makes sense, but to me it raises the issue that perhaps some of this work should actually happen outside the UI thread.

DomClark · 2020-09-30T18:59:27Z

... it raises the issue that perhaps some of this work should actually happen outside the UI thread.

True - it would definitely be nice to load samples (and presets too, I guess) asynchronously. Let's leave it in for now.

Spekular · 2020-10-01T13:42:55Z

Thanks for the review, @DomClark! Recent commits should address the issues you brought up.

I also tried replacing the preview mutex with a QThreadPool, but QRunnable::create(... lambda expression...) is only in Qt 5.15 and later and we're quite a bit behind. Anyway, the idea was as follows:

Create a thread pool with a maximum of one thread. As long as all access to m_previewPlayHandle happens by starting QRunnables on the thread pool, we achieve thread confinement for the play handle and also get out of the event thread's way.
previewFileItem and stopPreview would initially call clear() on the thread pool, clearing any queued-but-not-started tasks (since they're outdated). Then they would call start(QRunnable::create(... do all the work in a lambda...)). This should let the thread pool take care of their work ASAP unless another, newer, event comes along first.
mouseReleaseEvent would just queue itself. It wouldn't call clear(), since it may or may not need to stop the preview.

Veratil

The only other thing I see is no whitespace between parenthesis and curly brace in if (){ .. }.

include/FileBrowser.h

Veratil · 2020-10-01T15:17:53Z

src/gui/FileBrowser.cpp

+
+
+


I recall somewhere about having 4 newlines between functions, but I don't know if we care to keep doing that? I say because this hunk is unrelated to PR, and more a formatting update.

I wanted to keep FileBrowserTreeWidget's spacing a bit more consistent since it varied before, but I can undo this if you'd like.

FWIW, I don't like the four newlines. (I'm generally in favour of anything that uses less vertical space.)

I would also prefer a single newline between functions of the same class, but I've generally stuck with four in order to have some semblance of consistency.

Yeah - I don't think it should be changed here, but it might be a candidate for the reformatting if others agree.

I think it would be a thing for the clang-tidy/format PR.

src/gui/FileBrowser.cpp

Spekular · 2020-10-02T19:39:53Z

Thanks for the review @Veratil! I've fixed the event handler visibility and spacing in if statements, and also fixed some inconsistent spacing between FileBrowserTreeWidget's methods. I can revert the spacing changes if you'd like, but figured if they're going to be there they should be consistent.

Spekular · 2020-10-03T12:48:55Z

Any objections to me merging this? There are technically no approving reviews, I suppose, but I've fixed the issues brought up in review and no one has mentioned new ones so far.

src/gui/FileBrowser.cpp

PhysSong · 2020-10-31T03:21:40Z

src/gui/FileBrowser.cpp

 			{
-				s->setDoneMayReturnTrue( true );
-				m_previewPlayHandle = NULL;


I think this removed line(m_previewPlayHandle = NULL;) is the key of #5736. Mixer will cleanup the play handle later, and we won't stop previewing in this case.

- Extract file item preview start and end into new methods `previewFileItem` and `stopPreview`. - Add event handlers: - `keyPressEvent` to allow auto preview (on up/down arrow navigation), manual preview (space), and send to editors (enter) - `keyReleaseEvent` to end previews when preview key is released - `hideEvent` to end previews when switching sidebar tab or hiding sidebar - Functions that operate on a `FileItem` now take it as an argument instead of using a member variable - `getContextActions` provides menu items for sending clips to the song editor and BB editor with minimal duplicate code - Some formatting changes in affected code - Replace many instances of `NULL` with `nullptr`

Spekular added the in progress label Mar 23, 2020

Spekular marked this pull request as draft April 19, 2020 14:08

ryuukumar mentioned this pull request Jul 21, 2020

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

56 tasks

IanCaio reviewed Aug 15, 2020

View reviewed changes

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

Spekular added 4 commits September 23, 2020 16:18

Dropdown edits, attempted refactor

2574067

Working preview and add shortcuts

50cc251

Cleanup

87c51f6

Fixes after merge

e0e1f1c

Spekular force-pushed the browserShortcuts branch from 7aab491 to e0e1f1c Compare September 23, 2020 15:16

Spekular added 3 commits September 23, 2020 20:51

DRY up code and prevent endless previews

88f57b4

Initial send to new sample track implementation

7e3eb83

Don't send sample tracks to BB Editor

69ae7d1

Spekular added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing and removed in progress labels Sep 23, 2020

Spekular marked this pull request as ready for review September 23, 2020 20:22

Spekular added 3 commits September 24, 2020 16:43

Qt 5.6 compat, better comments, remove usused methods

53b4bcc

Change preview/add shortcut to space/enter

72bf011

Left/right keys should still stop previews

6f1441d

Veratil reviewed Sep 26, 2020

View reviewed changes

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

Spekular added 2 commits September 26, 2020 19:17

Formatting and NULL -> nullptr changes

5a22a20

Missed one

3700c72

Attempt to fix lag when spamming preview

098fe62

Simplify locking, stop previews when tab hidden

cdb4e5c

Spekular changed the title ~~Preview/add samples/presets with arrow keys~~ Keyboard shortcuts to preview/add sounds from sidebar Sep 29, 2020

Spekular requested a review from Veratil September 30, 2020 14:38

DomClark reviewed Sep 30, 2020

View reviewed changes

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

include/FileBrowser.h Outdated Show resolved Hide resolved

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

Spekular added 2 commits October 1, 2020 14:57

Fixes from Dom's review

5e0b8e7

TODO comments

4a314ab

Veratil reviewed Oct 1, 2020

View reviewed changes

Formatting changes from Veratil's review

638d717

DomClark reviewed Oct 3, 2020

View reviewed changes

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

src/gui/FileBrowser.cpp Outdated Show resolved Hide resolved

More review fixes

9d60c0e

DomClark approved these changes Oct 3, 2020

View reviewed changes

Spekular merged commit 109a7c4 into LMMS:master Oct 3, 2020

Spekular deleted the browserShortcuts branch October 3, 2020 19:31

Spekular mentioned this pull request Oct 26, 2020

Crash when previewing sample #5736

Closed

PhysSong reviewed Oct 31, 2020

View reviewed changes

This was referenced Nov 14, 2020

Add option to continue sidebar previews when mouse released #5787

Merged

Make a selection dialog (AFP/Sampe-track) for dragged samples #5671

Closed

zonkmachine mentioned this pull request Aug 13, 2022

Arrow keys should preview samples in the file browser #871

Closed

Spekular mentioned this pull request Nov 4, 2023

Improve search performance in FileBrowser #6962

Merged

Spekular mentioned this pull request Jun 7, 2024

key-bind to play side-bar sample #7307

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard shortcuts to preview/add sounds from sidebar #5427

Keyboard shortcuts to preview/add sounds from sidebar #5427

Spekular commented Mar 23, 2020 •

edited

Loading

LmmsBot commented Mar 23, 2020 •

edited

Loading

Linux

Windows

macOS

Spekular commented Sep 26, 2020

Veratil left a comment

Spekular commented Sep 26, 2020

Spekular commented Sep 29, 2020 •

edited

Loading

Spekular commented Sep 29, 2020

DomClark left a comment

Spekular commented Sep 30, 2020

Spekular commented Sep 30, 2020

DomClark commented Sep 30, 2020

Spekular commented Oct 1, 2020 •

edited

Loading

Veratil left a comment

Veratil Oct 1, 2020

Spekular Oct 2, 2020

DomClark Oct 2, 2020

Spekular Oct 2, 2020 •

edited

Loading

DomClark Oct 2, 2020

Veratil Oct 2, 2020

Spekular commented Oct 2, 2020

Spekular commented Oct 3, 2020

PhysSong Oct 31, 2020 •

edited

Loading

Keyboard shortcuts to preview/add sounds from sidebar #5427

Keyboard shortcuts to preview/add sounds from sidebar #5427

Conversation

Spekular commented Mar 23, 2020 • edited Loading

LmmsBot commented Mar 23, 2020 • edited Loading

Linux

Windows

macOS

Spekular commented Sep 26, 2020

Veratil left a comment

Choose a reason for hiding this comment

Spekular commented Sep 26, 2020

Spekular commented Sep 29, 2020 • edited Loading

Spekular commented Sep 29, 2020

DomClark left a comment

Choose a reason for hiding this comment

Spekular commented Sep 30, 2020

Spekular commented Sep 30, 2020

DomClark commented Sep 30, 2020

Spekular commented Oct 1, 2020 • edited Loading

Veratil left a comment

Choose a reason for hiding this comment

Veratil Oct 1, 2020

Choose a reason for hiding this comment

Spekular Oct 2, 2020

Choose a reason for hiding this comment

DomClark Oct 2, 2020

Choose a reason for hiding this comment

Spekular Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

DomClark Oct 2, 2020

Choose a reason for hiding this comment

Veratil Oct 2, 2020

Choose a reason for hiding this comment

Spekular commented Oct 2, 2020

Spekular commented Oct 3, 2020

PhysSong Oct 31, 2020 • edited Loading

Choose a reason for hiding this comment

Spekular commented Mar 23, 2020 •

edited

Loading

LmmsBot commented Mar 23, 2020 •

edited

Loading

Spekular commented Sep 29, 2020 •

edited

Loading

Spekular commented Oct 1, 2020 •

edited

Loading

Spekular Oct 2, 2020 •

edited

Loading

PhysSong Oct 31, 2020 •

edited

Loading