Issue #22767: Implement Unmute-all and Unsolo-all buttons to mixer#33798
Issue #22767: Implement Unmute-all and Unsolo-all buttons to mixer#33798pombcoraft wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds two mixer controls—unmute-all (Ctrl+Shift+U) and unsolo-all (Ctrl+Shift+L): keyboard shortcuts (standard, AZERTY, mac), PlaybackController methods and dispatcher registrations, UI actions, a new MixerUnMuteAndUnSoloSection QML component and MixerPanel layout integration, a MixerPanelContextMenuModel visibility property and signal, mixer-model synchronization for solo/mute updates, and a small tidy_file.sh tooling update. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/app/configs/data/shortcuts_azerty.xml`:
- Around line 863-867: The shortcut conflict: the <key>unmute-all</key> binding
uses Ctrl+Shift+M which duplicates the existing <key>toggle-mmrest</key>
binding; change the <seq> value for unmute-all (the <SC> entry with key
"unmute-all") to an unused combination (e.g., Ctrl+Shift+U) so it no longer
collides with the "toggle-mmrest" shortcut and update any related
documentation/tests referencing the old binding.
In `@src/app/configs/data/shortcuts_mac.xml`:
- Around line 822-826: The shortcut for the <key>unmute-all</key> action
conflicts with <key>toggle-mmrest</key> because both use
<seq>Ctrl+Shift+M</seq>; update the <seq> value inside the <SC> block for
<key>unmute-all</key> to the same alternative shortcut used in the AZERTY and
standard layouts (replace the current "Ctrl+Shift+M" string), ensuring the new
sequence matches the other layouts so the conflict is resolved and behavior is
consistent.
In `@src/app/configs/data/shortcuts.xml`:
- Around line 822-826: The new keyboard shortcut for the action with key
"unmute-all" currently uses the same chord as the existing "toggle-mmrest" (both
have <seq>Ctrl+Shift+M</seq>), causing a collision; pick a different,
non-conflicting <seq> value for either "unmute-all" or "toggle-mmrest" (e.g.,
change the <seq> under the SC entry for key "unmute-all" to an alternative
chord) and ensure the updated chord is unique across shortcuts in shortcuts.xml
so both actions remain reachable.
In `@src/playback/internal/playbackcontroller.cpp`:
- Around line 963-997: Add null-safety guards at the start of both
PlaybackController::unmuteAll() and PlaybackController::unsoloAll(): verify
notationPlayback() and m_notation (as used) are non-null (or use
IF_ASSERT_FAILED macros like other methods) and return early if the checks fail;
then proceed to call existingTrackIdSet(), m_notation->soloMuteState(),
trackSoloMuteState(), setTrackSoloMuteState(...) and updateSoloMuteStates() as
before. Ensure you reference the same null-check pattern used in
updateCurrentTempo() and seekElement() so behavior and logging/assertion are
consistent.
- Around line 983-997: The unsoloAll() implementation is inconsistent with
unmuteAll(): it must skip the metronome track like unmuteAll() does and also fix
local style issues; update PlaybackController::unsoloAll() to check and skip the
metronome (use the same check used in unmuteAll()), rename the local variable
new_state to newState to follow camelCase, remove the redundant "== true" so the
condition is simply if (newState.solo), and ensure spacing after keywords and
commas (e.g., if (newState.solo) and setTrackSoloMuteState(instrumentTrackId,
newState)); keep the call to updateSoloMuteStates() at the end.
- Around line 963-981: In PlaybackController::unmuteAll rename the local
variable new_state to newState (use camelCase), change the conditional from
if(new_state.mute == true) to if (newState.mute) (add a space after the if and
use the simpler boolean check), and ensure proper spacing in the call
setTrackSoloMuteState(instrumentTrackId,new_state) by adding a space after the
comma and updating the identifier to setTrackSoloMuteState(instrumentTrackId,
newState); keep the rest of the logic (notationPlayback(), metronomeTrackId(),
INotationSoloMuteState::SoloMuteState, updateSoloMuteStates()) unchanged.
In
`@src/playback/qml/MuseScore/Playback/internal/MixerUnMuteAndUnSoloSection.qml`:
- Around line 60-62: In MixerUnMuteAndUnSoloSection.qml remove uses of the
undefined content object (replace navigation.accessible.name with a direct
qsTrc("playback", "Unmute All") string), add on the root: property int
navigationRowStart: 0 and signal navigateControlIndexChanged(var index) so
root.navigationRowStart and the signal exist, and remove any other references to
content; then in MixerPanel.qml where the UnMuteAndUnSolo section is
instantiated, pass navigationRowStart (e.g. 650), add needReadChannelName:
prv.isPanelActivated, and wire the signal with onNavigateControlIndexChanged:
function(index) { prv.setNavigateControlIndex(index) } to match the pattern used
by soundSection and fxSection.
- Line 94: The component references an undefined identifier `content` (e.g.
navigation.panel: content.channelItem.panel and content.accessibleName); replace
those with direct, local properties: set the component's accessibleName to a
literal translation such as qsTrc("playback", "Mute and Solo controls") on the
root (MixerUnMuteAndUnSoloSection) and update navigation.panel to use the local
channelItem.panel (or another local panel property) instead of
content.channelItem.panel so no `content` identifier is required.
🪄 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: 14fe7254-aab5-4604-a415-71eb6a95f56d
📒 Files selected for processing (15)
src/app/configs/data/shortcuts.xmlsrc/app/configs/data/shortcuts_azerty.xmlsrc/app/configs/data/shortcuts_mac.xmlsrc/playback/internal/playbackconfiguration.cppsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/playbackcontroller.hsrc/playback/internal/playbackuiactions.cppsrc/playback/internal/playbackuiactions.hsrc/playback/playbacktypes.hsrc/playback/qml/MuseScore/Playback/CMakeLists.txtsrc/playback/qml/MuseScore/Playback/MixerPanel.qmlsrc/playback/qml/MuseScore/Playback/internal/MixerUnMuteAndUnSoloSection.qmlsrc/playback/qml/MuseScore/Playback/mixerpanelcontextmenumodel.cppsrc/playback/qml/MuseScore/Playback/mixerpanelcontextmenumodel.hsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.cpp
| void PlaybackController::unmuteAll() | ||
| { | ||
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | ||
|
|
||
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | ||
| if (instrumentTrackId == notationPlayback()->metronomeTrackId()) { | ||
| continue; | ||
| } | ||
|
|
||
| INotationSoloMuteState::SoloMuteState new_state = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId); | ||
| if(new_state.mute == true) { | ||
| new_state.mute = false; | ||
|
|
||
| setTrackSoloMuteState(instrumentTrackId,new_state); | ||
| } | ||
| } | ||
|
|
||
| updateSoloMuteStates(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Code style issues in unmuteAll().
Several style inconsistencies:
- Line 972: Variable
new_stateuses snake_case instead of camelCase (codebase convention) - Line 973: Redundant comparison
if(new_state.mute == true)should beif (new_state.mute), plus missing space afterif - Line 976: Missing space after comma in function call
♻️ Proposed fix for style issues
- INotationSoloMuteState::SoloMuteState new_state = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId);
- if(new_state.mute == true) {
+ INotationSoloMuteState::SoloMuteState newState = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId);
+ if (newState.mute) {
- new_state.mute = false;
+ newState.mute = false;
- setTrackSoloMuteState(instrumentTrackId,new_state);
+ setTrackSoloMuteState(instrumentTrackId, newState);
}🤖 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 963 - 981, In
PlaybackController::unmuteAll rename the local variable new_state to newState
(use camelCase), change the conditional from if(new_state.mute == true) to if
(newState.mute) (add a space after the if and use the simpler boolean check),
and ensure proper spacing in the call
setTrackSoloMuteState(instrumentTrackId,new_state) by adding a space after the
comma and updating the identifier to setTrackSoloMuteState(instrumentTrackId,
newState); keep the rest of the logic (notationPlayback(), metronomeTrackId(),
INotationSoloMuteState::SoloMuteState, updateSoloMuteStates()) unchanged.
| void PlaybackController::unmuteAll() | ||
| { | ||
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | ||
|
|
||
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | ||
| if (instrumentTrackId == notationPlayback()->metronomeTrackId()) { | ||
| continue; | ||
| } | ||
|
|
||
| INotationSoloMuteState::SoloMuteState new_state = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId); | ||
| if(new_state.mute == true) { | ||
| new_state.mute = false; | ||
|
|
||
| setTrackSoloMuteState(instrumentTrackId,new_state); | ||
| } | ||
| } | ||
|
|
||
| updateSoloMuteStates(); | ||
| } | ||
|
|
||
| void PlaybackController::unsoloAll() | ||
| { | ||
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | ||
|
|
||
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | ||
|
|
||
| INotationSoloMuteState::SoloMuteState new_state = trackSoloMuteState(instrumentTrackId); | ||
| if(new_state.solo == true) { | ||
| new_state.solo = false; | ||
| setTrackSoloMuteState(instrumentTrackId,new_state); | ||
| } | ||
| } | ||
|
|
||
| updateSoloMuteStates(); | ||
| } |
There was a problem hiding this comment.
Missing null safety checks in unmuteAll() and unsoloAll().
Both methods call notationPlayback()->existingTrackIdSet() (Lines 965, 985) and m_notation->soloMuteState() (Line 972) without checking if notationPlayback() or m_notation are non-null. Other methods in this file (e.g., updateCurrentTempo() at Line 178, seekElement() at Line 425) guard these calls with IF_ASSERT_FAILED or early returns.
🛡️ Proposed fix to add guards
void PlaybackController::unmuteAll()
{
+ IF_ASSERT_FAILED(notationPlayback() && m_notation) {
+ return;
+ }
+
InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet();
...
}
void PlaybackController::unsoloAll()
{
+ IF_ASSERT_FAILED(notationPlayback()) {
+ return;
+ }
+
InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet();
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void PlaybackController::unmuteAll() | |
| { | |
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | |
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | |
| if (instrumentTrackId == notationPlayback()->metronomeTrackId()) { | |
| continue; | |
| } | |
| INotationSoloMuteState::SoloMuteState new_state = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId); | |
| if(new_state.mute == true) { | |
| new_state.mute = false; | |
| setTrackSoloMuteState(instrumentTrackId,new_state); | |
| } | |
| } | |
| updateSoloMuteStates(); | |
| } | |
| void PlaybackController::unsoloAll() | |
| { | |
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | |
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | |
| INotationSoloMuteState::SoloMuteState new_state = trackSoloMuteState(instrumentTrackId); | |
| if(new_state.solo == true) { | |
| new_state.solo = false; | |
| setTrackSoloMuteState(instrumentTrackId,new_state); | |
| } | |
| } | |
| updateSoloMuteStates(); | |
| } | |
| void PlaybackController::unmuteAll() | |
| { | |
| IF_ASSERT_FAILED(notationPlayback() && m_notation) { | |
| return; | |
| } | |
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | |
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | |
| if (instrumentTrackId == notationPlayback()->metronomeTrackId()) { | |
| continue; | |
| } | |
| INotationSoloMuteState::SoloMuteState new_state = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId); | |
| if(new_state.mute == true) { | |
| new_state.mute = false; | |
| setTrackSoloMuteState(instrumentTrackId,new_state); | |
| } | |
| } | |
| updateSoloMuteStates(); | |
| } | |
| void PlaybackController::unsoloAll() | |
| { | |
| IF_ASSERT_FAILED(notationPlayback()) { | |
| return; | |
| } | |
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | |
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | |
| INotationSoloMuteState::SoloMuteState new_state = trackSoloMuteState(instrumentTrackId); | |
| if(new_state.solo == true) { | |
| new_state.solo = false; | |
| setTrackSoloMuteState(instrumentTrackId,new_state); | |
| } | |
| } | |
| updateSoloMuteStates(); | |
| } |
🤖 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 963 - 997, Add
null-safety guards at the start of both PlaybackController::unmuteAll() and
PlaybackController::unsoloAll(): verify notationPlayback() and m_notation (as
used) are non-null (or use IF_ASSERT_FAILED macros like other methods) and
return early if the checks fail; then proceed to call existingTrackIdSet(),
m_notation->soloMuteState(), trackSoloMuteState(), setTrackSoloMuteState(...)
and updateSoloMuteStates() as before. Ensure you reference the same null-check
pattern used in updateCurrentTempo() and seekElement() so behavior and
logging/assertion are consistent.
| void PlaybackController::unsoloAll() | ||
| { | ||
| InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); | ||
|
|
||
| for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { | ||
|
|
||
| INotationSoloMuteState::SoloMuteState new_state = trackSoloMuteState(instrumentTrackId); | ||
| if(new_state.solo == true) { | ||
| new_state.solo = false; | ||
| setTrackSoloMuteState(instrumentTrackId,new_state); | ||
| } | ||
| } | ||
|
|
||
| updateSoloMuteStates(); | ||
| } |
There was a problem hiding this comment.
Inconsistent metronome handling between unmuteAll() and unsoloAll().
unmuteAll() (Line 968-970) explicitly skips the metronome track, but unsoloAll() does not. While the QML UI behavior mirrors this (see MixerUnMuteAndUnSoloSection.qml:48-109), the inconsistency suggests a design gap: should solo operations on the metronome be allowed at all?
Additionally, the same code-style issues apply here: snake_case naming, redundant bool comparison, missing space after if (Line 990), and missing space after comma (Line 992).
♻️ Proposed fix for consistency and style
Add metronome skip for symmetry and apply style fixes:
void PlaybackController::unsoloAll()
{
InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet();
for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) {
-
- INotationSoloMuteState::SoloMuteState new_state = trackSoloMuteState(instrumentTrackId);
- if(new_state.solo == true) {
- new_state.solo = false;
- setTrackSoloMuteState(instrumentTrackId,new_state);
+ if (instrumentTrackId == notationPlayback()->metronomeTrackId()) {
+ continue;
+ }
+
+ INotationSoloMuteState::SoloMuteState newState = trackSoloMuteState(instrumentTrackId);
+ if (newState.solo) {
+ newState.solo = false;
+ setTrackSoloMuteState(instrumentTrackId, newState);
}
}🤖 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 983 - 997, The
unsoloAll() implementation is inconsistent with unmuteAll(): it must skip the
metronome track like unmuteAll() does and also fix local style issues; update
PlaybackController::unsoloAll() to check and skip the metronome (use the same
check used in unmuteAll()), rename the local variable new_state to newState to
follow camelCase, remove the redundant "== true" so the condition is simply if
(newState.solo), and ensure spacing after keywords and commas (e.g., if
(newState.solo) and setTrackSoloMuteState(instrumentTrackId, newState)); keep
the call to updateSoloMuteStates() at the end.
| navigation.panel: content.channelItem.panel | ||
| navigation.row: root.navigationRowStart | ||
| navigation.accessible.name: content.accessibleName + " " + qsTrc("playback", "Unmute All") |
There was a problem hiding this comment.
Incomplete navigation/property contract across MixerUnMuteAndUnSoloSection.qml and MixerPanel.qml.
MixerUnMuteAndUnSoloSection.qml references content (undefined), root.navigationRowStart (not declared), and emits navigateControlIndexChanged (signal not declared). Meanwhile, MixerPanel.qml instantiates the section without providing navigationRowStart or handling onNavigateControlIndexChanged. The shared root cause is an incomplete copy-paste adaptation from an existing mixer section (likely MixerMuteAndSoloSection).
In MixerUnMuteAndUnSoloSection.qml:
- Remove all references to
content(lines 60–62, 94–96) and replace with a direct accessible-name string. - Declare
property int navigationRowStart: 0andsignal navigateControlIndexChanged(var index)onroot.
In MixerPanel.qml (lines 329–336):
- Add
navigationRowStart: <value>(e.g., 650),needReadChannelName: prv.isPanelActivated, andonNavigateControlIndexChanged: function(index) { prv.setNavigateControlIndex(index) }.
Match the pattern used by soundSection, fxSection, and the other mixer sections in MixerPanel.qml.
🤖 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/internal/MixerUnMuteAndUnSoloSection.qml`
around lines 60 - 62, In MixerUnMuteAndUnSoloSection.qml remove uses of the
undefined content object (replace navigation.accessible.name with a direct
qsTrc("playback", "Unmute All") string), add on the root: property int
navigationRowStart: 0 and signal navigateControlIndexChanged(var index) so
root.navigationRowStart and the signal exist, and remove any other references to
content; then in MixerPanel.qml where the UnMuteAndUnSolo section is
instantiated, pass navigationRowStart (e.g. 650), add needReadChannelName:
prv.isPanelActivated, and wire the signal with onNavigateControlIndexChanged:
function(index) { prv.setNavigateControlIndex(index) } to match the pattern used
by soundSection and fxSection.
This commit adds a button to unmute all muted channels in the mixer and another to unsolo all soloed channels in the mixer. Removing the need to do these actions to each channel one by one. It also adds a shortcut to unmute all (ctrl+shift+m) and a shortcut to unsolo all (ctrl+shift+l) following the same logic as the buttons. Closes: musescore#22767 Co-authored-by: Joana Guia <joana.a.guia@tecnico.ulisboa.pt>
015dfbc to
8b146a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/playback/internal/playbackcontroller.cpp (3)
968-970:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent metronome handling between
unmuteAll()andunsoloAll().
unmuteAll()explicitly skips the metronome track (lines 968-970 insrc/playback/internal/playbackcontroller.cpp), butunsoloAll()(lines 987-993 insrc/playback/internal/playbackcontroller.cpp) does not. For symmetry and to prevent unexpected behavior, both methods should handle the metronome consistently.♻️ Proposed fix to add metronome skip in `unsoloAll()`
void PlaybackController::unsoloAll() { IF_ASSERT_FAILED(notationPlayback() && m_notation) { return; } InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { + if (instrumentTrackId == notationPlayback()->metronomeTrackId()) { + continue; + } + INotationSoloMuteState::SoloMuteState newState = trackSoloMuteState(instrumentTrackId); if (newState.solo) { newState.solo = false; setTrackSoloMuteState(instrumentTrackId, newState); } } updateSoloMuteStates(); }🤖 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 968 - 970, unmuteAll() currently skips the metronome by checking if instrumentTrackId == notationPlayback()->metronomeTrackId(), but unsoloAll() does not; update unsoloAll() to perform the same check and continue (skip) when the current instrumentTrackId equals notationPlayback()->metronomeTrackId() so both methods handle the metronome consistently. Locate the loop in unsoloAll() that iterates instrumentTrackId (the same iteration context as in unmuteAll()) and add the identical metronome skip condition using notationPlayback()->metronomeTrackId().
963-981:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing null safety checks and redundant boolean comparison.
Both issues flagged in previous review remain:
- No null checks for
notationPlayback()(line 965) orm_notation(line 972) before dereferencing. Other methods in this file (e.g.,updateCurrentTempo()at line 178,seekElement()at line 425) guard these calls.- Line 973: Redundant
== truecomparison; preferif (newState.mute).🛡️ Proposed fix
void PlaybackController::unmuteAll() { + IF_ASSERT_FAILED(notationPlayback() && m_notation) { + return; + } + InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { if (instrumentTrackId == notationPlayback()->metronomeTrackId()) { continue; } INotationSoloMuteState::SoloMuteState newState = m_notation->soloMuteState()->trackSoloMuteState(instrumentTrackId); - if (newState.mute == true) { + if (newState.mute) { newState.mute = false; setTrackSoloMuteState(instrumentTrackId, newState); } } updateSoloMuteStates(); }🤖 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 963 - 981, PlaybackController::unmuteAll lacks null-safety and uses a redundant boolean comparison; before calling notationPlayback() or dereferencing m_notation, guard with early returns (or null checks) similar to updateCurrentTempo()/seekElement so you skip processing if notationPlayback() or m_notation is null, and replace "if (newState.mute == true)" with "if (newState.mute)"; ensure you still skip metronomeTrackId(), call setTrackSoloMuteState(instrumentTrackId, newState) and finally call updateSoloMuteStates() only after the null checks.
983-996:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing null safety checks and redundant boolean comparison.
Same issues as
unmuteAll():
- No null check for
notationPlayback()(line 985) before callingexistingTrackIdSet(). The call totrackSoloMuteState()at line 988 also dereferencesm_notation(see line 327) without a guard.- Line 989: Redundant
== truecomparison; preferif (newState.solo).🛡️ Proposed fix
void PlaybackController::unsoloAll() { + IF_ASSERT_FAILED(notationPlayback() && m_notation) { + return; + } + InstrumentTrackIdSet existingTrackIdSet = notationPlayback()->existingTrackIdSet(); for (const InstrumentTrackId& instrumentTrackId : existingTrackIdSet) { INotationSoloMuteState::SoloMuteState newState = trackSoloMuteState(instrumentTrackId); - if (newState.solo == true) { + if (newState.solo) { newState.solo = false; setTrackSoloMuteState(instrumentTrackId, newState); } } updateSoloMuteStates(); }🤖 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 983 - 996, PlaybackController::unsoloAll lacks null checks and uses a redundant boolean comparison; guard calls to notationPlayback() (and any downstream methods that dereference m_notation such as trackSoloMuteState) by returning early if notationPlayback() is null, then iterate existingTrackIdSet; replace the redundant "if (newState.solo == true)" with "if (newState.solo)"; keep calls to setTrackSoloMuteState(instrumentTrackId, newState) and updateSoloMuteStates() unchanged but only run them when notationPlayback() is valid.
🤖 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/qml/MuseScore/Playback/MixerPanel.qml`:
- Around line 329-336: The MixerUnMuteAndUnSoloSection instance is missing
required navigation properties used internally (causing runtime errors); set the
navigation props on this instance: provide channelItem.panel by wiring
channelItem.panel: root.channelItem.panel, set navigationRowStart (e.g.
navigationRowStart: 650 to sit between the mute/solo and title sections),
forward accessibleName: accessibleName: root.accessibleName, and hook the
navigation signal with onNavigateControlIndexChanged:
root.navigateControlIndexChanged; also add needReadChannelName: true if the
component expects it. Ensure MixerUnMuteAndUnSoloSection.qml declares these
properties/signals if not already.
In `@src/playback/qml/MuseScore/Playback/mixerpanelcontextmenumodel.cpp`:
- Line 52: The string returned for MixerSectionType::UnMuteAndUnSolo uses
inconsistent capitalization ("Unmute and Unsolo"); update the TranslatableString
in the corresponding case in mixerpanelcontextmenumodel.cpp to use "Unmute and
unsolo" so it matches the lowercase "solo" used in the "Mute and solo" entry
(change the second word to lowercase "unsolo" in the TranslatableString call).
In `@tools/codestyle/tidy_file.sh`:
- Around line 18-19: The tidy_file.sh script currently sets
UNCRUSTIFY_CONFIG="${REPO_ROOT}/muse/tools/codestyle/uncrustify_muse.cfg" and
assumes the submodule file exists; update tidy_file.sh (and mention
hooks/pre-commit which calls it) to check for the existence of the
UNCRUSTIFY_CONFIG path and exit with a clear actionable message if missing
(e.g., instructing to init/update the muse_framework submodule or run a download
step), or implement an automatic fallback that downloads or copies the config
(reuse the logic/pattern from buildscripts/tools/codestyle/download.cmake)
before invoking uncrustify; reference the REPO_ROOT and UNCRUSTIFY_CONFIG
variables so the check/fallback is implemented using those symbols.
---
Duplicate comments:
In `@src/playback/internal/playbackcontroller.cpp`:
- Around line 968-970: unmuteAll() currently skips the metronome by checking if
instrumentTrackId == notationPlayback()->metronomeTrackId(), but unsoloAll()
does not; update unsoloAll() to perform the same check and continue (skip) when
the current instrumentTrackId equals notationPlayback()->metronomeTrackId() so
both methods handle the metronome consistently. Locate the loop in unsoloAll()
that iterates instrumentTrackId (the same iteration context as in unmuteAll())
and add the identical metronome skip condition using
notationPlayback()->metronomeTrackId().
- Around line 963-981: PlaybackController::unmuteAll lacks null-safety and uses
a redundant boolean comparison; before calling notationPlayback() or
dereferencing m_notation, guard with early returns (or null checks) similar to
updateCurrentTempo()/seekElement so you skip processing if notationPlayback() or
m_notation is null, and replace "if (newState.mute == true)" with "if
(newState.mute)"; ensure you still skip metronomeTrackId(), call
setTrackSoloMuteState(instrumentTrackId, newState) and finally call
updateSoloMuteStates() only after the null checks.
- Around line 983-996: PlaybackController::unsoloAll lacks null checks and uses
a redundant boolean comparison; guard calls to notationPlayback() (and any
downstream methods that dereference m_notation such as trackSoloMuteState) by
returning early if notationPlayback() is null, then iterate existingTrackIdSet;
replace the redundant "if (newState.solo == true)" with "if (newState.solo)";
keep calls to setTrackSoloMuteState(instrumentTrackId, newState) and
updateSoloMuteStates() unchanged but only run them when notationPlayback() is
valid.
🪄 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: c953c180-e1e3-4449-9ea7-4693a7e76a94
📒 Files selected for processing (16)
src/app/configs/data/shortcuts.xmlsrc/app/configs/data/shortcuts_azerty.xmlsrc/app/configs/data/shortcuts_mac.xmlsrc/playback/internal/playbackconfiguration.cppsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/playbackcontroller.hsrc/playback/internal/playbackuiactions.cppsrc/playback/internal/playbackuiactions.hsrc/playback/playbacktypes.hsrc/playback/qml/MuseScore/Playback/CMakeLists.txtsrc/playback/qml/MuseScore/Playback/MixerPanel.qmlsrc/playback/qml/MuseScore/Playback/internal/MixerUnMuteAndUnSoloSection.qmlsrc/playback/qml/MuseScore/Playback/mixerpanelcontextmenumodel.cppsrc/playback/qml/MuseScore/Playback/mixerpanelcontextmenumodel.hsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.cpptools/codestyle/tidy_file.sh
✅ Files skipped from review due to trivial changes (1)
- src/playback/qml/MuseScore/Playback/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (10)
- src/playback/internal/playbackcontroller.h
- src/app/configs/data/shortcuts_azerty.xml
- src/app/configs/data/shortcuts_mac.xml
- src/playback/playbacktypes.h
- src/playback/internal/playbackuiactions.h
- src/playback/internal/playbackconfiguration.cpp
- src/playback/qml/MuseScore/Playback/mixerpanelcontextmenumodel.h
- src/playback/qml/MuseScore/Playback/mixerpanelmodel.cpp
- src/playback/internal/playbackuiactions.cpp
- src/playback/qml/MuseScore/Playback/internal/MixerUnMuteAndUnSoloSection.qml
| MixerUnMuteAndUnSoloSection { | ||
| anchors.left: parent.left | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| visible: contextMenuModel.unMuteAndUnSoloSectionVisible | ||
| headerVisible: contextMenuModel.labelsSectionVisible | ||
| headerWidth: prv.headerWidth | ||
| model: mixerPanelModel | ||
| } |
There was a problem hiding this comment.
Missing required navigation properties causes runtime error and breaks keyboard navigation.
The MixerUnMuteAndUnSoloSection component references root.channelItem.panel, root.navigationRowStart, root.accessibleName, and root.navigateControlIndexChanged in its button navigation setup (see src/playback/qml/MuseScore/Playback/internal/MixerUnMuteAndUnSoloSection.qml lines 40, 42, 44, 68, 70, 72), but none of these properties are set here. Accessing undefined root.channelItem.panel will cause a QML runtime error when keyboard navigation activates. Additionally, the missing onNavigateControlIndexChanged handler breaks navigation tracking, and the missing navigationRowStart breaks keyboard navigation sequencing.
Every other mixer section in this file (sound, fx, auxSends, balance, volume, fader, muteAndSolo, title) sets navigationRowStart and wires onNavigateControlIndexChanged.
🔧 Proposed fix to add missing navigation properties
MixerUnMuteAndUnSoloSection {
anchors.left: parent.left
anchors.verticalCenter: parent.verticalCenter
visible: contextMenuModel.unMuteAndUnSoloSectionVisible
headerVisible: contextMenuModel.labelsSectionVisible
headerWidth: prv.headerWidth
model: mixerPanelModel
+ navigationRowStart: 650
+ needReadChannelName: prv.isPanelActivated
+
+ onNavigateControlIndexChanged: function(index) {
+ prv.setNavigateControlIndex(index)
+ }
}Note: navigationRowStart: 650 is suggested to place it between MixerMuteAndSoloSection (600) and MixerTitleSection (700). You may also need to verify that MixerUnMuteAndUnSoloSection.qml declares the required properties (navigationRowStart, needReadChannelName, signal navigateControlIndexChanged) if they are not already present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MixerUnMuteAndUnSoloSection { | |
| anchors.left: parent.left | |
| anchors.verticalCenter: parent.verticalCenter | |
| visible: contextMenuModel.unMuteAndUnSoloSectionVisible | |
| headerVisible: contextMenuModel.labelsSectionVisible | |
| headerWidth: prv.headerWidth | |
| model: mixerPanelModel | |
| } | |
| MixerUnMuteAndUnSoloSection { | |
| anchors.left: parent.left | |
| anchors.verticalCenter: parent.verticalCenter | |
| visible: contextMenuModel.unMuteAndUnSoloSectionVisible | |
| headerVisible: contextMenuModel.labelsSectionVisible | |
| headerWidth: prv.headerWidth | |
| model: mixerPanelModel | |
| navigationRowStart: 650 | |
| needReadChannelName: prv.isPanelActivated | |
| onNavigateControlIndexChanged: function(index) { | |
| prv.setNavigateControlIndex(index) | |
| } | |
| } |
🤖 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 329 - 336,
The MixerUnMuteAndUnSoloSection instance is missing required navigation
properties used internally (causing runtime errors); set the navigation props on
this instance: provide channelItem.panel by wiring channelItem.panel:
root.channelItem.panel, set navigationRowStart (e.g. navigationRowStart: 650 to
sit between the mute/solo and title sections), forward accessibleName:
accessibleName: root.accessibleName, and hook the navigation signal with
onNavigateControlIndexChanged: root.navigateControlIndexChanged; also add
needReadChannelName: true if the component expects it. Ensure
MixerUnMuteAndUnSoloSection.qml declares these properties/signals if not
already.
| case MixerSectionType::Fader: return TranslatableString("playback", "Fader"); | ||
| case MixerSectionType::MuteAndSolo: return TranslatableString("playback", "Mute and solo"); | ||
| case MixerSectionType::Title: return TranslatableString("playback", "Name"); | ||
| case MixerSectionType::UnMuteAndUnSolo: return TranslatableString("playback", "Unmute and Unsolo"); |
There was a problem hiding this comment.
Inconsistent capitalization in section title.
The title "Unmute and Unsolo" capitalizes "Unsolo", but line 50 uses lowercase "solo" in "Mute and solo". For consistency, this should be "Unmute and unsolo".
✏️ Proposed fix
- case MixerSectionType::UnMuteAndUnSolo: return TranslatableString("playback", "Unmute and Unsolo");
+ case MixerSectionType::UnMuteAndUnSolo: return TranslatableString("playback", "Unmute and unsolo");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case MixerSectionType::UnMuteAndUnSolo: return TranslatableString("playback", "Unmute and Unsolo"); | |
| case MixerSectionType::UnMuteAndUnSolo: return TranslatableString("playback", "Unmute and unsolo"); |
🤖 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/mixerpanelcontextmenumodel.cpp` at line
52, The string returned for MixerSectionType::UnMuteAndUnSolo uses inconsistent
capitalization ("Unmute and Unsolo"); update the TranslatableString in the
corresponding case in mixerpanelcontextmenumodel.cpp to use "Unmute and unsolo"
so it matches the lowercase "solo" used in the "Mute and solo" entry (change the
second word to lowercase "unsolo" in the TranslatableString call).
| REPO_ROOT="${HERE}/../.." | ||
| UNCRUSTIFY_CONFIG="${REPO_ROOT}/muse/tools/codestyle/uncrustify_muse.cfg" # lives in framework submodule |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify uncrustify_muse.cfg exists at the new hardcoded path
# Check if the config file exists at the new path
if [ -f "muse/tools/codestyle/uncrustify_muse.cfg" ]; then
echo "✓ Config file found at muse/tools/codestyle/uncrustify_muse.cfg"
else
echo "✗ Config file NOT found at muse/tools/codestyle/uncrustify_muse.cfg"
echo "Searching for uncrustify_muse.cfg anywhere in the repository:"
fd -t f "uncrustify_muse.cfg"
fi
# Also check if the old config path exists
if [ -f "tools/codestyle/uncrustify_musescore.cfg" ]; then
echo "ℹ Old config file still exists at tools/codestyle/uncrustify_musescore.cfg"
fiRepository: musescore/MuseScore
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate uncrustify_muse*.cfg files =="
fd -t f -a "uncrustify_muse*.cfg" . || true
echo
echo "== Locate uncrustify_musescore.cfg =="
fd -t f -a "uncrustify_musescore.cfg" . || true
echo
echo "== Print tidy_file.sh relevant lines (18-30) =="
sed -n '1,120p' tools/codestyle/tidy_file.sh | nl -ba | sed -n '1,120p'
echo
echo "== Search for UNCRUSTIFY_CONFIG usage =="
rg -n "UNCRUSTIFY_CONFIG|uncrustify_muse\.cfg|uncrustify_musescore\.cfg" tools/codestyle/tidy_file.sh tools/codestyle -S || trueRepository: musescore/MuseScore
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Tool availability =="
command -v fd || echo "fd: not found"
command -v rg || echo "rg: not found"
command -v cat || echo "cat: not found"
echo
echo "== Locate uncrustify_muse*.cfg files =="
find . -type f -name 'uncrustify_muse*.cfg' 2>/dev/null | sed 's|^\./||' || true
echo
echo "== Locate uncrustify_musescore.cfg =="
find . -type f -name 'uncrustify_musescore.cfg' 2>/dev/null | sed 's|^\./||' || true
echo
echo "== Print tools/codestyle/tidy_file.sh (lines 1-80) with line numbers =="
sed -n '1,120p' tools/codestyle/tidy_file.sh | cat -n | sed -n '1,120p'
echo
echo "== Search for UNCRUSTIFY_CONFIG/uncrustify cfg references in tidy_file.sh =="
if command -v rg >/dev/null 2>&1; then
rg -n "UNCRUSTIFY_CONFIG|uncrustify_muse\.cfg|uncrustify_musescore\.cfg" tools/codestyle/tidy_file.sh || true
else
grep -nE "UNCRUSTIFY_CONFIG|uncrustify_muse\.cfg|uncrustify_musescore\.cfg" tools/codestyle/tidy_file.sh || true
fiRepository: musescore/MuseScore
Length of output: 2767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check whether ./muse/ exists =="
ls -la muse 2>/dev/null || echo "muse/ not present in this checkout"
echo
echo "== Search for uncrustify_muse/uncrustify_musescore references across repo =="
rg -n "uncrustify_muse|uncrustify_musescore|uncrustify\.cfg" . || true
echo
echo "== Find download.cmake files =="
find . -type f -name 'download.cmake' 2>/dev/null | sed 's|^\./||' || true
echo
echo "== Search for uncrustify in *.cmake files =="
rg -n "uncrustify" --glob='*.cmake' . || true
echo
echo "== Show any uncrustify download/copy commands near matches =="
# Heuristic: print 60 lines around any line containing 'uncrustify' in CMake
rg -n "uncrustify" --glob='*.cmake' . -C 30 || trueRepository: musescore/MuseScore
Length of output: 6392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show tools/codestyle/tidy_file.sh (15-35) =="
sed -n '15,40p' tools/codestyle/tidy_file.sh | cat -n
echo
echo "== Show buildscripts/tools/codestyle/download.cmake =="
sed -n '1,120p' buildscripts/tools/codestyle/download.cmake | cat -n
echo
echo "== Show buildscripts/tools/codestyle/format_file.cmake (1-80) =="
sed -n '1,90p' buildscripts/tools/codestyle/format_file.cmake | cat -n
echo
echo "== Find pre-commit / hooks that call tidy_file.sh =="
rg -n "tidy_file\.sh|tools/codestyle/tidy_file\.sh|codestyle/tidy" . || true
echo
echo "== List .git hooks / pre-commit configs if present =="
find . -maxdepth 3 -type f \( -iname '*pre-commit*' -o -iname '*precommit*' -o -iname '*.pre-commit*' \) 2>/dev/null || trueRepository: musescore/MuseScore
Length of output: 3577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show hooks/pre-commit around where tidy_file.sh is called (60-120) =="
sed -n '60,140p' hooks/pre-commit | cat -n
echo
echo "== Search for uncrustify_muse.cfg anywhere in repo =="
find . -type f -name 'uncrustify_muse.cfg' -print | sed 's|^\./||' || true
echo
echo "== Search for uncrustify_muse*.cfg anywhere in repo =="
find . -type f -name 'uncrustify_muse*.cfg' -print | sed 's|^\./||' || trueRepository: musescore/MuseScore
Length of output: 2441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for uncrustify_musescore.cfg =="
find . -type f -name 'uncrustify_musescore.cfg' -print | sed 's|^\./||' || true
echo
echo "== Any references to uncrustify_musescore in repo =="
rg -n "uncrustify_musescore\.cfg" . || true
echo
echo "== Any references to uncrustify_muse.cfg besides tidy_file/download/format =="
rg -n "uncrustify_muse\.cfg" . || trueRepository: musescore/MuseScore
Length of output: 720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .gitmodules =="
if [ -f .gitmodules ]; then
cat .gitmodules
else
echo "No .gitmodules file found."
fi
echo
echo "== Check if 'muse' is tracked as a submodule (git config) =="
if command -v git >/dev/null 2>&1; then
git config --file .gitmodules --get-regexp '^submodule\..*\.path$' || true
git submodule status || true
else
echo "git not available."
fi
echo
echo "== List contents of muse/ (max depth 3) =="
find muse -maxdepth 3 -print 2>/dev/null || trueRepository: musescore/MuseScore
Length of output: 371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search docs for muse_framework/submodule initialization =="
rg -n "muse_framework|muse/.*submodule|git submodule|submodule.*muse" . || true
echo
echo "== Search for 'pre-commit' setup instructions =="
rg -n "pre-commit|hooks/pre-commit" . || trueRepository: musescore/MuseScore
Length of output: 2955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== README.md (70-120) =="
sed -n '70,120p' README.md | cat -n
echo
echo "== hooks/install.sh (1-120) =="
sed -n '1,160p' hooks/install.sh | cat -n
echo
echo "== hooks/uninstall.sh (1-120) =="
sed -n '1,120p' hooks/uninstall.sh | cat -nRepository: musescore/MuseScore
Length of output: 3529
Fix pre-commit uncrustify config path (may break formatting for developers)
tools/codestyle/tidy_file.shhardcodesUNCRUSTIFY_CONFIG="${REPO_ROOT}/muse/tools/codestyle/uncrustify_muse.cfg"and uses it inuncrustify -c ....hooks/pre-commitrunstidy_file.shdirectly and does not download/copyuncrustify_muse.cfg, so the hook relies entirely on the submodule file being present at that exact path.- In this repo checkout,
muse/tools/codestyle/uncrustify_muse.cfg(and anyuncrustify_muse*.cfg) is missing; add a clear existence check with actionable guidance (e.g., init/update themuse_frameworksubmodule) or maketidy_file.shdownload the cfg (similar tobuildscripts/tools/codestyle/download.cmake). - This looks like an infra/code-style change rather than the mixer-button feature—if it wasn’t intended, split or justify it in the PR.
🤖 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 `@tools/codestyle/tidy_file.sh` around lines 18 - 19, The tidy_file.sh script
currently sets
UNCRUSTIFY_CONFIG="${REPO_ROOT}/muse/tools/codestyle/uncrustify_muse.cfg" and
assumes the submodule file exists; update tidy_file.sh (and mention
hooks/pre-commit which calls it) to check for the existence of the
UNCRUSTIFY_CONFIG path and exit with a clear actionable message if missing
(e.g., instructing to init/update the muse_framework submodule or run a download
step), or implement an automatic fallback that downloads or copies the config
(reuse the logic/pattern from buildscripts/tools/codestyle/download.cmake)
before invoking uncrustify; reference the REPO_ROOT and UNCRUSTIFY_CONFIG
variables so the check/fallback is implemented using those symbols.
Resolves: #22767
This commit adds a button to unmute all muted channels in the mixer and another to unsolo all soloed channels in the mixer. Removing the need to do these actions to each channel one by one.
It also adds a shortcut to unmute all (ctrl+shift+m) and a shortcut to unsolo all (ctrl+shift+l) following the same logic as the buttons.