MusicXML: import multi-child dynamics as a single marking

[rpatters1]

Resolves: #34439

MusicXML allows a single <dynamics> element to hold several children, and the specification calls this out explicitly: "Dynamics elements may also be combined to create marks not covered by a single element, such as <sf/><mp/>." The element is one marking; its children spell out the glyph sequence it is drawn with.

The importer created one Dynamic per child instead, so such a marking rendered as several dynamics stacked on top of each other at the same tick. The file attached to the issue has three of them, and none of them read correctly.

The fix

MusicXmlParserDirection::dynamics() and MusicXmlParserNotations::dynamics() now collect the children of each <dynamics> element into a single Dynamic instead of pushing one entry to m_dynamicsList per child.

Each child is converted to the SMuFL markup MuseScore spells it with before being concatenated. Their names cannot be concatenated directly: "sf" + "mp" gives "sfmp", which names no dynamic, so Dynamic::setDynamicType() would leave it as plain text with no glyphs. Concatenating markup instead lets Dynamic::parseDynamicText() do its normal job. A combination that names a type still resolves to it, and one that does not stays DynamicType::OTHER while keeping the correct glyphs.

This also makes m_dynamicsList uniformly markup. It was already mixed, since textToDynamic() pushes Dynamic::dynamicText() into the same list.

Source	Before	After
<sf/><mp/>	2 dynamics, overlapping	1 dynamic, OTHER, glyphs s f m p
<other-dynamics>s</other-dynamics><f/><other-dynamics>m</other-dynamics><p/>	4 dynamics, overlapping	same as above
<ff/><other-dynamics>z</other-dynamics>	2 dynamics, overlapping	1 dynamic, OTHER, glyphs f f z
<other-dynamics>s</other-dynamics><f/>	2 dynamics, overlapping	1 dynamic, DynamicType::SF

Dynamics that name a MuseScore type are unaffected, which is the overwhelming majority of real-world files: a single-child <dynamics> produces exactly what it did before.

One deliberate consequence: a combination MuseScore has no type for is DynamicType::OTHER, whose velocity is -1, so it has no playback effect unless the direction also carries <sound dynamics="_"/>. That matches how MuseScore treats any dynamic whose text it cannot classify, including ones typed by hand.

Tests

testMultipleDynamics covers all four cases in the table, including one combination that resolves to a real type so the classification path is exercised too.

inferredDynamicRange is the existing test most exposed to this change, since isLikelyDynamicRange() tests m_dynamicsList.size() == 2. That still holds: a dynamic range is three sibling <direction-type> elements, each holding a single-child <dynamics>, not one element with two children.

Full suite passes on macOS apart from inferredCredits1 and inferredCredits2, which fail on main as well. Both differ only in a vbox <height> (13.9828 vs 13.9746), the platform font-metric divergence the suite's DISABLED_EXCEPT_ON_LINUX macro exists for.

---

Proposal: multi-glyph dynamics on the export side

I kept this PR to the reported bug, but the same idea has more to give, and I would rather show you the whole picture than open a surprise follow-up. This section is a proposal, not part of the diff. I am happy to fold it into this PR, do it as a follow-up, or drop it entirely — whichever you prefer.

The observation behind #34439 is that a MusicXML <dynamics> element is a sequence, so it can describe markings that no single element names. The importer now reads that. The exporter still does not write it.

1. Decompose glyph runs on export. ExportMusicXml::dynamic() writes any dynamic it cannot match to a single MusicXML element as one <other-dynamics> blob, so an sfmp marking exports as <other-dynamics>sfmp</other-dynamics> — text that other applications will render as literal letters. Emitting the longest valid MusicXML elements found in the glyph run, with <other-dynamics> only for fragments that have no element, would give:

• sfmp → <sf/><mp/>
• ffz → <ff/><other-dynamics>z</other-dynamics>
• sff, sfff, sfffz → <sf/><f/>, <sf/><f/><f/>, <sf/><f/><f/><other-dynamics>z</other-dynamics> (MuseScore types with no MusicXML element)
• m, r, s, z → unchanged; single glyphs with no MusicXML element and nothing to decompose

Combined with this PR, sfmp and ffz would then round-trip exactly. Note this changes testDynamics1.xml, which contains <other-dynamics>sff</other-dynamics> and is a musicXmlIoTest; converting it to musicXmlIoTestRef with a _ref.xml keeps the input file untouched and makes the intended output change visible in review.

2. Read the glyphs rather than a codepoint table. The export path maps plainText() characters against a hardcoded table of the seven single-letter SMuFL dynamics (U+E520–U+E526). Ligature glyphs such as dynamicFF, dynamicMF, dynamicSforzato and dynamicPPPP are not in it, so a dynamic containing one currently exports as a raw private-use character inside <other-dynamics>. Walking Dynamic::fragmentList() and resolving SymIds through IEngravingFontsProvider would handle those, and would also separate leading and trailing plain text from the glyph run properly.

3. The matching import gap. A single element MuseScore cannot classify still becomes plain text with no glyphs — <sfzp/>, which is valid MusicXML but has no MuseScore DynamicType, and hand-authored text such as <other-dynamics>sfmp</other-dynamics>. Decomposing the letters into glyphs would fix both. This becomes required rather than merely nice if (1) is done, because sfzp is a valid MusicXML dynamic: a glyph run spelling sfzp would export as <sfzp/> and would not survive the round trip back.

The MNX module already does all of this and could be drawn on: toMnxDynamicFromSymIds() and dynamicGlyphLetters() in internal/shared/mnxtypesconv.cpp derive glyph-to-letter spellings from MuseScore's own DynamicType table rather than a maintained second table, and splitDynamicText() in internal/export/mnxexportparts.cpp does the fragment walk in (2). Whether that is worth extracting somewhere both modules can use, or is better duplicated in the MusicXML module, is a question I would want your steer on.

• 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).

[coderabbitai]

📝 Walkthrough

Walkthrough

MusicXML dynamic parsing now converts recognized names to MuseScore SMuFL markup and preserves unrecognized other-dynamics text. Each <dynamics> node produces one combined marking for direction-level and note-notation dynamics. A regression test adds MusicXML and MuseScore reference fixtures covering compound, standard, custom, and velocity-based dynamics.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main MusicXML import change.
Description check	✅ Passed	The description explains the problem, implementation, tests, linked issue, and checklist status in sufficient detail.
Linked Issues check	✅ Passed	The changes satisfy issue #34439 by combining multi-child dynamics into one marking and preserving the complete glyph sequence.
Out of Scope Changes check	✅ Passed	All code and test changes support issue #34439; the additional exporter discussion is explicitly identified as a proposal outside the diff.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped musescore/muse_framework.git.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/importexport/musicxml/tests/musicxml_tests.cpp (1)

1027-1029: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add notation-level dynamics coverage.

testMultipleDynamics.xml contains only direction-level <dynamics> elements. The test does not execute MusicXmlParserNotations::dynamics(). Add a compound <note><notations><dynamics> case and its expected Dynamic entry in testMultipleDynamics_ref.mscx.

🤖 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/musicxml/tests/musicxml_tests.cpp` around lines 1027 - 1029,
Extend the testMultipleDynamics fixture to include a compound note containing
notations-level dynamics, ensuring MusicXmlParserNotations::dynamics() is
exercised. Update testMultipleDynamics_ref.mscx with the corresponding expected
Dynamic entry while preserving the existing direction-level dynamics coverage.

🤖 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.

Nitpick comments:
In `@src/importexport/musicxml/tests/musicxml_tests.cpp`:
- Around line 1027-1029: Extend the testMultipleDynamics fixture to include a
compound note containing notations-level dynamics, ensuring
MusicXmlParserNotations::dynamics() is exercised. Update
testMultipleDynamics_ref.mscx with the corresponding expected Dynamic entry
while preserving the existing direction-level dynamics coverage.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c202e9d-6a47-43f3-8d4d-f3c3142ee16f

📥 Commits

Reviewing files that changed from the base of the PR and between f4cdc6b and 0190cbc.

📒 Files selected for processing (4)

• src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp
• src/importexport/musicxml/tests/data/testMultipleDynamics.xml
• src/importexport/musicxml/tests/data/testMultipleDynamics_ref.mscx
• src/importexport/musicxml/tests/musicxml_tests.cpp

[rpatters1]

CI here is apparently red from a GitHub Actions outage rather than from this change.

All three actual failures are at the Set up job step, before anything is built, and carry the same error:

Failed to resolve action download info. Error: Service Unavailable

build (linux_arm64) (16:04–16:08 UTC), add_to_projects (16:07–16:10) and run_tests (16:22–16:24) each retried three times and gave up. The remaining red entries — build (linux_x64), windows_x64, Setup VTests workflow and the second run_tests — were cancelled by the matrix fail-fast, not failed. Every job whose runner came up outside that window passed, including macos_universal.

Could someone with write access re-run the failed jobs? run_tests never actually executed, so the unit tests are still unverified on CI.