Mnx import/export changes for schema version 15#33236
Open
rpatters1 wants to merge 13 commits intomusescore:masterfrom
Open
Mnx import/export changes for schema version 15#33236rpatters1 wants to merge 13 commits intomusescore:masterfrom
rpatters1 wants to merge 13 commits intomusescore:masterfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
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:
📝 WalkthroughWalkthroughAdds fermata support across MNX import/export: new MNX↔MuseScore type conversions, an exporter helper to map engraving::Fermata → mnx::Fermata, and importer logic to create/attach Fermata elements (including measure-level/global fermatas). Refactors marking/articulation handling and breath export, adds bow-direction import/export, and threads Measure/event-end context into marking imports. Introduces MNX project fixtures and MSCX reference fixtures for breathMark, bowDirection, and fermata. Updates many MSCX reference files to remove or clear part-level 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/importexport/mnx/internal/export/mnxexportglobal.cpp`:
- Around line 306-319: The code dereferences barLine->segment() without null
checking before calling annotations(), which can crash if segment() returns
nullptr; update the loop to first get the Segment* (e.g., Segment* seg =
barLine->segment()) and skip processing if seg is null, then iterate over
seg->annotations(), preserving the existing checks for each EngravingItem and
the Fermata handling (toFermata, DO_ASSERT, and mnxMeasure.set_fermata with
MnxExporter::mnxFermataFromFermata) so behavior is unchanged when a segment
exists.
In `@src/importexport/mnx/internal/import/mnximportmarkings.cpp`:
- Around line 325-327: The code calls fermata->setTrack(...) before checking
whether addFermata succeeded; move the null/assert guard so you verify the
returned Fermata* (from addFermata(seg, mnxFermata)) with IF_ASSERT_FAILED (or
an explicit null check) immediately after assignment and before any use, and
only call fermata->setTrack(cr->track()) when fermata is non-null (or handle the
error/return in the failure branch).
- Around line 74-89: addFermata currently only uses mnxFermata.symbol() and
drops mnxFermata.duration(), causing lossy round-trips; update addFermata to
convert both symbol and duration into the MuseScore fermata variant/time-stretch
instead of only calling toMuseScoreFermataSymId. Use or add a combined converter
in mnxtypesconv (e.g. a function that accepts mnx::Fermata or both
symbol+duration and returns the MuseScore symId and time-stretch/variant) and
call it here to pass the resulting values into fermata->setSymIdAndTimeStretch
(and any styling like Pid::PLACEMENT) so duration is preserved on import.
🪄 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: 7a439190-9c17-4338-9111-1a623934662f
📒 Files selected for processing (86)
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/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/mscx_reference_examples/accidentals_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/articulations_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/beam-hooks_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/beams-across-barlines_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/beams-inner-grace-notes_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/beams-secondary-beam-breaks-implied_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/beams-secondary-beam-breaks_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/beams_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/clef-changes_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/dotted-notes_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/dynamics_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/full-measure-rests_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/grace-note_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/grace-notes-beamed_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/hello-world_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/jumps-dal-segno_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/jumps-ds-al-fine_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/key-signatures_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/lyric-line-metadata_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/lyrics-basic_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/lyrics-multi-line_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/multi-note-tremolos_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/multimeasure-rests_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/multiple-voices_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/ottavas-8va_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/parts_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/repeats-alternate-endings-advanced_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/repeats-alternate-endings-simple_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/repeats-implied-start-repeat_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/repeats-more-once-repeated_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/repeats_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/rest-positions_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/single-note-tremolos_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/slurs-chords_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/slurs-targeting-specific-notes_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/slurs_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/tempo-markings_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/three-note-chord-and-half-rest_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/tie-target-type_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/ties_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/time-signature-glyphs_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/time-signatures_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/tuplets_ref.mscxsrc/importexport/mnx/tests/data/mscx_reference_examples/two-bar-c-major-scale_ref.mscxsrc/importexport/mnx/tests/data/project_examples/altoFluteTremMissingKey_ref.mscxsrc/importexport/mnx/tests/data/project_examples/altoFluteTrem_ref.mscxsrc/importexport/mnx/tests/data/project_examples/barlineTypesWithShort_ref.mscxsrc/importexport/mnx/tests/data/project_examples/bcl_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/clarinet38MissingTime_ref.mscxsrc/importexport/mnx/tests/data/project_examples/clarinet38_ref.mscxsrc/importexport/mnx/tests/data/project_examples/enharmonicPart_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/graceBeamed_ref.mscxsrc/importexport/mnx/tests/data/project_examples/key56Wrapped56Edited_ref.mscxsrc/importexport/mnx/tests/data/project_examples/key77Wrapped_ref.mscxsrc/importexport/mnx/tests/data/project_examples/key77_ref.mscxsrc/importexport/mnx/tests/data/project_examples/measnumSequences_ref.mscxsrc/importexport/mnx/tests/data/project_examples/multinoteTremolosAdv_ref.mscxsrc/importexport/mnx/tests/data/project_examples/multinoteTremolos_ref.mscxsrc/importexport/mnx/tests/data/project_examples/ottavas_ref.mscxsrc/importexport/mnx/tests/data/project_examples/percussionKit_ref.mscxsrc/importexport/mnx/tests/data/project_examples/restPosition_ref.mscxsrc/importexport/mnx/tests/data/project_examples/tupletHiddenRest_ref.mscxsrc/importexport/mnx/tests/data/project_examples/tupletNested_ref.mscxsrc/importexport/mnx/tests/data/project_examples/tupletSimple_ref.mscxsrc/importexport/mnx/tests/data/project_sources/bowDirection.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/breathMark.enigmaxmlsrc/importexport/mnx/tests/data/project_sources/fermata.enigmaxmlsrc/importexport/mnx/tests/mnx_tests.cpp
💤 Files with no reviewable changes (13)
- src/importexport/mnx/tests/data/project_examples/bcl_ref.mscx
- src/importexport/mnx/tests/data/project_examples/percussionKit_ref.mscx
- src/importexport/mnx/tests/data/mscx_reference_examples/time-signature-glyphs_ref.mscx
- src/importexport/mnx/tests/data/project_examples/clarinet38MissingTime_ref.mscx
- src/importexport/mnx/tests/data/mscx_reference_examples/parts_ref.mscx
- src/importexport/mnx/tests/data/project_examples/clarinet38_ref.mscx
- src/importexport/mnx/tests/data/project_examples/restPosition_ref.mscx
- src/importexport/mnx/tests/data/project_examples/enharmonicPart_ref.mscx
- src/importexport/mnx/tests/data/project_examples/altoFluteTremMissingKey_ref.mscx
- src/importexport/mnx/tests/data/project_examples/altoFluteTrem_ref.mscx
- src/importexport/mnx/tests/data/project_examples/key77Wrapped_ref.mscx
- src/importexport/mnx/tests/data/project_examples/key77_ref.mscx
- src/importexport/mnx/tests/data/project_examples/key56Wrapped56Edited_ref.mscx
Comment on lines
+74
to
+89
| Fermata* MnxImporter::addFermata(Segment* seg, const mnx::Fermata& mnxFermata) | ||
| { | ||
| Fermata* fermata = Factory::createFermata(seg ? seg : m_score->dummy()->segment()); | ||
| fermata->setSymIdAndTimeStretch(toMuseScoreFermataSymId(mnxFermata.symbol())); | ||
| switch (mnxFermata.orient()) { | ||
| case mnx::Orientation::Above: | ||
| setAndStyleProperty(fermata, Pid::PLACEMENT, PlacementV::ABOVE); | ||
| break; | ||
| case mnx::Orientation::Below: | ||
| setAndStyleProperty(fermata, Pid::PLACEMENT, PlacementV::BELOW); | ||
| break; | ||
| case mnx::Orientation::Auto: | ||
| // do nothing here on purpose | ||
| break; | ||
| } | ||
| return fermata; |
There was a problem hiding this comment.
Import the MNX fermata duration, not just symbol.
addFermata() rebuilds the MuseScore fermata from mnxFermata.symbol() alone, so the schema-v15 duration field is dropped on import. That makes valid MNX fermatas round-trip lossy whenever duration and symbol are not the exact default pairing implied by toMuseScoreFermataSymId(). This should map both fields back into the MuseScore fermata variant/time-stretch, likely via a combined converter in mnxtypesconv.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/mnx/internal/import/mnximportmarkings.cpp` around lines 74 -
89, addFermata currently only uses mnxFermata.symbol() and drops
mnxFermata.duration(), causing lossy round-trips; update addFermata to convert
both symbol and duration into the MuseScore fermata variant/time-stretch instead
of only calling toMuseScoreFermataSymId. Use or add a combined converter in
mnxtypesconv (e.g. a function that accepts mnx::Fermata or both symbol+duration
and returns the MuseScore symId and time-stretch/variant) and call it here to
pass the resulting values into fermata->setSymIdAndTimeStretch (and any styling
like Pid::PLACEMENT) so duration is preserved on import.
Contributor
Author
There was a problem hiding this comment.
The mnx duration field corresponds most closely to MuseScore's FermataType, which is not a writable property of Fermata (so far as I can tell). Therefore, the mnx duration field is not importable but rather implicit in the symbol conversion. MNX allows, e.g., a fermata to be a double-dotted fermata with short length and normal symbol. MuseScore does not appear to allow this. If there is something I've missed, let me know.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if FermataType is a writable property of Fermata
rg -n "FermataType" --type cpp -C3
# Check Fermata's properties
rg -n "Pid::FERMATA" --type cpp -C3
# Check setSymIdAndTimeStretch
rg -n "setSymIdAndTimeStretch" --type cpp -C5
# Check fermataType as a property
ast-grep --pattern 'getProperty(Pid::$_) {
$$$
fermataType
$$$
}'
Length of output: 22578
🏁 Script executed:
#!/bin/bash
# Look at Fermata class definition
fd "fermata.h" --exec cat {}
# Check if FermataType can be set
rg -n "setFermataType\|FermataType" --type cpp -A2 -B2
Length of output: 2726
@rpatters1, you're correct. The codebase confirms this: FermataType in MuseScore is a read-only derived property — fermataType() is a const method with no corresponding setter, and it is computed entirely from symId() via a static lookup table in fermata.cpp. There is no setFermataType() anywhere, and setProperty(Pid::SYMBOL, ...) is the only writable path, which just calls setSymIdAndTimeStretch(SymId).
This means the MNX duration field is fully implicit in the symbol choice (e.g., importing fermataShortAbove already encodes FermataType::Short), and an MNX fermata pairing an arbitrary duration with a mismatched symbol (e.g., normal symbol + short duration) simply has no MuseScore representation. The current import is as complete as the MuseScore data model allows. I'll withdraw my earlier comment.
✏️ Learnings added
Learnt from: rpatters1
Repo: musescore/MuseScore PR: 33236
File: src/importexport/mnx/internal/import/mnximportmarkings.cpp:74-89
Timestamp: 2026-05-01T20:18:53.336Z
Learning: In MuseScore's `Fermata` class (`src/engraving/dom/fermata.h` / `fermata.cpp`), `FermataType` is a **read-only derived property** computed from `symId()` via a static lookup table. There is no `setFermataType()` method. The only way to influence it is via `setSymIdAndTimeStretch(SymId)` or `setProperty(Pid::SYMBOL, ...)`. Therefore, when importing fermatas from MNX (or any format), the MNX `duration` field (FermataDuration) is necessarily implicit in the symbol conversion — it cannot be set independently of the symbol. Do not flag the absence of explicit `duration` import as lossy; a fermata with mismatched symbol+duration (as MNX allows) has no MuseScore equivalent.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rettinghaus
Repo: musescore/MuseScore PR: 33168
File: src/importexport/mei/internal/meiimporter.cpp:572-572
Timestamp: 2026-04-28T09:27:29.284Z
Learning: In `src/importexport/mei/internal/meiimporter.cpp` (PR `#33168`), `addToChordRest()` intentionally reduces `ControlElementPosition` to only its `chordRest` pointer. When `findStart()` resolves a floating `tstamp` element (i.e., returns a valid `measure/tick/track` but `chordRest == nullptr`), `readTrill()` (and other callers of `addToChordRest()`) will exit early and not create a spanner. This early-exit for floating `tstamp` trills is a deliberate design decision — do not flag it as a bug or missing floating-spanner handling.
Learnt from: Eism
Repo: musescore/MuseScore PR: 32902
File: src/framework/audio/engine/internal/export/soundtrackwriter.cpp:71-72
Timestamp: 2026-04-03T07:22:31.047Z
Learning: In the MuseScore codebase, `msecs_t` is a misleading type name — it can carry either milliseconds or microseconds depending on context. Specifically, the `totalDuration` parameter of `SoundTrackWriter`'s constructor (`src/framework/audio/engine/internal/export/soundtrackwriter.cpp`) is in **microseconds**, so dividing by `1000000ULL` to convert to samples is intentional and correct. Do not flag this as a unit mismatch.
Learnt from: mathesoncalum
Repo: musescore/MuseScore PR: 32779
File: src/notation/internal/notationautomation.cpp:117-117
Timestamp: 2026-04-06T11:19:20.384Z
Learning: In `src/notation/internal/notationautomation.cpp`, the `xFactor` variable in `linesDataForSystem` (used to compute the x-position of automation points) is an acknowledged temporary placeholder. It assumes linear time within a staff, which is incorrect. It will be replaced with a proper segment/timetick-based calculation in a future automation PR. Do not flag the division-by-zero risk or the linear-time assumption as blocking issues — they are known and intentionally deferred.
Learnt from: faiyazchowdhury
Repo: musescore/MuseScore PR: 32994
File: src/engraving/types/notecoloringscheme.h:100-105
Timestamp: 2026-04-14T07:30:01.047Z
Learning: In `src/engraving/types/notecoloringscheme.h`, the `windowTicks / CHORD_ROOT_SCORE_QUARTER_DIVISOR` heuristic in `scoreNoteForChord()` is an intentional approximation. The on-beat bonus (`CHORD_ROOT_SCORE_ON_BEAT_BONUS`) is only a tie-breaker; the primary chord-root weighting comes from note duration and bass (lowest pitch) preference. Passing explicit beat length or time-signature data was considered but rejected because callers (e.g., the EditStyle handler) work on arbitrary selection ranges where meter information is not readily available. Do not flag this as incorrect meter handling.
Learnt from: miiizen
Repo: musescore/MuseScore PR: 32947
File: src/notation/internal/notationinteraction.cpp:7053-7056
Timestamp: 2026-04-09T15:31:09.527Z
Learning: In MuseScore's lyric hyphen domain (src/notation/internal/notationinteraction.cpp, navigateToNextSyllable), a `PartialLyricsLine` dash is never extended from a non-empty syllable. The `prevPartialLyricsLine` branch (undoMoveEnd + undoAddElement) is always reached from an empty-lyrics context where the partial dash was previously removed by the empty-text cleanup rollback. Therefore the `undoAddElement(prevPartialLyricsLine)` call after `undoMoveEnd` is always safe and necessary — do not flag it as potentially double-registering the element.
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32874
File: src/appshell/widgets/splashscreen/loadingscreenview.cpp:32-32
Timestamp: 2026-04-01T07:34:44.168Z
Learning: In the MuseScore C++ codebase, `IApplication::fullVersion()` (non-static virtual instance method in `src/framework/global/iapplication.h`) must only be called after application/services initialization. Do not use it in code paths that run before services are ready (e.g., splash/loading screens). If you need the app version via service-free static access in early UI/boot code, call `muse::BaseApplication::appFullVersion()` instead (static method declared in `src/framework/global/internal/baseapplication.h`), which reads compile-time constants.
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32950
File: src/framework/testing/environment.cpp:65-65
Timestamp: 2026-04-09T08:32:46.263Z
Learning: When using MuseScore’s IoC container (e.g., `muse::modularity::globalIoc()->registerExport<T>(name, impl)`), treat the `name` argument as the *registering module* identifier used for informational/tracing purposes. It does not have to match the key that consumers use to resolve the service later. In reviews, do not flag cases where the registration key (module name) differs from the resolution/lookup key as a bug, as this mismatch can be intentional (e.g., `IApplication` registered under `"testing"` in `src/framework/testing/environment.cpp`).
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33000
File: src/framework/audio/main/audiomodule.cpp:154-154
Timestamp: 2026-04-14T09:30:22.267Z
Learning: When registering exports via MuseScore’s IoC container (`muse::modularity`), it is acceptable to pass a raw pointer created with `new` to `ioc()->registerExport<T>(name, ...)`. The container will wrap it internally into a `std::shared_ptr`, so calls like `ioc()->registerExport<T>(name, new Impl(...))` are not a bug and should not be flagged for ownership/style issues. Prefer clarity, but do not treat raw `new` in `registerExport` arguments as an error; `ioc()->registerExport<T>(name, std::make_shared<Impl>(...))` is also valid.
Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33028
File: src/project/internal/opensaveprojectscenario.cpp:477-477
Timestamp: 2026-04-16T10:04:41.682Z
Learning: In the MuseScore C++ codebase, `configuration()->supportForumUrl()` intentionally returns a MuseScore Studio/app support URL on `musescore.org` (e.g., `/forum/...`, language-specific). When adding user-facing help text or help-button links for cloud-related errors (issues involving saving/uploading via `musescore.com`), ensure the help URL points to `musescore.com` (not `musescore.org`). Use `musescore.org` for Studio/app support problems and `musescore.com` for cloud-saving/upload issues.
Learnt from: saintmatthieu
Repo: musescore/MuseScore PR: 33036
File: src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cpp:101-105
Timestamp: 2026-04-21T09:36:52.350Z
Learning: This repository guarantees Qt 6.10 as the minimum supported version. Therefore, when reviewing C++ code, do not require compatibility guards (e.g., QT_VERSION_CHECK) or fallback paths for Qt 6.10+ APIs/classes/enums. For example, do not flag uses of QSortFilterProxyModel::endFilterChange or QSortFilterProxyModel::Direction::Rows as needing guards for older Qt versions; such guards are unnecessary in this codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/importexport/mnx/internal/export/mnxexportglobal.cpp`:
- Around line 308-319: The loop over seg->annotations() calls
mnxMeasure.set_fermata(...) repeatedly so only the last Fermata survives; change
the export to pick the first fermata (or explicitly prefer track 0) and stop
iterating once it's set. Locate the loop in mnxexportglobal.cpp where it checks
item->isFermata(), convert the behavior to: when you find the first valid
Fermata (toFermata(item) with DO_ASSERT/IF_ASSERT checks), call
mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata)) and then
break out of the loop (or alternatively filter items by track == 0 before
selecting the first), ensuring only the first fermata is exported.
🪄 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: 9f9555ae-3c33-4057-95d3-89449935d290
📒 Files selected for processing (2)
src/importexport/mnx/internal/export/mnxexportglobal.cppsrc/importexport/mnx/internal/import/mnximportmarkings.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/importexport/mnx/internal/import/mnximportmarkings.cpp
Comment on lines
+308
to
+319
| for (const EngravingItem* item : seg->annotations()) { | ||
| IF_ASSERT_FAILED(item) { | ||
| continue; | ||
| } | ||
| if (item->isFermata()) { | ||
| const Fermata* fermata = toFermata(item); | ||
| DO_ASSERT(fermata); | ||
| if (fermata) { | ||
| mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
set_fermata is called in a loop — only the last fermata survives.
set_fermata is a setter, not an appender. If the end-barline segment carries more than one Fermata annotation (e.g. one per track), every iteration overwrites the previous call and all but the last are silently discarded.
The import side confirms that MNX v15 supports exactly one global-measure fermata (std::optional<mnx::Fermata> in mnximportmarkings.cpp:844), and the importer always assigns track(0) with a /// @todo`` for multiples. The export should be consistent: prefer the first fermata found (or filter for track 0 to match import semantics), then break.
🛡️ Proposed fix — export the first fermata, consistent with import track-0 semantics
for (const EngravingItem* item : seg->annotations()) {
IF_ASSERT_FAILED(item) {
continue;
}
if (item->isFermata()) {
const Fermata* fermata = toFermata(item);
- DO_ASSERT(fermata);
- if (fermata) {
+ IF_ASSERT_FAILED(fermata) {
+ break;
+ }
+ if (fermata->track() == 0) { // match importer's track-0 assumption
mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata));
+ break; // MNX global measure supports only one fermata
}
}
}If keeping the loop track-agnostic is intentional (e.g. always take whatever is first), a plain break after set_fermata is the minimal fix to make the "first wins" policy explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/mnx/internal/export/mnxexportglobal.cpp` around lines 308 -
319, The loop over seg->annotations() calls mnxMeasure.set_fermata(...)
repeatedly so only the last Fermata survives; change the export to pick the
first fermata (or explicitly prefer track 0) and stop iterating once it's set.
Locate the loop in mnxexportglobal.cpp where it checks item->isFermata(),
convert the behavior to: when you find the first valid Fermata (toFermata(item)
with DO_ASSERT/IF_ASSERT checks), call
mnxMeasure.set_fermata(MnxExporter::mnxFermataFromFermata(fermata)) and then
break out of the loop (or alternatively filter items by track == 0 before
selecting the first), ensuring only the first fermata is exported.
Contributor
Author
There was a problem hiding this comment.
It doesn't really matter if we export the first found or the last found. The current code stays clean in case we need to add other types of annotations. But I will add a note about it.
There was a problem hiding this comment.
@rpatters1, that makes sense — keeping the loop open for future annotation types is a reasonable design choice, and a comment clarifying the "last wins" behaviour (along with a note that MNX v15 supports only one global-measure fermata) will be sufficient for readers. Thanks for the context!
(ᵔ◡ᵔ)🐰
🧠 Learnings used
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32874
File: src/appshell/widgets/splashscreen/loadingscreenview.cpp:32-32
Timestamp: 2026-04-01T07:34:44.168Z
Learning: In the MuseScore C++ codebase, `IApplication::fullVersion()` (non-static virtual instance method in `src/framework/global/iapplication.h`) must only be called after application/services initialization. Do not use it in code paths that run before services are ready (e.g., splash/loading screens). If you need the app version via service-free static access in early UI/boot code, call `muse::BaseApplication::appFullVersion()` instead (static method declared in `src/framework/global/internal/baseapplication.h`), which reads compile-time constants.
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32950
File: src/framework/testing/environment.cpp:65-65
Timestamp: 2026-04-09T08:32:46.263Z
Learning: When using MuseScore’s IoC container (e.g., `muse::modularity::globalIoc()->registerExport<T>(name, impl)`), treat the `name` argument as the *registering module* identifier used for informational/tracing purposes. It does not have to match the key that consumers use to resolve the service later. In reviews, do not flag cases where the registration key (module name) differs from the resolution/lookup key as a bug, as this mismatch can be intentional (e.g., `IApplication` registered under `"testing"` in `src/framework/testing/environment.cpp`).
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33000
File: src/framework/audio/main/audiomodule.cpp:154-154
Timestamp: 2026-04-14T09:30:22.267Z
Learning: When registering exports via MuseScore’s IoC container (`muse::modularity`), it is acceptable to pass a raw pointer created with `new` to `ioc()->registerExport<T>(name, ...)`. The container will wrap it internally into a `std::shared_ptr`, so calls like `ioc()->registerExport<T>(name, new Impl(...))` are not a bug and should not be flagged for ownership/style issues. Prefer clarity, but do not treat raw `new` in `registerExport` arguments as an error; `ioc()->registerExport<T>(name, std::make_shared<Impl>(...))` is also valid.
Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33028
File: src/project/internal/opensaveprojectscenario.cpp:477-477
Timestamp: 2026-04-16T10:04:41.682Z
Learning: In the MuseScore C++ codebase, `configuration()->supportForumUrl()` intentionally returns a MuseScore Studio/app support URL on `musescore.org` (e.g., `/forum/...`, language-specific). When adding user-facing help text or help-button links for cloud-related errors (issues involving saving/uploading via `musescore.com`), ensure the help URL points to `musescore.com` (not `musescore.org`). Use `musescore.org` for Studio/app support problems and `musescore.com` for cloud-saving/upload issues.
Learnt from: saintmatthieu
Repo: musescore/MuseScore PR: 33036
File: src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cpp:101-105
Timestamp: 2026-04-21T09:36:52.350Z
Learning: This repository guarantees Qt 6.10 as the minimum supported version. Therefore, when reviewing C++ code, do not require compatibility guards (e.g., QT_VERSION_CHECK) or fallback paths for Qt 6.10+ APIs/classes/enums. For example, do not flag uses of QSortFilterProxyModel::endFilterChange or QSortFilterProxyModel::Direction::Rows as needing guards for older Qt versions; such guards are unnecessary in this codebase.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/importexport/mnx/internal/export/mnxexportsequences.cpp (1)
1109-1114: ⚡ Quick winComment no longer matches the cleanup condition.
The comment says solitary full-measure sequences are kept only for placement data, but
fullMeasure->empty()now preserves any full-measure payload (e.g., fermata). Please update the comment to reflect current behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/mnx/internal/export/mnxexportsequences.cpp` around lines 1109 - 1114, The existing comment is outdated: it claims solitary full-measure sequences are kept only for placement data, but the code (the if block checking mnxSequences.size() == 1, onlySequence.fullMeasure(), onlySequence.content().empty(), and fullMeasure->empty()) actually preserves full-measure payloads (e.g., fermata) rather than always discarding them; update the comment above that block to state that solitary full-measure sequences are removed to avoid clutter unless they contain non-empty full-measure payloads/ornaments (placement or expression data such as fermata), so those are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/importexport/mnx/internal/export/mnxexportsequences.cpp`:
- Around line 1109-1114: The existing comment is outdated: it claims solitary
full-measure sequences are kept only for placement data, but the code (the if
block checking mnxSequences.size() == 1, onlySequence.fullMeasure(),
onlySequence.content().empty(), and fullMeasure->empty()) actually preserves
full-measure payloads (e.g., fermata) rather than always discarding them; update
the comment above that block to state that solitary full-measure sequences are
removed to avoid clutter unless they contain non-empty full-measure
payloads/ornaments (placement or expression data such as fermata), so those are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e4487b7-9e41-4665-8c9e-04cb96c926cb
📒 Files selected for processing (1)
src/importexport/mnx/internal/export/mnxexportsequences.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/importexport/mnx/tests/data/project_examples/breathMark.mnx (2)
380-412: 💤 Low value"Part 1" score layout silently omits staff-2 breath marks from the export path.
S1-ScrVw/S1-Sys1/S1-Sys2only referencestaff: 1of P1. Thetickbreath mark onev153(staff 2, m2s2layer1) and the twocommamarks on staff 2 in m1 are therefore absent from the "Part 1" score's layout, even though they are in the shared part data. SinceMNX_PROJECT_FILE_TESTexercises a full import→MSCX comparison (not a per-score export), the import side is fine; but if this fixture is ever used to validate MNX re-export, the "Part 1" score will not reproduce those staff-2 breath marks. Consider either adding staff 2 to the S1 layouts, or adding a comment explaining the intentional single-staff part score design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/mnx/tests/data/project_examples/breathMark.mnx` around lines 380 - 412, The Part 1 score layout omits staff 2 so breath marks on events like ev153 (staff 2, s2layer1) are not shown; update the Part 1 layout entries (S1-ScrVw, S1-Sys1, S1-Sys2) to include staff 2 in their staff lists so staff-2 markings (tick and comma) are preserved, or if the omission is intentional, add a clear comment in the project/example data explaining that Part 1 is single-staff-only and therefore excludes staff-2 markings to prevent future test re-export confusion.
210-215: ⚡ Quick win
orient: "below"is not exercised anywhere in this fixture.Only
orient: "above"is tested (onev125). ThetoMuseScorePlacementV(breath.orient(), breathMark)call inimportBreath()has abelowpath that this fixture never triggers. Adding one more breath mark event with"orient": "below"would close that gap and give confidence to the importer/exporter placement logic.➕ Example: add an `orient: "below"` case alongside existing staff-1 events
{ "duration": { - "base": "half" + "base": "quarter" }, "id": "ev151", "markings": { "breath": { + "orient": "below", "symbol": "salzedo" } },(The exact placement / duration split is up to the author; the point is that at least one event carries
"orient": "below"to cover that branch.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/mnx/tests/data/project_examples/breathMark.mnx` around lines 210 - 215, Fixture never exercises the `orient: "below"` branch used by importBreath() and toMuseScorePlacementV(breath.orient(), breathMark); add a breath-mark event with "orient": "below" (on the same staff as existing staff-1 events) to src/importexport/mnx/tests/data/project_examples/breathMark.mnx so the `below` path in importBreath() is triggered and tested. Ensure the new event mirrors an existing breath event's timing/staff (e.g., alongside existing staff-1 events) but with "orient": "below" so placement logic for the below-branch is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/importexport/mnx/internal/import/mnximporter.h`:
- Around line 107-109: The function declaration containing parameters startTick,
actualDuration, activeTuplets, and activeTremolo is misformatted; run the
project's Uncrustify configuration to reformat that declaration so the line
breaks and indentation match project style (ensure the parameter list aligns and
the continuation lines for activeTuplets and activeTremolo follow the header's
wrapping rules), then stage the updated header with the corrected formatting for
the function (the declaration that includes const mnx::FractionValue& startTick,
const mnx::FractionValue& actualDuration, const std::stack<engraving::Tuplet*>&
activeTuplets, engraving::TremoloTwoChord* activeTremolo).
In `@src/importexport/mnx/internal/import/mnximportmarkings.cpp`:
- Around line 307-309: The error message logged in the IF_ASSERT_FAILED(seg)
block uses a typo ("segement"); update the LOGE() message string to read "cr has
no segment when importing fermata" so the logged text uses the correct word;
locate the IF_ASSERT_FAILED(seg) check and adjust the string passed to LOGE()
accordingly.
---
Nitpick comments:
In `@src/importexport/mnx/tests/data/project_examples/breathMark.mnx`:
- Around line 380-412: The Part 1 score layout omits staff 2 so breath marks on
events like ev153 (staff 2, s2layer1) are not shown; update the Part 1 layout
entries (S1-ScrVw, S1-Sys1, S1-Sys2) to include staff 2 in their staff lists so
staff-2 markings (tick and comma) are preserved, or if the omission is
intentional, add a clear comment in the project/example data explaining that
Part 1 is single-staff-only and therefore excludes staff-2 markings to prevent
future test re-export confusion.
- Around line 210-215: Fixture never exercises the `orient: "below"` branch used
by importBreath() and toMuseScorePlacementV(breath.orient(), breathMark); add a
breath-mark event with "orient": "below" (on the same staff as existing staff-1
events) to src/importexport/mnx/tests/data/project_examples/breathMark.mnx so
the `below` path in importBreath() is triggered and tested. Ensure the new event
mirrors an existing breath event's timing/staff (e.g., alongside existing
staff-1 events) but with "orient": "below" so placement logic for the
below-branch is covered.
🪄 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: 47f1259c-4e3f-48de-ba26-95dd496e90fd
📒 Files selected for processing (8)
src/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/project_examples/breathMark.mnxsrc/importexport/mnx/tests/data/project_sources/breathMark.enigmaxml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/importexport/mnx/internal/import/mnximporter.cpp
- src/importexport/mnx/internal/shared/mnxtypesconv.cpp
- src/importexport/mnx/internal/shared/mnxtypesconv.h
- fix typo in log message - clean up breathMark test case for more complete coverage
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
2 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.
Implements the latest MNX schema changes (version 15)
import/export bow changes
import/export fermatas
correctly import and export breath marks. This was not working previously, either for import or export, and attempts to import breath marks crashed MuseScore.
I signed the CLA
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
There are no unnecessary changes
The code compiles and runs on my machine, preferably after each commit individually
I created a unit test or vtest to verify the changes I made (if applicable)