Add manual reassignment for MuseSounds Matches#33645
Conversation
📝 WalkthroughWalkthroughThis PR implements a Muse Sounds instrument reassignment feature that lets users match their notation instruments to available Muse Sounds candidates. The 🚥 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/playback/qml/MuseScore/Playback/MixerPanel.qml (1)
154-160: 💤 Low valueConsider documenting the tolerance threshold.
The 1-pixel tolerance check is a good optimization to prevent unnecessary position updates. However, a brief inline comment would help future maintainers understand why this threshold was chosen.
📝 Suggested documentation improvement
let endY = Math.max(0, flickable.contentHeight - flickable.height) + // Skip positioning if already at end (within 1px tolerance) if (Math.abs(flickable.contentY - endY) < 1) { return }🤖 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 154 - 160, Add a short inline comment explaining the 1-pixel tolerance used in the check between flickable.contentY and endY (e.g., why Math.abs(... ) < 1 prevents tiny flicking/rounding updates), and consider replacing the magic number with a named constant (e.g., const TOLERANCE = 1) used in the comparison to make the intent clearer in the block that sets flickable.contentY = endY.src/playback/internal/playbackcontroller.cpp (1)
119-124: ⚖️ Poor tradeoffClarify why
activeProfileName == museSoundsProfileNameforces basic profile input.In
PlaybackController::doAddTrack,shouldAvoidMuseSamplerInput()is used only to chooseinParams(track input source): when the active profile equalsmuseSoundsProfileName,useBasicProfilebecomes true andinParamsis replaced with resources frombasicSoundProfileName(). Track output params are still derived from the existingoriginParams, so the active Muse Sounds profile still applies to playback/output, while MIDI/input uses the basic profile.The current predicate name doesn’t document this coupling; adding a short comment (or renaming) to explain why Muse Sounds active should imply “avoid MuseSampler input” would resolve the confusion.
src/playback/qml/MuseScore/Playback/musesoundsreassignmodel.cpp (1)
246-252: 💤 Low valueOK applies a candidate to every matched track by default.
selectedCandidateIndexdefaults to0, so any track that has at least one candidate but no current MuseSampler match will be silently reassigned to its top-ranked candidate when the user presses OK, with no per-row opt-out. If the intent is "only change what the user picked," consider adding a "keep current"/no-change option per row.🤖 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/musesoundsreassignmodel.cpp` around lines 246 - 252, The loop silently leaves choice.selectedCandidateIndex at its default 0 causing automatic reassignment; change the logic so selectedCandidateIndex is initialized to a sentinel (e.g., -1) and only set to a concrete index when a real match is detected (the code that compares currentParams.resourceMeta.type/id to choice.candidates[i].resource.id inside the for loop), and add a per-row "keep current"/no-change candidate in choice.candidates or the UI so that when selectedCandidateIndex == -1 the OK action skips that track instead of assigning the top-ranked candidate.src/playback/qml/MuseScore/Playback/SoundProfilesDialog.qml (1)
250-250: 💤 Low valueNon-reactive
enabledbinding.
canReassignInstrumentsToMuseSounds()is aQ_INVOKABLEwith no associated notify signal, so this binding evaluates once and won't refresh if availability changes while the dialog is open (e.g., after resources finish loading). If availability can change during the dialog's lifetime, expose it as a notifiableQ_PROPERTYinstead.🤖 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/SoundProfilesDialog.qml` at line 250, The binding uses the Q_INVOKABLE method canReassignInstrumentsToMuseSounds(), which is non-notifiable so enabled is evaluated only once; change the model to expose a notifiable Q_PROPERTY (e.g., bool canReassignInstrumentsToMuseSounds READ canReassignInstrumentsToMuseSounds NOTIFY canReassignInstrumentsToMuseSoundsChanged) with an accompanying signal canReassignInstrumentsToMuseSoundsChanged and emit that signal when the underlying availability changes, then update the QML usage to bind to profilesListModel.canReassignInstrumentsToMuseSounds (the property) instead of calling the method so the button enabled state reacts to runtime changes.src/playback/qml/MuseScore/Playback/MuseSoundsReassignDialog.qml (1)
120-125: 💤 Low valueTransient empty-state flash while resources load.
load()resolves asynchronously, so on openhasChoicesis initially false and this "No matching Muse Sounds were found…" message (and a disabled OK) is shown untilbuildChoicescompletes. Consider gating the empty-state on a loaded flag to avoid the brief misleading message.🤖 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/MuseSoundsReassignDialog.qml` around lines 120 - 125, Add a "loaded" flag and gate the empty-state and OK disabling on it: initialize reassignModel.loaded = false before calling reassignModel.load()/buildChoices, set reassignModel.loaded = true in the .then/after the async load/buildChoices completes (or in its finished callback), change the StyledTextLabel visible binding to visible: reassignModel.loaded && !reassignModel.hasChoices, and update any OK button disabled binding (where it checks !reassignModel.hasChoices) to also include !reassignModel.loaded so the empty message and disabled state only appear after loading completes.
🤖 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/macos_integration/CMakeLists.txt`:
- Around line 119-120: The CMake step ad-hoc-signs the nested library
(execute_process calling codesign on
"${the_appex_path}/Contents/MacOS/platforms/libqcocoa.dylib") but the packaging
script (buildscripts/packaging/macOS/package.sh) only re-signs the .appex
without --deep, so the ad-hoc signature can persist; fix by updating packaging
to either explicitly re-sign the nested libqcocoa.dylib with the real Developer
ID identity (the same style as the .appex sign, using the Developer ID
Application: MuseScore identity and entitlements.plist) or when signing the
.appex in package.sh add the --deep flag so the recursive signing overwrites the
ad-hoc signature; ensure the codesign invocation in package.sh mirrors the
runtime/options/entitlements usage used elsewhere so spctl/notarization sees the
proper Developer ID signature.
In `@src/playback/qml/MuseScore/Playback/soundprofilesmodel.cpp`:
- Around line 69-79: The method
SoundProfilesModel::reassignInstrumentsToMuseSounds appears unused; either wire
it into the dialog flow called by openMuseSoundsReassignDialog() (so the
dialog’s confirm/auto-apply action invokes
controller()->reassignInstrumentsToAvailableMuseSounds() via
SoundProfilesModel::reassignInstrumentsToMuseSounds() and then updates
m_activeProfile/m_currentlySelectedProfile and emits
activeProfileChanged()/currentlySelectedProfileChanged()), or remove the unused
method and any leftover references if the logic is intended to live elsewhere;
ensure canReassignInstrumentsToMuseSounds() remains the button enabled check
while the actual reassign work is triggered from the dialog confirm handler
(openMuseSoundsReassignDialog()) if you choose to keep the behavior.
In `@src/project/qml/MuseScore/Project/internal/ScoresPage/ScoresGridView.qml`:
- Line 123: Add an inline comment in ScoresGridView.qml next to the
ScrollBar.vertical: null line explaining why the vertical scrollbar was removed
(e.g., to avoid duplicate scroll chrome when the parent provides scrolling or to
enable touch/paging behavior) and what interaction replaces it; reference the
related StyledScrollBar usage in ScoresListView.qml so reviewers see the
intentional inconsistency; then verify and document (in the same comment) that
keyboard and focus-based scrolling still work (e.g., via arrow/page keys or
focus handling on the grid items) and note any required focus/keyboard handlers
that must remain or be added to allow discoverability and navigation of
off-viewport content.
---
Nitpick comments:
In `@src/playback/qml/MuseScore/Playback/MixerPanel.qml`:
- Around line 154-160: Add a short inline comment explaining the 1-pixel
tolerance used in the check between flickable.contentY and endY (e.g., why
Math.abs(... ) < 1 prevents tiny flicking/rounding updates), and consider
replacing the magic number with a named constant (e.g., const TOLERANCE = 1)
used in the comparison to make the intent clearer in the block that sets
flickable.contentY = endY.
In `@src/playback/qml/MuseScore/Playback/MuseSoundsReassignDialog.qml`:
- Around line 120-125: Add a "loaded" flag and gate the empty-state and OK
disabling on it: initialize reassignModel.loaded = false before calling
reassignModel.load()/buildChoices, set reassignModel.loaded = true in the
.then/after the async load/buildChoices completes (or in its finished callback),
change the StyledTextLabel visible binding to visible: reassignModel.loaded &&
!reassignModel.hasChoices, and update any OK button disabled binding (where it
checks !reassignModel.hasChoices) to also include !reassignModel.loaded so the
empty message and disabled state only appear after loading completes.
In `@src/playback/qml/MuseScore/Playback/musesoundsreassignmodel.cpp`:
- Around line 246-252: The loop silently leaves choice.selectedCandidateIndex at
its default 0 causing automatic reassignment; change the logic so
selectedCandidateIndex is initialized to a sentinel (e.g., -1) and only set to a
concrete index when a real match is detected (the code that compares
currentParams.resourceMeta.type/id to choice.candidates[i].resource.id inside
the for loop), and add a per-row "keep current"/no-change candidate in
choice.candidates or the UI so that when selectedCandidateIndex == -1 the OK
action skips that track instead of assigning the top-ranked candidate.
In `@src/playback/qml/MuseScore/Playback/SoundProfilesDialog.qml`:
- Line 250: The binding uses the Q_INVOKABLE method
canReassignInstrumentsToMuseSounds(), which is non-notifiable so enabled is
evaluated only once; change the model to expose a notifiable Q_PROPERTY (e.g.,
bool canReassignInstrumentsToMuseSounds READ canReassignInstrumentsToMuseSounds
NOTIFY canReassignInstrumentsToMuseSoundsChanged) with an accompanying signal
canReassignInstrumentsToMuseSoundsChanged and emit that signal when the
underlying availability changes, then update the QML usage to bind to
profilesListModel.canReassignInstrumentsToMuseSounds (the property) instead of
calling the method so the button enabled state reacts to runtime changes.
🪄 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: 004645f2-a979-4bed-83f7-6e942027a225
📒 Files selected for processing (18)
musesrc/macos_integration/CMakeLists.txtsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/playbackcontroller.hsrc/playback/iplaybackcontroller.hsrc/playback/playbackmodule.cppsrc/playback/qml/MuseScore/Playback/CMakeLists.txtsrc/playback/qml/MuseScore/Playback/MixerPanel.qmlsrc/playback/qml/MuseScore/Playback/MuseSoundsReassignDialog.qmlsrc/playback/qml/MuseScore/Playback/SoundProfilesDialog.qmlsrc/playback/qml/MuseScore/Playback/musesoundsreassignmodel.cppsrc/playback/qml/MuseScore/Playback/musesoundsreassignmodel.hsrc/playback/qml/MuseScore/Playback/soundprofilesmodel.cppsrc/playback/qml/MuseScore/Playback/soundprofilesmodel.hsrc/playback/tests/mocks/playbackcontrollermock.hsrc/project/qml/MuseScore/Project/internal/ScoresPage/ScoresGridView.qmlsrc/stubs/playback/playbackcontrollerstub.cppsrc/stubs/playback/playbackcontrollerstub.h
| execute_process(COMMAND codesign --force --sign - --timestamp=none \"${the_appex_path}/Contents/MacOS/platforms/libqcocoa.dylib\") | ||
| execute_process(COMMAND codesign --force --sign - --timestamp=none --options runtime --entitlements \"${CMAKE_CURRENT_SOURCE_DIR}/entitlements.plist\" \"${the_appex_path}\") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at how the .appex / nested plugins are signed during packaging.
fd -t f 'package.sh' buildscripts | xargs -r rg -nC3 'codesign|--deep|libqcocoa|appex|--entitlements'
# Any other place that signs platform plugins?
rg -nC3 'libqcocoa|platforms' --glob '!**/*.dylib'Repository: musescore/MuseScore
Length of output: 35656
Clarify packaging re-signing: ad-hoc libqcocoa.dylib may not be overwritten
CMake does ad-hoc signing of the bundled nested dylib before signing the .appex:
execute_process(COMMAND codesign --force --sign - --timestamp=none \"${the_appex_path}/Contents/MacOS/platforms/libqcocoa.dylib\")
execute_process(COMMAND codesign --force --sign - --timestamp=none --options runtime --entitlements \"${CMAKE_CURRENT_SOURCE_DIR}/entitlements.plist\" \"${the_appex_path}\")In buildscripts/packaging/macOS/package.sh, the packaging step re-signs only the .appex with codesign ... -s "Developer ID Application: MuseScore" without --deep (and there’s no explicit re-sign of libqcocoa.dylib there); --deep is only used when re-signing the main app, followed by codesign --verify --deep --strict. That means the ad-hoc signature on libqcocoa.dylib could persist into the shipped bundle.
Ensure packaging either (a) re-signs the nested libqcocoa.dylib explicitly with the real Developer ID identity, or (b) re-signs the .appex recursively (via --deep) so the ad-hoc signature is overwritten; otherwise spctl/notarization may fail.
🤖 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/macos_integration/CMakeLists.txt` around lines 119 - 120, The CMake step
ad-hoc-signs the nested library (execute_process calling codesign on
"${the_appex_path}/Contents/MacOS/platforms/libqcocoa.dylib") but the packaging
script (buildscripts/packaging/macOS/package.sh) only re-signs the .appex
without --deep, so the ad-hoc signature can persist; fix by updating packaging
to either explicitly re-sign the nested libqcocoa.dylib with the real Developer
ID identity (the same style as the .appex sign, using the Developer ID
Application: MuseScore identity and entitlements.plist) or when signing the
.appex in package.sh add the --deep flag so the recursive signing overwrites the
ad-hoc signature; ensure the codesign invocation in package.sh mirrors the
runtime/options/entitlements usage used elsewhere so spctl/notarization sees the
proper Developer ID signature.
| void SoundProfilesModel::reassignInstrumentsToMuseSounds() | ||
| { | ||
| controller()->reassignInstrumentsToAvailableMuseSounds(); | ||
|
|
||
| if (INotationProjectPtr project = context()->currentProject()) { | ||
| m_activeProfile = project->audioSettings()->activeSoundProfile().toQString(); | ||
| m_currentlySelectedProfile = m_activeProfile; | ||
| emit activeProfileChanged(); | ||
| emit currentlySelectedProfileChanged(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any caller of the auto-apply method (QML or C++)
rg -nP -C3 'reassignInstrumentsToMuseSounds\s*\(' -g '!**/soundprofilesmodel.*'Repository: musescore/MuseScore
Length of output: 45
Potential dead code: SoundProfilesModel::reassignInstrumentsToMuseSounds()
SoundProfilesDialog.qml only calls openMuseSoundsReassignDialog() (the button action) and uses canReassignInstrumentsToMuseSounds() only for the button enabled state; the repo contains no other call sites for reassignInstrumentsToMuseSounds() beyond its definition (src/playback/qml/MuseScore/Playback/soundprofilesmodel.cpp:69-79). Wire it into the dialog/auto-apply flow or remove it.
🤖 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/soundprofilesmodel.cpp` around lines 69 -
79, The method SoundProfilesModel::reassignInstrumentsToMuseSounds appears
unused; either wire it into the dialog flow called by
openMuseSoundsReassignDialog() (so the dialog’s confirm/auto-apply action
invokes controller()->reassignInstrumentsToAvailableMuseSounds() via
SoundProfilesModel::reassignInstrumentsToMuseSounds() and then updates
m_activeProfile/m_currentlySelectedProfile and emits
activeProfileChanged()/currentlySelectedProfileChanged()), or remove the unused
method and any leftover references if the logic is intended to live elsewhere;
ensure canReassignInstrumentsToMuseSounds() remains the button enabled check
while the actual reassign work is triggered from the dialog confirm handler
(openMuseSoundsReassignDialog()) if you choose to keep the behavior.
| visible: view.contentHeight > view.height | ||
| z: 2 | ||
| } | ||
| ScrollBar.vertical: null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare scrollbar patterns between list and grid views
echo "=== ScoresListView scrollbar ==="
rg -n 'ScrollBar\.vertical' src/project/qml/MuseScore/Project/internal/ScoresPage/ScoresListView.qml -A 5
echo -e "\n=== ScoresGridView scrollbar ==="
rg -n 'ScrollBar\.vertical' src/project/qml/MuseScore/Project/internal/ScoresPage/ScoresGridView.qml -A 5Repository: musescore/MuseScore
Length of output: 557
Document rationale + confirm UX/accessibility after removing grid vertical scrollbar
ScoresGridView.qmlsetsScrollBar.vertical: null(line 123), whileScoresListView.qmlretains aStyledScrollBar(around lines 192-197), creating UI inconsistency without any in-code explanation.- Add an inline comment explaining why the grid should remove the scrollbar (and what problem/behavior it addresses or replaces).
- Verify/discuss that users can still discover and navigate additional content beyond the viewport (including keyboard/focus scrolling) despite the missing scrollbar.
🤖 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/project/qml/MuseScore/Project/internal/ScoresPage/ScoresGridView.qml` at
line 123, Add an inline comment in ScoresGridView.qml next to the
ScrollBar.vertical: null line explaining why the vertical scrollbar was removed
(e.g., to avoid duplicate scroll chrome when the parent provides scrolling or to
enable touch/paging behavior) and what interaction replaces it; reference the
related StyledScrollBar usage in ScoresListView.qml so reviewers see the
intentional inconsistency; then verify and document (in the same comment) that
keyboard and focus-based scrolling still work (e.g., via arrow/page keys or
focus handling on the grid items) and note any required focus/keyboard handlers
that must remain or be added to allow discoverability and navigation of
off-viewport content.
|
Resolves: N/A Summary: Changes:
Manual verification:
Automated verification:
Companion framework PR: |
|
Companion framework PR: https://github.com/musescore/muse_framework/pull/NUMBER |
|
Please read, understand, and complete the contributor checklist before opening a PR. |
3 participants
Adds a manual Muse Sounds reassignment workflow for matching score instruments to available Muse Sounds resources.
Manual verification:
Automated verification:
Depends on companion muse_framework PR:
https://github.com/musescore/muse_framework/pulls?q=is%3Apr+author%3ATrevorDogVHPASTE_LINK_TO_THE_FIRST_PR_HEREResolves: #NNNNN