MNX import/export changes for schema version 19#33958
Open
rpatters1 wants to merge 28 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR updates MNX import and export support for fermatas, bow directions, breath marks, dynamics, arpeggios, slurs, hairpins, ottavas, and beam direction. It adds shared conversion helpers for orientation, placement, fermata symbols, and dynamic types, adjusts MNX exporter/importer sequence handling and metadata population, and updates build configuration. New and revised MNX/MuseScore fixtures were added for the covered musical cases. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were 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: 7
🤖 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/importexport/mnx/CMakeLists.txt`:
- Line 22: The system mnxdom fallback is not gated to the same API/schema level
as the pinned Git commit, so older installed versions can be selected and then
fail on missing schema-19 symbols. Update the `MSS_USE_SYSTEM_MNXDOM` path in
`src/importexport/mnx/CMakeLists.txt` around `pkg_check_modules` to require an
explicit minimum version or add a configure-time feature check for the needed
mnx types such as `mnx::DynamicValue` and `mnx::FermataSymbol`, so only
compatible system packages are accepted.
In `@src/importexport/mnx/internal/export/mnxexportparts.cpp`:
- Around line 496-499: The ottava-end handling in `MnxExportParts::...` is still
calling `toChord(ottava->endElement())` unconditionally, which can trip the
cast/assert path for rest or other non-chord endpoints. Update this branch to
first check `ottava->endElement()->isChord()` and only then cast to `Chord` and
inspect `graceNotesBefore()`, keeping the existing grace-index adjustment logic
for valid chord endpoints.
- Around line 344-350: The slur export path in mnxexportparts.cpp only verifies
endCR is non-null, but it can still point to a chord/rest that was skipped
during MNX export. In the logic around findFirstChordRest(), mnxEventFromCR(),
and ensure_slurs(), add a validation step that confirms the end chord/rest has a
corresponding MNX event before appending endCR->eid() as the slur target, and
skip emitting the reference if no exported event exists.
In `@src/importexport/mnx/internal/import/mnximporter.cpp`:
- Around line 846-848: The fermata import path in mnximporter.cpp uses
Measure::getSegment for the EndBarLine case, which only returns an existing
segment and can leave addFermata with null when no explicit barline was created.
Update the fermata handling in the MNX measure import flow to use the same
creating relative lookup as the barline path, ensuring the EndBarLine segment is
created before calling addFermata. Keep the fix localized around
mnxMeasure.fermata(), measure, and addFermata so the global fermata is always
attached correctly.
In `@src/importexport/mnx/internal/import/mnximportpartmeasures.cpp`:
- Around line 1229-1236: Filter sequences by the resolved staff before applying
the voice offset in mnximportpartmeasures::importPartMeasures. The current loop
over mnxMeasure.sequences() uses x as a global sequence index, but curTrackIdx
+= x should only use the offset among sequences belonging to the same staff as
the dynamic. Update the mnxDynamic.voice() handling to first restrict to
sequences for the target staff, then apply the voice match and offset so
multi-staff parts map to the correct MuseScore voice.
In `@src/importexport/mnx/internal/shared/mnxtypesconv.cpp`:
- Around line 761-769: The mnx::Orientation::Auto branch in
toMuseScorePlacementV assumes item is always non-null and dereferences it
directly, so make that contract explicit by either changing the parameter to a
reference if every caller must provide an EngravingItem, or adding a null
check/assert with a safe fallback before accessing
propertyDefault(Pid::PLACEMENT). Keep the fix localized in toMuseScorePlacementV
and update any callers if the signature changes.
In `@src/importexport/mnx/internal/shared/mnxtypesconv.h`:
- Line 25: The mnxtypesconv.h header uses std::pair in its public API, but it
does not include the defining header directly. Add the appropriate include for
std::pair in this header so it is self-contained, and keep the existing
declarations that expose std::pair unchanged.
🪄 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: 6bb7451d-ffe9-4e06-a63f-0445ab40c0e7
📒 Files selected for processing (46)
src/importexport/mnx/CMakeLists.txtsrc/importexport/mnx/internal/export/mnxexporter.cppsrc/importexport/mnx/internal/export/mnxexporter.hsrc/importexport/mnx/internal/export/mnxexportglobal.cppsrc/importexport/mnx/internal/export/mnxexportlayouts.cppsrc/importexport/mnx/internal/export/mnxexportparts.cppsrc/importexport/mnx/internal/export/mnxexportsequences.cppsrc/importexport/mnx/internal/import/mnximporter.cppsrc/importexport/mnx/internal/import/mnximporter.hsrc/importexport/mnx/internal/import/mnximportmarkings.cppsrc/importexport/mnx/internal/import/mnximportpartmeasures.cppsrc/importexport/mnx/internal/shared/mnxtypesconv.cppsrc/importexport/mnx/internal/shared/mnxtypesconv.hsrc/importexport/mnx/tests/data/README.mdsrc/importexport/mnx/tests/data/mscx_reference_examples/dynamics_ref.mscxsrc/importexport/mnx/tests/data/project_examples/arpeggios.mnxsrc/importexport/mnx/tests/data/project_examples/arpeggios_ref.mscxsrc/importexport/mnx/tests/data/project_examples/beamsOverBarlines_ref.mscxsrc/importexport/mnx/tests/data/project_examples/bowDirection.mnxsrc/importexport/mnx/tests/data/project_examples/bowDirection_ref.mscxsrc/importexport/mnx/tests/data/project_examples/breathMark.mnxsrc/importexport/mnx/tests/data/project_examples/breathMark_ref.mscxsrc/importexport/mnx/tests/data/project_examples/dynamicV1V2.mnxsrc/importexport/mnx/tests/data/project_examples/dynamicV1V2_ref.mscxsrc/importexport/mnx/tests/data/project_examples/dynamicVoice.mnxsrc/importexport/mnx/tests/data/project_examples/dynamicVoice_ref.mscxsrc/importexport/mnx/tests/data/project_examples/dynamicsHairpins.mnxsrc/importexport/mnx/tests/data/project_examples/dynamicsHairpins_ref.mscxsrc/importexport/mnx/tests/data/project_examples/enharmonics.mnxsrc/importexport/mnx/tests/data/project_examples/enharmonics_ref.mscxsrc/importexport/mnx/tests/data/project_examples/fermata.mnxsrc/importexport/mnx/tests/data/project_examples/fermata_ref.mscxsrc/importexport/mnx/tests/data/project_examples/graceArps.mnxsrc/importexport/mnx/tests/data/project_examples/graceArps_ref.mscxsrc/importexport/mnx/tests/data/project_examples/nonArpeggios.mnxsrc/importexport/mnx/tests/data/project_examples/nonArpeggios_ref.mscxsrc/importexport/mnx/tests/data/project_sources/arpeggios.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/bowDirection.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/breathMark.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/dynamicV1V2.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/dynamicVoice.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/dynamicsHairpins.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/fermata.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/graceArps.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/nonArpeggios.enigmaxmlsrc/importexport/mnx/tests/mnx_tests.cpp
| mnxdom | ||
| GIT_REPOSITORY https://github.com/rpatters1/mnxdom.git | ||
| GIT_TAG e7c947bf768caccf315426dcae0dfac02caf738b | ||
| GIT_TAG 11a12e6a2492ca54183acf5b22a288dae032c625 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the system mnxdom path enforces a version/API gate.
rg -n 'pkg_check_modules\(mnxdom|MSS_USE_SYSTEM_MNXDOM|DynamicValue|FermataSymbol' src/importexport/mnxRepository: musescore/MuseScore
Length of output: 10190
Gate system mnxdom to the same schema/API level as the pinned commit.
The Git-fetched mnxdom is pinned to a commit supporting schema-19 types (e.g., mnx::DynamicValue, mnx::FermataSymbol), but enabling MSS_USE_SYSTEM_MNXDOM relies on an unconstrained pkg_check_modules call. Older system versions lack these types, causing compilation failures.
Enforce a minimum version in pkg_check_modules or add a configure-time check for the required types.
🤖 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/importexport/mnx/CMakeLists.txt` at line 22, The system mnxdom fallback
is not gated to the same API/schema level as the pinned Git commit, so older
installed versions can be selected and then fail on missing schema-19 symbols.
Update the `MSS_USE_SYSTEM_MNXDOM` path in `src/importexport/mnx/CMakeLists.txt`
around `pkg_check_modules` to require an explicit minimum version or add a
configure-time feature check for the needed mnx types such as
`mnx::DynamicValue` and `mnx::FermataSymbol`, so only compatible system packages
are accepted.
Contributor
Author
There was a problem hiding this comment.
This can't be resolved with the current build approach. We need to get this merged, and then I will refactor the build using a submodule, so that there will no longer be a need to use a system version of the mnxdom.
Contributor
There was a problem hiding this comment.
@rpatters1 Understood — thanks for the clarification. Given the planned follow-up to switch mnxdom handling to a submodule and remove the system-installed path, I won’t push further on this in the current PR.
If you want, I can open a follow-up GitHub issue to track that build refactor separately. I’ve also noted this context for future reviews.
(•ᴗ•)🐇
🧠 Learnings used
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33517
File: CMakeLists.txt:52-56
Timestamp: 2026-05-21T09:19:10.076Z
Learning: In the MuseScore (musescore/MuseScore) repo, the CMake modules like `CheckCompilerVersion`, `GetCompilerInfo`, `SetupBuildEnvironment`, `GetPlatformInfo`, `SetupCompilerCache`, `SetupCompileWarnings`, `SetupQt6`, `GetUtilsFunctions`, etc. come from the `muse` git submodule under `muse/` (framework sources in `muse/framework/`). In environments that don’t clone submodules (e.g., the sandbox), searches for these module files may return empty results—this is expected and should not be flagged as a bug. When reviewing CMake, expect `${MUSE_FRAMEWORK_PATH}/buildscripts/cmake` to be added to `CMAKE_MODULE_PATH` so these modules are discoverable during normal builds.
Contributor
Author
There was a problem hiding this comment.
Sure, open an issue and assign me it.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="musescore/MuseScore"
assignee="rpatters1"
body_file="$(mktemp)"
cat > "$body_file" <<'EOF'
## Summary
Refactor the MNX build integration to stop relying on the optional system-installed `mnxdom` path and instead use a repository-managed dependency flow (planned submodule-based approach).
## Background
In `#33958`, the fetched `mnxdom` revision was updated to a newer upstream commit to support the schema/API surface now required by the MNX import/export code. The current `MSS_USE_SYSTEM_MNXDOM` path in `src/importexport/mnx/CMakeLists.txt` does not enforce compatibility with that same API level, but this cannot be cleanly resolved within the current build approach.
Per the PR discussion, the intended follow-up is to refactor the build so that `mnxdom` is handled via a submodule and the need for a system-installed fallback is removed.
## Affected area
- `src/importexport/mnx/CMakeLists.txt`
- MNX build/dependency wiring for `iex_mnx`
## Requested changes
- Refactor `mnxdom` integration to use the planned submodule-based build approach.
- Remove or otherwise retire the `MSS_USE_SYSTEM_MNXDOM` path if it is no longer needed.
- Ensure the build uses a single controlled `mnxdom` source that matches the API/schema requirements of the MNX import/export code.
## Rationale
This avoids configuration-time ambiguity where a system package may not match the API level expected by the source tree, and it aligns dependency resolution with the intended long-term build structure.
## Acceptance criteria
- `iex_mnx` no longer depends on an unconstrained system `mnxdom` package path.
- The build configuration documents and uses the repository-managed `mnxdom` dependency flow.
- MNX import/export code builds against the intended `mnxdom` revision/API surface without needing separate system-package compatibility handling.
## Backlinks
- PR: https://github.com/musescore/MuseScore/pull/33958
- Review comment: https://github.com/musescore/MuseScore/pull/33958#discussion_r3475542228
Requested by: `@rpatters1`
EOF
issue_url="$(gh issue create \
--repo "$repo" \
--title "Refactor MNX build to use submodule-managed mnxdom and remove system fallback" \
--body-file "$body_file" \
--assignee "$assignee")"
echo "CREATED_ISSUE_URL=$issue_url"Length of output: 2712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="musescore/MuseScore"
body_file="$(mktemp)"
cat > "$body_file" <<'EOF'
## Summary
Refactor the MNX build integration to stop relying on the optional system-installed `mnxdom` path and instead use a repository-managed dependency flow (planned submodule-based approach).
## Background
In `#33958`, the fetched `mnxdom` revision was updated to a newer upstream commit to support the schema/API surface now required by the MNX import/export code. The current `MSS_USE_SYSTEM_MNXDOM` path in `src/importexport/mnx/CMakeLists.txt` does not enforce compatibility with that same API level, but this cannot be cleanly resolved within the current build approach.
Per the PR discussion, the intended follow-up is to refactor the build so that `mnxdom` is handled via a submodule and the need for a system-installed fallback is removed.
## Affected area
- `src/importexport/mnx/CMakeLists.txt`
- MNX build/dependency wiring for `iex_mnx`
## Requested changes
- Refactor `mnxdom` integration to use the planned submodule-based build approach.
- Remove or otherwise retire the `MSS_USE_SYSTEM_MNXDOM` path if it is no longer needed.
- Ensure the build uses a single controlled `mnxdom` source that matches the API/schema requirements of the MNX import/export code.
## Rationale
This avoids configuration-time ambiguity where a system package may not match the API level expected by the source tree, and it aligns dependency resolution with the intended long-term build structure.
## Acceptance criteria
- `iex_mnx` no longer depends on an unconstrained system `mnxdom` package path.
- The build configuration documents and uses the repository-managed `mnxdom` dependency flow.
- MNX import/export code builds against the intended `mnxdom` revision/API surface without needing separate system-package compatibility handling.
## Backlinks
- PR: https://github.com/musescore/MuseScore/pull/33958
- Review comment: https://github.com/musescore/MuseScore/pull/33958#discussion_r3475542228
Requested by: `@rpatters1`
Note: assignment to `@rpatters1` was requested in the PR discussion, but may need to be applied manually if the integration lacks assignee permissions.
EOF
issue_url="$(gh issue create \
--repo "$repo" \
--title "Refactor MNX build to use submodule-managed mnxdom and remove system fallback" \
--body-file "$body_file")"
echo "CREATED_ISSUE_URL=$issue_url"Length of output: 2609
@rpatters1 Opened the follow-up issue: #33966
I also tried to assign it to you, but the current GitHub integration does not have assignee permissions (Resource not accessible by integration), so that part will need to be done manually.
🧠 Learnings used
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33517
File: CMakeLists.txt:52-56
Timestamp: 2026-05-21T09:19:10.076Z
Learning: In the MuseScore (musescore/MuseScore) repo, the CMake modules like `CheckCompilerVersion`, `GetCompilerInfo`, `SetupBuildEnvironment`, `GetPlatformInfo`, `SetupCompilerCache`, `SetupCompileWarnings`, `SetupQt6`, `GetUtilsFunctions`, etc. come from the `muse` git submodule under `muse/` (framework sources in `muse/framework/`). In environments that don’t clone submodules (e.g., the sandbox), searches for these module files may return empty results—this is expected and should not be flagged as a bug. When reviewing CMake, expect `${MUSE_FRAMEWORK_PATH}/buildscripts/cmake` to be added to `CMAKE_MODULE_PATH` so these modules are discoverable during normal builds.
This was referenced Jun 25, 2026
Contributor
|
I wonder whether a rebase fixes the (vtest) build failure? |
Contributor
Author
Considering the failure is nowhere near the mnx code, Imma go with yes. |
- fix typo in log message - clean up breathMark test case for more complete coverage
… beam direction) Add more tests for voiced dynamics
… producing the wrong value.
…th voice assignments on imports. This commit fixes that.
…of downloading w3c/mnx repo.
- wrap import in transaction to suppress undo warning - suppress undo warnings in text fixture
9 tasks
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.
This PR depends on #33236. If this PR is merged first, then that one (MNX schema 15) should be closed without merging.
In addition to all the changes for schema 15, this PR contains the following.
MNX dynamics support. This is what schema 19 introduced.
Bug fixes.
diatonicDeltain export.Add provenance information, which was added to the current
mnxdom.Add comprehensive testing of dynamics and other bug fixes.
I signed the CLA as username:
The title of the PR describes the problem it addresses.
Each commit's message describes its purpose and effects, and references the issue it resolves. If changes are extensive, there is a sequence of easily reviewable commits.
The code in the PR follows the coding rules.
I understand all aspects of the code I'm contributing and I'm able to explain it if requested.
The code compiles and runs on my machine, preferably after each commit individually. I have manually tested and verified that my changes fulfil their intended purpose.
No prior attempts to resolve this problem exist, or if they do, I listed them in my PR description and described how I avoided repeating past mistakes.
There are no unnecessary changes.
I created a unit test or vtest to verify the changes I made (if applicable).