expose audio output device selection in preferences#464
Merged
nschimme merged 1 commit intoMUME:masterfrom Feb 24, 2026
Merged
Conversation
Reviewer's GuideImplements configurable audio output device selection: the preferences UI now exposes an output device combo box whose selection is persisted in configuration and used to set the QAudioOutput device for both music and sound effects, with automatic updates when configuration or available devices change. Sequence diagram for updating audio output device from preferences to audio managerssequenceDiagram
actor User
participant AudioPage
participant Configuration
participant AudioSettings
participant ChangeMonitor
participant AudioManager
participant MusicManager
participant SfxManager
participant QMediaDevices
User->>AudioPage: Change outputDeviceComboBox selection
AudioPage->>AudioPage: slot_outputDeviceChanged(index)
AudioPage->>AudioSettings: setOutputDeviceId(selectedDeviceId)
AudioSettings->>ChangeMonitor: notifyAll()
ChangeMonitor->>AudioManager: onConfigurationChanged()
AudioManager->>AudioManager: updateOutputDevices()
alt DeviceId_is_empty
AudioManager->>QMediaDevices: defaultAudioOutput()
QMediaDevices-->>AudioManager: targetDevice
else Specific_device_selected
AudioManager->>QMediaDevices: audioOutputs()
QMediaDevices-->>AudioManager: devices
AudioManager->>AudioManager: Find_device_matching_deviceId
AudioManager->>AudioManager: targetDevice = matching_or_default
end
AudioManager->>MusicManager: updateOutputDevice(targetDevice)
AudioManager->>SfxManager: updateOutputDevice(targetDevice)
MusicManager->>MusicManager: Set_device_on_all_channels
SfxManager->>SfxManager: Set_device_on_output
Note over AudioManager,QMediaDevices: QMediaDevices.audioOutputsChanged also triggers updateOutputDevices()
Class diagram for configurable audio output device selectionclassDiagram
class Configuration {
}
class AudioSettings {
-ChangeMonitor m_changeMonitor
-int m_musicVolume
-int m_soundVolume
-QByteArray m_outputDeviceId
-bool m_unlocked
+void read(QSettings &conf)
+void write(QSettings &conf) const
+int getMusicVolume() const
+void setMusicVolume(int value)
+int getSoundVolume() const
+void setSoundVolume(int value)
+const QByteArray &getOutputDeviceId() const
+void setOutputDeviceId(const QByteArray &id)
+bool isUnlocked() const
+void setUnlocked()
}
class AudioManager {
-MusicManager *m_music
-SfxManager *m_sfx
+AudioManager(MediaLibrary &library, GameObserver &observer, QObject *parent)
+~AudioManager()
+void updateVolumes()
+void updateOutputDevices()
}
class MusicManager {
+MusicManager(MediaLibrary &library, QObject *parent)
+void playMusic(const QString &musicFile)
+void stopMusic()
+void updateVolumes()
+void slot_onMediaChanged()
+void updateOutputDevice(const QAudioDevice &device)
}
class SfxManager {
+SfxManager(MediaLibrary &library, QObject *parent)
+void playSound(const QString &soundName)
+void updateVolume()
+void updateOutputDevice(const QAudioDevice &device)
}
class AudioPage {
+AudioPage(QWidget *parent)
+~AudioPage()
+void slot_loadConfig()
+void slot_musicVolumeChanged(int value)
+void slot_soundsVolumeChanged(int value)
+void slot_outputDeviceChanged(int index)
+void slot_updateDevices()
}
class QMediaDevices {
+static QList~QAudioDevice~ audioOutputs()
+static QAudioDevice defaultAudioOutput()
+signal audioOutputsChanged()
}
class QAudioDevice {
+QByteArray id() const
+QString description() const
}
class QAudioOutput {
+void setDevice(const QAudioDevice &device)
+QAudioDevice device() const
}
Configuration *-- AudioSettings : contains
AudioManager --> Configuration : reads_writes_audio_settings
AudioManager *-- MusicManager : owns
AudioManager *-- SfxManager : owns
AudioPage --> Configuration : loads_and_saves_AudioSettings
AudioManager --> QMediaDevices : queries_outputs
MusicManager --> QAudioOutput : uses
SfxManager --> QAudioOutput : uses
QAudioOutput --> QAudioDevice : current_device
AudioPage --> QMediaDevices : populates_device_list
AudioSettings --> QAudioDevice : stores_id_of
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- You’re storing
QAudioDevice::id()(aQByteArray) in the combo box andAudioSettingsas aQString, then comparing it back todevice.id(); consider keeping this consistently asQByteArray(e.g., store/compareQByteArrayor useQVariantwithQByteArray) to avoid subtle issues with implicit conversions or non-text IDs. - There are two different audio-disabling macros in use (
NO_AUDIOandMMAPPER_NO_AUDIO); it would be safer to standardize on one of these in the new code paths so that the UI enablement and the compile-time audio guards cannot diverge.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re storing `QAudioDevice::id()` (a `QByteArray`) in the combo box and `AudioSettings` as a `QString`, then comparing it back to `device.id()`; consider keeping this consistently as `QByteArray` (e.g., store/compare `QByteArray` or use `QVariant` with `QByteArray`) to avoid subtle issues with implicit conversions or non-text IDs.
- There are two different audio-disabling macros in use (`NO_AUDIO` and `MMAPPER_NO_AUDIO`); it would be safer to standardize on one of these in the new code paths so that the UI enablement and the compile-time audio guards cannot diverge.
## Individual Comments
### Comment 1
<location path="src/preferences/audiopage.cpp" line_range="92-101" />
<code_context>
+
+void AudioPage::slot_updateDevices()
+{
+ ui->outputDeviceComboBox->blockSignals(true);
+ QString currentId = ui->outputDeviceComboBox->currentData().toString();
+ if (currentId.isEmpty()) {
+ currentId = getConfig().audio.getOutputDeviceId();
+ }
+
+ ui->outputDeviceComboBox->clear();
+ ui->outputDeviceComboBox->addItem(tr("System Default"), QString());
+
+#ifndef MMAPPER_NO_AUDIO
+ const QList<QAudioDevice> devices = QMediaDevices::audioOutputs();
+ for (const QAudioDevice &device : devices) {
+ ui->outputDeviceComboBox->addItem(device.description(), device.id());
+ }
+#endif
+
+ int index = ui->outputDeviceComboBox->findData(currentId);
+ if (index != -1) {
+ ui->outputDeviceComboBox->setCurrentIndex(index);
+ } else {
+ ui->outputDeviceComboBox->setCurrentIndex(0);
+ }
+ ui->outputDeviceComboBox->blockSignals(false);
+}
</code_context>
<issue_to_address>
**suggestion:** Using RAII for blocking signals would make this more robust against early returns.
This is safe as written, but fragile if future edits add early returns between the two calls. Prefer `QSignalBlocker blocker(ui->outputDeviceComboBox);` to make this robust and exception-safe while keeping intent clear.
Suggested implementation:
```cpp
void AudioPage::slot_updateDevices()
{
QSignalBlocker blocker(ui->outputDeviceComboBox);
const auto &settings = getConfig().audio;
ui->musicVolumeSlider->setValue(settings.getMusicVolume());
ui->soundsVolumeSlider->setValue(settings.getSoundVolume());
QString currentId = ui->outputDeviceComboBox->currentData().toString();
if (currentId.isEmpty()) {
currentId = settings.getOutputDeviceId();
}
ui->outputDeviceComboBox->clear();
ui->outputDeviceComboBox->addItem(tr("System Default"), QString());
#ifndef MMAPPER_NO_AUDIO
const QList<QAudioDevice> devices = QMediaDevices::audioOutputs();
for (const QAudioDevice &device : devices) {
ui->outputDeviceComboBox->addItem(device.description(), device.id());
}
#endif
int index = ui->outputDeviceComboBox->findData(currentId);
if (index != -1) {
ui->outputDeviceComboBox->setCurrentIndex(index);
} else {
ui->outputDeviceComboBox->setCurrentIndex(0);
}
}
```
To compile successfully, ensure the following headers are included at the top of `src/preferences/audiopage.cpp` (or in the corresponding header if that's where other Qt includes live):
1. `#include <QSignalBlocker>`
2. If not already present for the device enumeration:
- `#include <QMediaDevices>`
- `#include <QAudioDevice>`
Place these alongside the existing Qt includes to match the project's include style.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #464 +/- ##
===========================================
- Coverage 66.48% 25.58% -40.90%
===========================================
Files 85 500 +415
Lines 4186 41178 +36992
Branches 255 4479 +4224
===========================================
+ Hits 2783 10537 +7754
- Misses 1403 30641 +29238 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/media/MusicManager.cpp" line_range="213-216" />
<code_context>
}
#ifndef MMAPPER_NO_AUDIO
+void MusicManager::updateOutputDevice(const QAudioDevice &device)
+{
+ for (int i = 0; i < 2; ++i) {
+ if (m_channels[i].audioOutput->device() != device) {
+ m_channels[i].audioOutput->setDevice(device);
+ }
</code_context>
<issue_to_address>
**suggestion:** updateOutputDevice assumes exactly two channels via a magic number
The loop is hard-coded to `for (int i = 0; i < 2; ++i)`, which couples this function to the current channel count. If `m_channels` ever changes size, this will silently misbehave. Please iterate over `std::size(m_channels)` or a named constant representing the channel count instead of using the literal `2`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+213
to
+216
| void MusicManager::updateOutputDevice(const QAudioDevice &device) | ||
| { | ||
| for (int i = 0; i < 2; ++i) { | ||
| if (m_channels[i].audioOutput->device() != device) { |
There was a problem hiding this comment.
suggestion: updateOutputDevice assumes exactly two channels via a magic number
The loop is hard-coded to for (int i = 0; i < 2; ++i), which couples this function to the current channel count. If m_channels ever changes size, this will silently misbehave. Please iterate over std::size(m_channels) or a named constant representing the channel count instead of using the literal 2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add configurable audio output device selection and propagate it through configuration, UI, and audio managers.
New Features:
Enhancements: