Implement Reset Mixer Settings to Default Button#33799
Open
wjana298 wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis pull request implements a "reset mixer to defaults" feature for the playback system. The implementation spans the controller layer, UI action infrastructure, QML components, and model synchronization. The feature adds a new notification to the IPlaybackController interface, implements resetMixerToDefaults() to reset per-track and master audio parameters, exposes it as a keyboard-triggered UI action (Ctrl+Shift+R), renders a QML button with confirmation dialog, and synchronizes the UI state through the mixer panel model. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/playback/internal/playbackcontroller.cpp`:
- Line 131: The shortcut registration currently calls resetMixerToDefaults
directly via dispatcher()->reg(..., RESET_MIXER_CODE, ...,
&PlaybackController::resetMixerToDefaults); change it to route through the same
confirmation gate used by the button/QML flow: bind RESET_MIXER_CODE to a new or
existing handler (e.g., PlaybackController::requestResetMixerConfirmation or the
exact method the QML button uses) that shows the confirmation dialog and only
calls PlaybackController::resetMixerToDefaults after the user confirms; ensure
the dispatcher()->reg uses that confirmation handler instead of
resetMixerToDefaults so the keyboard shortcut cannot bypass confirmation.
- Around line 961-1033: The resetMixerToDefaults must also restore aux/reverb
channel outputs; update PlaybackController::resetMixerToDefaults to iterate
m_auxTrackIdMap (or the aux IDs from audioSettings()/m_auxTrackIdMap), build
default AuxOutputParams (reset volume/balance/mute/forceMute and set default
send/return values via configuration() or the same defaults used for tracks),
call audioSettings()->setAuxOutputParams(auxInstrumentTrackId, auxParams) for
each aux and push those to playback via
playback()->setAuxControlParams(auxTrackId, auxParams.control()) (and any
equivalent playback API to set aux return/reverb params). Ensure you reference
AuxOutputParams, m_auxTrackIdMap, audioSettings()->auxOutputParams /
setAuxOutputParams, configuration() defaults, and
playback()->setAuxControlParams when making the changes.
In `@src/playback/qml/MuseScore/Playback/MixerPanel.qml`:
- Around line 46-48: The toolbar's onResetMixerRequested currently calls the
local root.resetMixer() and the file contains a local hardcoded reset
implementation (aux defaults in the block around the function used at lines
96-113); instead, remove the local reset logic and route the toolbar click to
the canonical controller action "reset-mixer" so both entry points use the same
controller-computed defaults. Concretely: replace the root.resetMixer() call
path from onResetMixerRequested with invoking the controller's "reset-mixer"
action (or emitting the same signal the shortcut uses) and delete the local
aux-default assignments (isActive = true, audioSignalPercentage = 30) so
defaults are computed only by the controller reset implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2206e34e-8567-404d-a2fe-143f44b05c07
📒 Files selected for processing (13)
src/app/configs/data/shortcuts.xmlsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/playbackcontroller.hsrc/playback/internal/playbackuiactions.cppsrc/playback/internal/playbackuiactions.hsrc/playback/iplaybackcontroller.hsrc/playback/qml/MuseScore/Playback/CMakeLists.txtsrc/playback/qml/MuseScore/Playback/MixerPanel.qmlsrc/playback/qml/MuseScore/Playback/internal/MixerPanelToolbar.qmlsrc/playback/qml/MuseScore/Playback/internal/ResetMixerButton.qmlsrc/playback/qml/MuseScore/Playback/mixerchannelitem.cppsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.cppsrc/playback/tests/mocks/playbackcontrollermock.h
| dispatcher()->reg(this, PLAYBACK_SETUP, this, &PlaybackController::openPlaybackSetupDialog); | ||
| dispatcher()->reg(this, TOGGLE_HEAR_PLAYBACK_WHEN_EDITING_CODE, this, &PlaybackController::toggleHearPlaybackWhenEditing); | ||
| dispatcher()->reg(this, "playback-reload-cache", this, &PlaybackController::reloadPlaybackCache); | ||
| dispatcher()->reg(this, RESET_MIXER_CODE, this, &PlaybackController::resetMixerToDefaults); |
Contributor
There was a problem hiding this comment.
Route the shortcut through the same confirmation gate.
Line 131 binds reset-mixer directly to resetMixerToDefaults(). With the new Ctrl+Shift+R shortcut, that makes the destructive reset execute immediately, while the confirmation in this PR exists only in the button/QML flow. The shortcut needs the same guard or it can wipe mixer state in one keystroke.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/playback/internal/playbackcontroller.cpp` at line 131, The shortcut
registration currently calls resetMixerToDefaults directly via
dispatcher()->reg(..., RESET_MIXER_CODE, ...,
&PlaybackController::resetMixerToDefaults); change it to route through the same
confirmation gate used by the button/QML flow: bind RESET_MIXER_CODE to a new or
existing handler (e.g., PlaybackController::requestResetMixerConfirmation or the
exact method the QML button uses) that shows the confirmation dialog and only
calls PlaybackController::resetMixerToDefaults after the user confirms; ensure
the dispatcher()->reg uses that confirmation handler instead of
resetMixerToDefaults so the keyboard shortcut cannot bypass confirmation.
Comment on lines
+961
to
+1033
| void PlaybackController::resetMixerToDefaults() | ||
| { | ||
| IF_ASSERT_FAILED(audioSettings() && notationPlayback()) { | ||
| return; | ||
| } | ||
|
|
||
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | ||
|
|
||
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | ||
| if (!muse::contains(m_instrumentTrackIdMap, instrumentTrackId)) { | ||
| continue; | ||
| } | ||
|
|
||
| AudioOutputParams outParams = trackOutputParams(instrumentTrackId); | ||
| const bool wasMuted = outParams.muted; | ||
| const bool wasForceMute = outParams.forceMute; | ||
|
|
||
| outParams.volume = 0.f; | ||
| outParams.balance = 0.f; | ||
|
|
||
| const AudioInputParams inParams = audioSettings()->trackInputParams(instrumentTrackId); | ||
| const AudioSourceType sourceType = inParams.isValid() ? inParams.type() : AudioSourceType::Fluid; | ||
| const muse::String instrumentSoundId = inParams.resourceMeta.attributeVal(PLAYBACK_SETUP_DATA_ATTRIBUTE); | ||
|
|
||
| const bool isMetronome = instrumentTrackId == notationPlayback()->metronomeTrackId(); | ||
|
|
||
| if (isMetronome) { | ||
| outParams.muted = wasMuted; | ||
| } else { | ||
| const auto soloMuteState = trackSoloMuteState(instrumentTrackId); | ||
| outParams.solo = soloMuteState.solo; | ||
| outParams.muted = soloMuteState.mute; | ||
| } | ||
| outParams.forceMute = wasForceMute; | ||
|
|
||
| if (!isMetronome) { | ||
| if (outParams.auxSends.empty()) { | ||
| const auto& auxMap = m_auxTrackIdMap; | ||
| for (aux_channel_idx_t idx = 0; idx < static_cast<aux_channel_idx_t>(auxMap.size()); ++idx) { | ||
| gain_t signalAmount = configuration()->defaultAuxSendValue(idx, sourceType, instrumentSoundId); | ||
| outParams.auxSends.emplace_back(AuxSendParams { signalAmount, true }); | ||
| } | ||
| } else { | ||
| for (aux_channel_idx_t idx = 0; idx < static_cast<aux_channel_idx_t>(outParams.auxSends.size()); ++idx) { | ||
| gain_t signalAmount = configuration()->defaultAuxSendValue(idx, sourceType, instrumentSoundId); | ||
| outParams.auxSends[idx] = AuxSendParams { signalAmount, true }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| audioSettings()->setTrackOutputParams(instrumentTrackId, outParams); | ||
|
|
||
| audio::TrackId trackId = m_instrumentTrackIdMap.at(instrumentTrackId); | ||
| playback()->setControlParams(trackId, outParams.control()); | ||
| playback()->setAuxSendsParams(trackId, outParams.auxSends); | ||
| } | ||
|
|
||
| AudioOutputParams masterParams = audioSettings()->masterAudioOutputParams(); | ||
| const bool masterWasMuted = masterParams.muted; | ||
| const bool masterWasForceMute = masterParams.forceMute; | ||
|
|
||
| masterParams.volume = 0.f; | ||
| masterParams.balance = 0.f; | ||
|
|
||
| masterParams.muted = masterWasMuted; | ||
| masterParams.forceMute = masterWasForceMute; | ||
|
|
||
| audioSettings()->setMasterAudioOutputParams(masterParams); | ||
| playback()->setMasterControlParams(masterParams.control()); | ||
|
|
||
| m_mixerResetRequested.notify(); | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
Reset the aux/reverb outputs, not just track sends.
Lines 996-1029 rebuild per-track sends and master control params, but they never restore audioSettings()->auxOutputParams(...) or push default aux-track params back into playback. As written, edits made on the aux/reverb channels themselves survive the reset, so this does not fully return the mixer to the "New-Score" defaults described in the PR objective.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/playback/internal/playbackcontroller.cpp` around lines 961 - 1033, The
resetMixerToDefaults must also restore aux/reverb channel outputs; update
PlaybackController::resetMixerToDefaults to iterate m_auxTrackIdMap (or the aux
IDs from audioSettings()/m_auxTrackIdMap), build default AuxOutputParams (reset
volume/balance/mute/forceMute and set default send/return values via
configuration() or the same defaults used for tracks), call
audioSettings()->setAuxOutputParams(auxInstrumentTrackId, auxParams) for each
aux and push those to playback via playback()->setAuxControlParams(auxTrackId,
auxParams.control()) (and any equivalent playback API to set aux return/reverb
params). Ensure you reference AuxOutputParams, m_auxTrackIdMap,
audioSettings()->auxOutputParams / setAuxOutputParams, configuration() defaults,
and playback()->setAuxControlParams when making the changes.
Comment on lines
+46
to
+48
| onResetMixerRequested: { | ||
| root.resetMixer() | ||
| } |
Contributor
There was a problem hiding this comment.
Route reset through the controller action, not a local QML reset implementation.
Line 46 and Line 47 send the button flow to root.resetMixer(), and Lines 96-113 hardcode aux defaults (isActive = true, audioSignalPercentage = 30). This can diverge from the canonical reset behavior used by the "reset-mixer" action/shortcut, where defaults are computed in controller logic per track/source. Please wire the toolbar reset to the same "reset-mixer" action path and remove the local reset logic so both entry points produce identical defaults.
Also applies to: 96-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/playback/qml/MuseScore/Playback/MixerPanel.qml` around lines 46 - 48, The
toolbar's onResetMixerRequested currently calls the local root.resetMixer() and
the file contains a local hardcoded reset implementation (aux defaults in the
block around the function used at lines 96-113); instead, remove the local reset
logic and route the toolbar click to the canonical controller action
"reset-mixer" so both entry points use the same controller-computed defaults.
Concretely: replace the root.resetMixer() call path from onResetMixerRequested
with invoking the controller's "reset-mixer" action (or emitting the same signal
the shortcut uses) and delete the local aux-default assignments (isActive =
true, audioSignalPercentage = 30) so defaults are computed only by the
controller reset implementation.
Contributor
|
Hmm, as mentioned in the issue discussion we would really prefer to have undo/redo supported in the mixer before trying to implement this. That's a pretty serious undertaking - I certainly wouldn't try to implement that as an afterthought to this task. I recognise that the popup you've added scoots around this a little but overall I'm not sure - I'll raise this internally. |
Added button and shortcut that revert all changes in the Mixer Workspace Panel back to "New-Score" defaults (Volume, Pan, Aux sends and Reverb). When the button is pressed, a pop-up window appears asking to confirm the reset, since the changes will be permanent. Closes musescore#24685 Co-authored-by: Miguel Sousa <miguel.de.sousa@tecnico.ulisboa.pt>
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
3 participants
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.
Resolves: #24685
Added button and shortcut (Crtl+Shift+R) that reverts all changes in the Mixer Workspace Panel back to "New-Score" defaults (Volume, Pan, Aux sends and Reverb).
When the button is pressed, a pop-up window appears asking to confirm the reset, since the changes will be permanent.