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

Allow loading custom sound config with '--sound-config' argument #4859

Closed
wants to merge 6 commits into from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jul 18, 2022

first step to resolve https://bugs.launchpad.net/mixxx/+bug/1516035

This allows loading a custom sound config via --sound-config config_file.xml. For now this is restricted to files inside the settings dir.
If the argument not added or the passed file is not readable, default soundconfig.xml is used.
Note: the given config file (command line or default) is used for all read/write operations in the current session, i.e. if you change your config those changes will be stored in your custom sound config.

Thus, the workflow currently goes like this:
Create sound profile

  • connect audio devices and start Mixxx
  • adjust sound devices and options as desired
  • quit Mixxx
  • copy soundconfig.xml and rename to something like soundconfig_DDJ-SB3_NIAudio8.xml

Load sound profile

  • connect audio devices and start Mixxx
  • start Mixxx with mixxx --sound-config soundconfig_DDJ-SB3_NIAudio8.xml

I post-poned adding a profile selector to Preferences > Sound Hardware since that is more elaborate than I first thought. It's mainly getting the UX right, but also some flaws in current DlgPrefSound become obvious when dealing with profiles for the entire page (e.g. some settings are applied instantly).

@github-actions github-actions bot added the ui label Jul 18, 2022
@ronso0 ronso0 added this to the 2.4.0 milestone Jul 18, 2022
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks for splitting into reviewable commits. lgtm for me so far.

src/util/cmdlineargs.cpp Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Jul 18, 2022

I think this is good enough to be mergd (if a second reviewer doesn't spot issues, of course).

The preferences / startup UI integration is a bit more elaborate and I'm unsure I find time to finish that soonish, so I'll do it another branch.

@ronso0 ronso0 marked this pull request as ready for review July 18, 2022 19:17
@ronso0 ronso0 changed the title Allow loading custom sound config Allow loading custom sound config with '--sound-config' argument Jul 18, 2022
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM. Though I still need to do a manual test to verify.

@ronso0
Copy link
Member Author

ronso0 commented Jul 20, 2022

Cool. I didn't do too many tests myself yet. With an arg config it shouldn't even matter if soundconfig.xml exists or not.

@ronso0
Copy link
Member Author

ronso0 commented Aug 2, 2022

Merge?

@Holzhaus
Copy link
Member

Holzhaus commented Aug 2, 2022

Just a suggestion (not blocking merge or something), but if you can't specify full paths anyway, why not just dictate a naming scheme for the sake of shorter command lines?

For example, instead of:

mixxx --sound-config soundconfig_DDJ-SB3_NIAudio8.xml

we could use:

mixxx --sound-config DDJ-SB3_NIAudio8

And mixxx creates the filename from the name automatically?

@ronso0
Copy link
Member Author

ronso0 commented Aug 2, 2022

Sure, that makes sense.
Tbh I consider this PR a temporary solution, so I'd rather implement this in #4862. There, only the basenames are visible in the preferences so we'd (re)construct the file paths anyway.

@ronso0 ronso0 marked this pull request as draft August 3, 2022 12:24
@ronso0
Copy link
Member Author

ronso0 commented Aug 3, 2022

When reworking and simplifying #4862 it turned out that all the file(name) handling should rather happen in SoundManager, thus I suspend this one. Actually, dealing with filenames in general and in particular legacy soundconfig.xml is the bulk of the work :\

@ronso0
Copy link
Member Author

ronso0 commented Aug 10, 2022

I'm closing this. Sound config via command line is part of #4862 now

@ronso0 ronso0 closed this Aug 10, 2022
@ronso0
Copy link
Member Author

ronso0 commented Aug 10, 2022

Thanks for the review @Swiftb0y

@ronso0 ronso0 deleted the soundconfig-selector branch August 10, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants