Remove ASIO button from Audio/Network settings when using JACK#2215
Remove ASIO button from Audio/Network settings when using JACK#2215softins merged 2 commits intojamulussoftware:masterfrom
Conversation
| @@ -132,7 +132,7 @@ CClientSettingsDlg::CClientSettingsDlg ( CClient* pNCliP, CClientSettings* pNSet | |||
|
|
|||
There was a problem hiding this comment.
It looks to me like lines 120 onwards through 146 are ASIO-specific code. Can they all go into a #if block? (Else hide the controls?)
Also, is there any code handling change in selection that should not be compiled in the same cases?
There was a problem hiding this comment.
Guess that would be possible, also the channel mapping does not really apply when using JACK. Am I correct?
As if so, there is more to be removed when using JACK.
There was a problem hiding this comment.
Any code that doesn't apply for JACK also doesn't apply for JACK on Windows.
There was a problem hiding this comment.
Hm, !defined( WITH_JACK ) covers iOS and Android.
You need something that says "MacOS or (Windows and not JACK)" -- or "not JACK" and then "either MacOS or Windows".
There was a problem hiding this comment.
I only used this once (line 120). All other 'if' lines that have WITH_JACK have a second check.
And this relates to the WhatsThis text for the Device selection and channel mapping.
Do you believe iOS and Android do not support multiple devices?
And channel mapping, I also left this in place just in case it will be possible to have more than 2. I know for sure that channel mapping does not apply to JACK, but the rest may or may not show this depending on connect hardware/installed drivers.
For a moment I had doubt...and wanted to make the WhatsThis separate for Windows and macOS. If you believe I should add the OS here, do you agree it would be better to do this as well?
There was a problem hiding this comment.
ASIO lets you choose ASIO driver, because multiple ASIO drivers can register and the app talks to the ASIO driver.
JACK doesn't work that way - the app talks to JACK, JACK manages multiple devices.
MacOS has this weird system for audio aggregate devices that I don't understand and I don't know how it's managed.
As far as I'm aware, Android and iOS don't let you know anything about the sound devices, just use them.
Again, that's why all this code should not be here -- it should be in the platform-specific sound code, then it would be clear what needs to be said for each case.
There was a problem hiding this comment.
Each sound system implementation could provide a panel with all the necessary widgets specifically for that sound system.
If there was nothing that could be done, the panel could be empty and set to hidden.
Then the common sound code could supply the system-specific panel to the settings code for inserting into the UI.
There was a problem hiding this comment.
MacOS has this weird system for audio aggregate devices that I don't understand and I don't know how it's managed.
In macOS you get the option to select which input/output combination you want to use supported by your system. So there is functionality linked to this drop down list. I have not seen the channel mapping on macOS, but I also haven't got a macOS running with multiple driver/outputs to select from.
I am still tempted to just use "WITH_JACK" for this, because I know for sure that it will not change the existing behaviour and you won't have to change/update if you suddenly have another OS that would support multiple devices/drivers.
My Android device it just list the Device as "Oboe" and no other option available. Similar as how to Linux is just showing "Jack" in the drop down and on Android I can not get to the tooltip and whatisthis detail in this dialog (or better..I don't know how to).
Not sure how you want to proceed with this. This issue/pr is just about removing the ASIO button when using JACK, and it seems we are trying to get a lot more done in this now.
Initially I just added removing the ASIO tooltip/whatisthis, as if the button is not there, then these should not be included as well.
Perhaps we should do another issue/pr for the "removal (hiding)" of the Device (with dropdown) for other platforms?
There was a problem hiding this comment.
OK for this PR, WITH_JACK will do.
Just to repeat... it worries me that there's leakage of sound system-specific code into the generic code. Each time something like this comes up, it triggers the question "Where else have we got it wrong?" (and it might not be just the sound code that's affected - there's potentially for leakage from the sound code to the client code in general, as demonstrated here).
I would like some work done to clean this up - move all the sound system-specific back into the subdirectories for each system, wrap that in a common wrapper in the common code and the common code where anything sound-related is needed.
But that's a task for another time.
There was a problem hiding this comment.
I would like some work done to clean this up - move all the sound system-specific back into the subdirectories
Sound like a good thing but also some massive work.
softins
left a comment
There was a problem hiding this comment.
Looks ok and the CI has no errors, so happy to approve.
Short description of changes
Remove the ASIO Settings button in the Audio/Network Setting dialog when using JACK or when using macOS.
Context: Fixes an issue?
Fixes: #2186
The ASIO sound dialog (from soundbase.cpp) is not displayed when using JACK.
Therefore it is only needed to remove the button from the settings dialog.
Does this change need documentation? What needs to be documented and how?
No
Status of this Pull Request
Complete
What is missing until this pull request can be merged?
Nothing
Checklist