Skip to content

Comments

Remove ASIO button from Audio/Network settings when using JACK#2215

Merged
softins merged 2 commits intojamulussoftware:masterfrom
henkdegroot:hide-asio-button-withjack
Jan 10, 2022
Merged

Remove ASIO button from Audio/Network settings when using JACK#2215
softins merged 2 commits intojamulussoftware:masterfrom
henkdegroot:hide-asio-button-withjack

Conversation

@henkdegroot
Copy link
Contributor

@henkdegroot henkdegroot commented Jan 5, 2022

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

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@henkdegroot henkdegroot changed the title Remove ASIO button when using JACK Remove ASIO button from Audio/Network settings when using JACK Jan 5, 2022
@@ -132,7 +132,7 @@ CClientSettingsDlg::CClientSettingsDlg ( CClient* pNCliP, CClientSettings* pNSet

Copy link
Collaborator

@pljones pljones Jan 5, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any code that doesn't apply for JACK also doesn't apply for JACK on Windows.

Copy link
Collaborator

@pljones pljones Jan 7, 2022

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@henkdegroot henkdegroot marked this pull request as draft January 7, 2022 06:18
@henkdegroot henkdegroot marked this pull request as ready for review January 9, 2022 20:49
Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

Looks ok and the CI has no errors, so happy to approve.

@softins softins merged commit bf7dd98 into jamulussoftware:master Jan 10, 2022
@henkdegroot henkdegroot deleted the hide-asio-button-withjack branch January 10, 2022 19:13
@gilgongo gilgongo added this to the 3.8.2 milestone Jan 17, 2022
@pgScorpio pgScorpio mentioned this pull request Mar 13, 2022
5 tasks
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.

Do not show ASIO Device settings and open ASIO settings on JACK version of Jamulus

4 participants