Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create separate arguments for input and output devices #858

Merged
merged 2 commits into from Dec 22, 2022
Merged

Create separate arguments for input and output devices #858

merged 2 commits into from Dec 22, 2022

Conversation

maingig
Copy link
Collaborator

@maingig maingig commented Dec 20, 2022

Fix for Issue #495. Use separate arguments for input and output devices.

Fix for Issue #495. Use separate arguments for input and output devices.
@ntonnaett
Copy link
Collaborator

ntonnaett commented Dec 20, 2022

Thanks for your PR! Looks straight forward. Is this tested with device name including a comma?

Could you please rebase on dev! Not really necessary.

@ntonnaett ntonnaett changed the base branch from main to dev December 20, 2022 14:34
@maingig
Copy link
Collaborator Author

maingig commented Dec 20, 2022

I tested with these devices:
"Rogue Amoeba Software, Inc.: Loopback Audio 2"
"Rogue Amoeba Software, Inc.: Loopback Audio 1"

Looks like there was one format issue. I fixed it in my branch. Not sure what to do here.

Fix format issue.
@ntonnaett
Copy link
Collaborator

I'm happy with this. The naming of the flags is nice for when we get tab completion one day. Let's see what others think. @dyfer @psiborg112 @cchafe

@psiborg112
Copy link
Collaborator

Looks good to me. Doesn't prevent people from doing things the current way if they're already in the habit of that (or have existing scripts they're using) while fixing the issue.

@ntonnaett
Copy link
Collaborator

Looks good to me. Doesn't prevent people from doing things the current way if they're already in the habit of that (or have existing scripts they're using) while fixing the issue.

Do you have an idea for this? Counting the commas? May be something for another PR.

@psiborg112
Copy link
Collaborator

Oh no, I just meant that it doesn't stop the current option from working the way people are used to while adding the necessary options to fix the comma issue. I don't think there's a more elegant solution than that.

@ntonnaett
Copy link
Collaborator

Yeah, I agree. But the case for more than one comma for --audiodevice is broken. So we could warn if it's more than 1 and tell the user to use the new flags.

@psiborg112
Copy link
Collaborator

Yes, a warning would probably be good in that circumstance - but definitely for a future PR.

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.

None yet

3 participants