Fix Settings bug that occurs when not using JACK#8299
Conversation
Opening the settings would unconditionally try to start the JACK server if it was not already running, even if the user was using an audio backend other than JACK. This fixed that problem by passing the JackNoStartServer option to jack_client_open() in the setupWidget. initJackClient() continues to use the JackNullOption option, so it will still attempt to start the JACK server if you select the JACK backend and restart LMMS.
JohannesLorenz
left a comment
There was a problem hiding this comment.
I only left 1 comment.
Review scope:
- Functional code review -> OK
- Style code review -> OK
- Testing -> OK
I tested:
- Jack Server not available on system -> Error message on stderr, settings dialog still opens normally
- Jack Client is available, but not started -> Jack is being started, dialog works
- Jack Client is already started -> No messages, dialog works
| : AudioDeviceSetupWidget(AudioJack::name(), parent) | ||
| { | ||
| const char* serverName = nullptr; | ||
| // TODO: Add a button to start the JACK server? |
There was a problem hiding this comment.
So, the disadvantage of this PR is for users who have jack installed properly but don't have it started currently, these users can now not see the option dialogue. I assume that's exactly what this comment proposes. It would be a good solution to this.
I assume it's not much work to add such a button to start (and possibly also to stop) the jack server? I guess it would be a nice compensation to this PR.
There was a problem hiding this comment.
Thanks for testing!
So, the disadvantage of this PR is for users who have jack installed properly but don't have it started currently, these users can now not see the option dialogue.
No, if JACK is the selected as the current audio device in the Settings, AudioJack::initJackClient() will be called at startup and start the JACK server if it isn't already. With this PR it just won't try to start the JACK server again when opening the Settings, since should already be started due to AudioJack::initJackClient().
The start/stop button would be useful in the future if we're able to change the audio device without restarting LMMS. We'd want there to be a way to display the JACK server status and provide a button to start the JACK server. Bitwig Studio has such a feature.
But for now having that button would be less useful. For example, if you're currently using the SDL backend but then go into the Settings and switch to JACK, the JACK server might not be running so you'd want to start it. However, even if you start it you wouldn't be able to use it until you restart LMMS, and LMMS already takes care of starting the JACK server at startup, so there wouldn't be much use to having the button right now.
There was a problem hiding this comment.
But for now having that button would be less useful. For example, if you're currently using the SDL backend but then go into the Settings and switch to JACK, the JACK server might not be running so you'd want to start it.
Sorry, yes, that was what I meant.
Let's see what Copilot has to say 😆
There was a problem hiding this comment.
Pull request overview
Fixes a regression where opening the Settings dialog would try to start the JACK server even when JACK isn’t the selected audio backend, causing multi-second delays and potentially disrupting system audio.
Changes:
- Open the JACK client from the Settings/setup widget with
JackNoStartServerto avoid auto-starting JACK. - Keep
AudioJack::initJackClient()behavior unchanged (JackNullOption) so selecting the JACK backend still attempts to start the server as before. - Remove the unused JACK server-name vararg from
jack_client_open()calls and refactor local statics into an anonymous namespace.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This fixes a regression from #7919.
Opening the Settings would unconditionally try to start the JACK server if it was not already running, even if the user was using an audio backend other than JACK. When it failed to start the JACK server, it would retry a few times which made opening the Settings take several seconds. For me, it also broke my whole system's audio, requiring me to restart pipewire (see the console logs below).
This PR fixes that problem by passing the
JackNoStartServeroption tojack_client_open()in thesetupWidgetused by the Settings.AudioJack::initJackClient()continues to use theJackNullOptionoption, so like before, it will still attempt to start the JACK server if you select the JACK backend and restart LMMS.I also moved some
staticfree functions into a nearby anonymous namespace, and I removed the server name argument from thejack_client_open()calls since that variadic argument is only supposed to be passed if theJackServerNameoption is passed (see documentation).Console output showing the bug that was fixed by this PR
Click to expand