[MusicXML] Fix octave-shift#34163
Conversation
|
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 skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMusicXML octave-shift import and export now support size 22 and corresponding three-octave ottava types. Pitch conversion recognizes ±36-semitone deltas, while ottava serialization emits size 22 start and stop directions. Direction export no longer derives placement attributes from ottava spanners. A new octave-shift round-trip fixture and test were added, and existing expected XML fixtures were updated to omit direction placement attributes. 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/musicxml/internal/import/importmusicxmlpass1.cpp`:
- Around line 3289-3297: The octave-shift size switch must distinguish the valid
stop value from other unknown sizes. Update the switch in the pass1 octave-shift
handling to map size 0 to the stop case, retain the existing mappings for 22,
15, and 8, and log an error for any other value instead of assigning sz = 1;
preserve pass2 synchronization by ensuring invalid sizes are not silently
treated as one-octave shifts.
In `@src/importexport/musicxml/tests/data/testOctaveShift.xml`:
- Around line 223-224: Add a focused MusicXML fixture or import assertion
covering an octave-shift stop element without the size attribute, alongside the
existing testOctaveShift coverage. Ensure the test verifies successful import
and the expected round-trip or canonicalized output, while retaining the
existing sized-stop cases.
🪄 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: 45df6b53-d7fe-4b54-8dc8-0317369d0e54
📒 Files selected for processing (12)
src/importexport/musicxml/internal/export/exportmusicxml.cppsrc/importexport/musicxml/internal/import/importmusicxmlpass1.cppsrc/importexport/musicxml/internal/import/importmusicxmlpass2.cppsrc/importexport/musicxml/tests/data/testColorExport_ref.xmlsrc/importexport/musicxml/tests/data/testExcludeInvisibleElements_invisible_ref.xmlsrc/importexport/musicxml/tests/data/testExcludeInvisibleElements_noinvisible_ref.xmlsrc/importexport/musicxml/tests/data/testLayout.xmlsrc/importexport/musicxml/tests/data/testLines1.xmlsrc/importexport/musicxml/tests/data/testOctaveShift.xmlsrc/importexport/musicxml/tests/data/testOverlappingSpanners.xmlsrc/importexport/musicxml/tests/data/testTimeTickExport_ref.xmlsrc/importexport/musicxml/tests/musicxml_tests.cpp
| <octave-shift type="stop" size="8" number="1"/> | ||
| </direction-type> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for a stop without size.
Every stop in this fixture includes size, so the new round-trip test does not verify the stated compatibility case. Add a focused import assertion or separate expected-output fixture, since export may canonicalize the stop by adding size.
Also applies to: 317-318, 411-412, 505-506, 599-600, 693-694
🤖 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/data/testOctaveShift.xml` around lines 223 -
224, Add a focused MusicXML fixture or import assertion covering an octave-shift
stop element without the size attribute, alongside the existing testOctaveShift
coverage. Ensure the test verifies successful import and the expected round-trip
or canonicalized output, while retaining the existing sized-stop cases.
Backport of musescore#34163 Co-Authored-By: Klaus Rettinghaus <rettinghaus@users.noreply.github.com>
Backport of musescore#34163 Co-Authored-By: Klaus Rettinghaus <rettinghaus@users.noreply.github.com>
Co-authored-by: James Mizen <jamesmmizen@gmail.com>
3 participants
Resolves: #34087
Fixes wrong (unterminated) import of ottava lines and extends support to 22va and 22ba lines. Furthermore it removes the superfluous
placementfor octave-shift directions.