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.

Sign up for GitHub

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

Jump to bottom

soundio: rename SoundDeviceError > SoundDeviceStatus #4907

Merged
merged 2 commits into from
Sep 3, 2022
Merged

soundio: rename SoundDeviceError > SoundDeviceStatus #4907

merged 2 commits into from
Sep 3, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Aug 20, 2022

I found this a bit misleading/annoying that a device status is named "Error", as well as the inadequate names SOUNDDEVICE_ERROR_OK and SOUNDDEVICE_ERROR_ERR

So now it's

enum SoundDeviceStatus {
    SOUNDDEVICE_ERROR = -1,
    SOUNDDEVICE_OK = 0,
    SOUNDDEVICE_ERROR_DUPLICATE_OUTPUT_CHANNEL,
    SOUNDDEVICE_ERROR_EXCESSIVE_OUTPUT_CHANNEL,
    SOUNDDEVICE_ERROR_EXCESSIVE_INPUT_CHANNEL,
    SOUNDDEVICE_ERROR_DEVICE_COUNT
};

@github-actions github-actions bot added the ui label Aug 20, 2022
@ronso0 ronso0 added engine and removed ui labels Aug 20, 2022
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 20, 2022
ferranpujolcamins
ferranpujolcamins approved these changes Aug 21, 2022
View reviewed changes
Copy link
Contributor

@ferranpujolcamins ferranpujolcamins left a comment

Choose a reason for hiding this comment

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

LGTM, only small nitpick that you didn't even introduce, but would be nice if you could fix.

src/soundio/soundmanager.cpp
@@ -562,11 +562,11 @@ SoundDeviceError SoundManager::setConfig(const SoundManagerConfig& config) {
m_pConfig->set(ConfigKey("[Soundcard]","Samplerate"),
ConfigValue(static_cast<int>(m_config.getSampleRate())));

err = setupDevices();
if (err == SOUNDDEVICE_ERROR_OK) {
status = setupDevices();
Copy link
Contributor

Choose a reason for hiding this comment

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

status can just be defined here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sure, I could do that, though I'm touching these files in #4862 anyway, thus I'd rather fix it there and keep this as pure renaming PR.

@ronso0 ronso0 mentioned this pull request Aug 21, 2022
Draft
4 tasks
@ronso0
Copy link
Member Author

ronso0 commented Aug 30, 2022

Anyone else has time for a quick review?

Swiftb0y
Swiftb0y reviewed Aug 30, 2022
View reviewed changes
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.

small nitpick if you're in the mood for cleanup. If you think it blows the scope, we can leave as is.

src/soundio/sounddevicestatus.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added the ui label Aug 31, 2022
Swiftb0y
Swiftb0y approved these changes Aug 31, 2022
View reviewed changes
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.

LGTM otherwise.

src/qml/qmlapplication.cpp Outdated Show resolved Hide resolved
@ronso0 ronso0 force-pushed the sounddeviceerror-status branch from d499e7a to 7303ba3 Compare September 2, 2022 20:55
@ronso0 ronso0 force-pushed the sounddeviceerror-status branch from 7303ba3 to c63b9da Compare September 2, 2022 20:56
@Swiftb0y Swiftb0y merged commit e28e041 into mixxxdj:main Sep 3, 2022
@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 3, 2022

Thanks

@ronso0 ronso0 deleted the sounddeviceerror-status branch September 3, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swiftb0y Swiftb0y Swiftb0y approved these changes

@ferranpujolcamins ferranpujolcamins ferranpujolcamins approved these changes

Assignees
No one assigned
Labels
engine ui
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ronso0 @Swiftb0y @ferranpujolcamins