Global Spacebar Play #7762

regulus79 · 2025-03-09T03:56:38Z

This PR refactors how the spacebar is handled by the editors and the MainWindow to allow it to play/stop (Shift+Space for play/pause) the last played editor no matter if it is in focus or not.

Changes

Editor::togglePlayStop made public to allow MainWindow to call it.
Instead of a shortcut, Editor::keyPressEvent was added which checks if the spacebar is pressed and plays/stops the editor.
The last used PlayMode is now stored in Song::m_lastPlayMode and is updated whenever the play mode changes, but not when it changes to PlayMode::None.
Added a new case to MainWindow::keyPressEvent to handle the spacebar and call togglePlayStop on the right editor.
ke->ignore() added to the end of PianoRoll::keyPressEvent, since without it, there were problems with the key event being lost and not triggering the piano roll to play.

New Changes

Made PatternEditor use Strong Focus like the rest of the editors. This fixes a bug where the pattern editor sometimes would not play even when it was in focus.
Added Shift+Space to play/pause the last played editor.
Fixed a bug where pressing space after clicking within the mixer would open a dialog to change the volume of the selected channel.
- Moved the code which handled the spacebar from MixerView to MixerChannelView, and did some minor refactoring (converting the event filter into just keyPressEvent)
- Also added e->ignore() to the default case of MixerView::keyPressEvent in order to let the key event be passed up to MainWindow.
- Added a call to setFocus in MixerChannelView::mousePressEvent so that the MixerChannelView will process still the spacebar key event if you click on it.

~~I did not implement a global Shift+Space which appears to pause/play the editors. Do you want me to?~~ Done.

qnebra

Works like described. Switching between editors works, space to play in active editor works, space to play in last active editor also works. I don't see issues with using it.

regulus79 · 2025-03-13T17:46:39Z

Has anyone experienced issues in this PR with the pattern editor? I recall when implementing it that when I opened an old project, the pattern editor wouldn't play via the space bar until you actually press the play button.

rubiefawn

I left a few style suggestions, which should be considered non-blocking.

include/Song.h

rubiefawn · 2025-03-13T18:56:35Z

src/gui/MainWindow.cpp

+			switch(Engine::getSong()->lastPlayMode())
+			{
+				case Song::PlayMode::Song:
+					getGUI()->songEditor()->togglePlayStop();
+					break;
+				case Song::PlayMode::MidiClip:
+					getGUI()->pianoRoll()->togglePlayStop();
+					break;
+				case Song::PlayMode::Pattern:
+					getGUI()->patternEditor()->togglePlayStop();
+					break;
+				case Song::PlayMode::AutomationClip:
+					getGUI()->automationEditor()->togglePlayStop();
+					break;
+				default:
+					getGUI()->songEditor()->togglePlayStop();
+					break;
+			}


cases should be on the same indentation level as their switch.

Source: Qt Coding Style since it is not explicitly overidden in the LMMS coding conventions; an example in the LMMS coding conventions; precedent established by existing LMMS code

Suggested change

switch(Engine::getSong()->lastPlayMode())

{

case Song::PlayMode::Song:

getGUI()->songEditor()->togglePlayStop();

break;

case Song::PlayMode::MidiClip:

getGUI()->pianoRoll()->togglePlayStop();

break;

case Song::PlayMode::Pattern:

getGUI()->patternEditor()->togglePlayStop();

break;

case Song::PlayMode::AutomationClip:

getGUI()->automationEditor()->togglePlayStop();

break;

default:

getGUI()->songEditor()->togglePlayStop();

break;

}

switch(Engine::getSong()->lastPlayMode())

{

case Song::PlayMode::Song:

getGUI()->songEditor()->togglePlayStop();

break;

case Song::PlayMode::MidiClip:

getGUI()->pianoRoll()->togglePlayStop();

break;

case Song::PlayMode::Pattern:

getGUI()->patternEditor()->togglePlayStop();

break;

case Song::PlayMode::AutomationClip:

getGUI()->automationEditor()->togglePlayStop();

break;

default:

getGUI()->songEditor()->togglePlayStop();

break;

}

Completely wrong to recommend this. Reopening for discussion. No, no no no no. 😆

For context whenever declaring law, the de facto law that proceeds should be carefully considered. We've never done this. I've never seen this in the wild and I vote we stop recommending this.

Poll created: https://discord.com/channels/203559236729438208/207159523616489473/1353744413310058588

rubiefawn · 2025-03-13T18:59:45Z

src/gui/editors/PianoRoll.cpp

@@ -1487,6 +1487,7 @@ void PianoRoll::keyPressEvent(QKeyEvent* ke)
 			}
 			break;
 		default:
+			ke->ignore();


Above comment about switch/case indentation is relevant here as well, but formatting this one is optional since you barely touched it.

src/gui/editors/Editor.cpp

include/Editor.h

Co-authored-by: Fawn <[email protected]>

AW1534

Tested this and it works perfectly as described. The code looks good too.

tresf · 2025-03-21T04:06:54Z

ke->ignore() added to the end of PianoRoll::keyPressEvent, since without it, there were problems with the key event being lost and not triggering the piano roll to play.

If possible, I'd like this tested against the qt6 branch as well. I had to add an extra ignore() for mouse propagation recently and this PR reminded me of that.

Add Qt6 Support #7339

tresf · 2025-03-21T04:22:12Z

This has some interesting behavior. This is best shown through a video. Steps:

Load a saved project
Click the title of the song-editor window
Hit Space
Click the title of the pattern-editor window
Hit Space (song-editor plays)
Click an empty space in the pattern-editor window
Hit Space (pattern-editor plays)

(Note: Clicking the pattern editor button in the toolbar and then space will also play the pattern-editor)

There are also some nuanced focus issues that are likely unrelated to this PR, such as hitting spacebar clicks a previously focused button. We may decide to remove certain item focuses when window focus changes, as a separate enhancement.

regulus79 · 2025-03-21T20:46:29Z

This has some interesting behavior. This is best shown through a video. Steps:

I can also reproduce this issue. It appears to only occur when loading saved projects. I'm not sure why this is happening, so I will have to investigate further.

regulus79 · 2025-03-21T21:50:38Z

For some reason after opening a project, the key event is not passed to the pattern editor even when the window is in focus. This does not occur for the Song Editor or the Piano Roll. It also does not occur the first time you open LMMS. However, the issue resolves itself when you interact with the Pattern Editor by either clicking somewhere in the window background.

…vent to make sure Editor recieves the key event if it is not processed.

regulus79 · 2025-03-21T22:26:37Z

Alright, I think I've fixed it! It appears that PatternEditor did not share the same setFocusPolicy as the other editors, which somehow caused the issue. I don't really understand how Qt's focus system works, but adding those lines seemed to fix the problem.

I also added a ke->ignore() to the default branch of AutomationEditor::keyPressEvent so that the event will be passed on to Editor::keyPressEvent if it is not processed by the pattern editor. This fixed an issue caused by this PR which I was not aware of, which broke the space bar in the automation editor.

regulus79 · 2025-03-22T00:41:23Z

If possible, I'd like this tested against the qt6 branch as well. I had to add an extra ignore() for mouse propagation recently and this PR reminded me of that.

I tested out that branch with this pr merged into it, and it seemed to work fine. I did encounter a segfault when adding an effect, but I can't reproduce it and I don't know if it's related.

tresf · 2025-03-22T04:45:49Z

Alright, I think I've fixed it! It appears that PatternEditor did not share the same setFocusPolicy as the other editors, which somehow caused the issue. I don't really understand how Qt's focus system works, but adding those lines seemed to fix the problem.

@regulus79 appears fixed here as well, thanks! This feature is awesome!

This may be unrelated to this PR, but the mixer doesn't work at all (well, I got it to work once but I don't know how)... instead it always tries to change a knob value. I assume this is due to the channel gaining focus, but the only way to reliably remove focus is to open an effect and hit space there. I'm not sure if you'd rather track that issue here or in a separate issue.

What's interesting about this behavior is that it continues to happen even after hiding the mixer. If you hit space, after hiding the mixer, it tries to adjust the mixer volume still. 😆

regulus79 · 2025-03-23T22:06:16Z

Before we merge, are you all sure you don't want me to add a global Shift+Space to pause?

sakertooth · 2025-03-23T22:13:56Z

Before we merge, are you all sure you don't want me to add a global Shift+Space to pause?

I would think the global spacebar would work as a toggle.

regulus79 · 2025-03-23T22:15:10Z

I'm asking because that's how it works right now. Normal space does play/stop, shift+space does play/pause.

sakertooth · 2025-03-23T22:22:36Z

I'm asking because that's how it works right now. Normal space does play/stop, shift+space does play/pause.

Oh wow I wasn't aware lol, I was thinking the global spacebar was play/pause. I think it would be nice if the global spacebar would align with what was chosen in the song editor (i.e., the |<<, <<, and >> options). We should also add that button for the pattern editor too I think.

sakertooth · 2025-03-23T22:24:15Z

Are we already doing this? Sorry I haven't tested the PR yet.

regulus79 · 2025-03-23T22:25:48Z

I think it would be nice if the global spacebar would align with what was chosen in the song editor (i.e., the |<<, <<, and >> options). We should also add that button for the pattern editor too I think.

That is already handled by the editor; this PR just calls the play/stop function. As for the pattern editor, perhaps that would fit in #7794?

sakertooth · 2025-03-23T22:28:12Z

Okay, so I suppose adding the Shift+Space would be okay for consistency mainly. I'm not really against the addition.

regulus79 · 2025-03-23T22:35:47Z

Done!

sakertooth

I would make some small formatting adjustments but I won't try and nitpick too much. I'll test the PR and after that we should be good.

Also, I think maybe the PR could use a better title? Something that describes what the commit actually does. Maybe like "Add a global spacebar".

include/Editor.h

src/gui/MainWindow.cpp

include/Song.h

Co-authored-by: Sotonye Atemie <[email protected]>

sakertooth · 2025-03-23T22:50:12Z

When I press spacebar in the Mixer, it shows this:

regulus79 · 2025-03-23T22:51:55Z

When I press spacebar in the Mixer, it shows this:

That also occurs in master. For some reason when you press spacebar after clicking within the mixer, it thinks you are trying to set the volume of the currently selected channel.

sakertooth · 2025-03-23T22:52:51Z

Any chance we could fix that in here? I think for this feature, the mixer is the most important part.

regulus79 · 2025-03-23T22:56:26Z

I mean, I guess I could. But I'm not sure it's really related to this PR.

sakertooth · 2025-03-23T22:59:39Z

I would think anyone using this feature, this would be somewhat of the first thing they notice. I get it's already on master but I was hoping for this feature to really be useful with the mixer in particular. If its an easy change, I think it would be fine to do it here. Otherwise, if it requires a decent amount of changes from what the PR has right now, then it can wait I guess.

sakertooth · 2025-03-23T23:00:55Z

I would also say its perfectly in scope to be honest: The feature is not really useful in the mixer, which is not something to scoff at.

If we wanted it to be also working in the mixer (which we should IMO), then if we don't do it here and in a follow up PR, the PRs/objectives end up being quite related actually.

regulus79 · 2025-03-23T23:08:45Z

Alright, it should be fixed!

I had to remove the handling of the space key event from MixerView and add it to keyPressEvent in MixerChannelView so that it would only be triggered if the user has the channel in focus. Also, it appeared that MixerChannelView was using an eventFilter to handle key press events, so I converted that to be keyPressEvent to be consistent.

regulus79 · 2025-03-23T23:15:20Z

Wait wait wait hold on a second I did something wrong

regulus79 · 2025-03-23T23:33:27Z

Yeah, so just like PianoRoll and AutomationEditor, the MixerView's keyPressEvent function did not set ke->ignore() by default if it did nothing, which meant the key event was not getting passed up to MainWindow. It should be fixed now.

sakertooth · 2025-03-23T23:40:12Z

Great, this is really handy now! :)

regulus79 · 2025-03-24T00:09:38Z

Wait a minute again... somehow I broke the spacebar to enter the volume. I should really be doing more testing before doing git push.

Unless we're okay with losing that functionality? If we want spacebar to always mean play/stop/pause, then maybe it would make sense to remove this shortcut/change it to something else?

sakertooth · 2025-03-24T00:20:05Z

Unless we're okay with losing that functionality? If we want spacebar to always mean play/stop/pause, then maybe it would make sense to remove this shortcut/change it to something else?

I wasn't even initially aware that spacebar in the Mixer opened the volume dialog. IMO, since we can already open the dialog by double clicking, opening it with the spacebar might be redundant. But yeah that's just my opinion. It's a hard decision and it needs more input from other devs/the community.

regulus79 · 2025-03-24T00:23:57Z

It's okay, I fixed it (for realsies this time :D)

Basically the MixerChannelView was not automatically getting focus when you click on it (except when renaming the channel, since in that case the lineEdit child had focus), so I added a call to setFocus in MixerChannelView::mousePressEvent which seemed to do the trick. I hope I tested it well enough this time.

tresf · 2025-03-24T14:48:21Z

Unless we're okay with losing that functionality? If we want spacebar to always mean play/stop/pause, then maybe it would make sense to remove this shortcut/change it to something else?

I wasn't even initially aware that spacebar in the Mixer opened the volume dialog. IMO, since we can already open the dialog by double clicking, opening it with the spacebar might be redundant. But yeah that's just my opinion. It's a hard decision and it needs more input from other devs/the community.

I thought about this too... It would likely become a future accessibility/robots issue. I'm sure we have plenty of these, but that's the first thing that comes to mind especially when we're talking about automating the GUI as a futuremap.

regulus79 added 2 commits March 8, 2025 22:17

Initial implementation

0c1e751

Fix spacebar not working when instrument window is open

2a134c2

qnebra approved these changes Mar 13, 2025

View reviewed changes

rubiefawn approved these changes Mar 13, 2025

View reviewed changes

regulus79 and others added 3 commits March 13, 2025 15:09

Update switch formatting

e13f7ba

Co-authored-by: Fawn <[email protected]>

Format getter into single line

fd4be68

Co-authored-by: Fawn <[email protected]>

Put the * with the variable name

15c6eb0

Co-authored-by: Fawn <[email protected]>

AW1534 approved these changes Mar 16, 2025

View reviewed changes

regulus79 added 2 commits March 21, 2025 18:19

Fix focus issues with PatternEditor

ec7f4b5

Add ke->ignore() at the default branch of AutomationEditor::keyPressE…

7e82530

…vent to make sure Editor recieves the key event if it is not processed.

Add Shift+Space for pause

826a55f

sakertooth reviewed Mar 23, 2025

View reviewed changes

include/Editor.h Outdated Show resolved Hide resolved

include/Editor.h Outdated Show resolved Hide resolved

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

include/Song.h Outdated Show resolved Hide resolved

Fix styling and remove inline

f69d0ca

Co-authored-by: Sotonye Atemie <[email protected]>

Fix mixer spacebar issue

755e31f

Make MixerView ignore ke by default

c42d9d1

Fix spacebar issue

e3253d2

Global Spacebar Play #7762

Are you sure you want to change the base?

Global Spacebar Play #7762

Conversation

regulus79 commented Mar 9, 2025 • edited Loading

Changes

New Changes

qnebra left a comment

Choose a reason for hiding this comment

regulus79 commented Mar 13, 2025

rubiefawn left a comment

Choose a reason for hiding this comment

rubiefawn Mar 13, 2025

Choose a reason for hiding this comment

tresf Mar 24, 2025

Choose a reason for hiding this comment

tresf Mar 24, 2025

Choose a reason for hiding this comment

tresf Mar 24, 2025

Choose a reason for hiding this comment

rubiefawn Mar 13, 2025

Choose a reason for hiding this comment

AW1534 left a comment

Choose a reason for hiding this comment

tresf commented Mar 21, 2025

tresf commented Mar 21, 2025 • edited Loading

regulus79 commented Mar 21, 2025

regulus79 commented Mar 21, 2025

regulus79 commented Mar 21, 2025

regulus79 commented Mar 22, 2025

tresf commented Mar 22, 2025

regulus79 commented Mar 23, 2025

sakertooth commented Mar 23, 2025

regulus79 commented Mar 23, 2025

sakertooth commented Mar 23, 2025

sakertooth commented Mar 23, 2025

regulus79 commented Mar 23, 2025

sakertooth commented Mar 23, 2025

regulus79 commented Mar 23, 2025

sakertooth left a comment

Choose a reason for hiding this comment

sakertooth commented Mar 23, 2025

regulus79 commented Mar 23, 2025 • edited Loading

sakertooth commented Mar 23, 2025 • edited Loading

regulus79 commented Mar 23, 2025

sakertooth commented Mar 23, 2025

sakertooth commented Mar 23, 2025 • edited Loading

regulus79 commented Mar 23, 2025

regulus79 commented Mar 23, 2025

regulus79 commented Mar 23, 2025

sakertooth commented Mar 23, 2025

regulus79 commented Mar 24, 2025

sakertooth commented Mar 24, 2025

regulus79 commented Mar 24, 2025

tresf commented Mar 24, 2025

regulus79 commented Mar 9, 2025 •

edited

Loading

tresf commented Mar 21, 2025 •

edited

Loading

regulus79 commented Mar 23, 2025 •

edited

Loading

sakertooth commented Mar 23, 2025 •

edited

Loading

sakertooth commented Mar 23, 2025 •

edited

Loading