[MusicXML] export images#33539
Open
rettinghaus wants to merge 4 commits into
Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughThis PR adds image export to MusicXML. The ImageStoreItem class gains a 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/export/exportmusicxml.cpp`:
- Around line 5193-5228: The image() method in ExportMusicXml currently always
emits an image source attribute (via getImageInfo) even when the plain .musicxml
export path (saveXml) will not materialize binary assets, causing broken
references; modify ExportMusicXml::image to check the export mode (the flag used
by saveXml to decide .mxl packaging) and: if exporting plain XML, either (A)
write the image binary out as a sidecar file next to the XML and set the source
attribute to that relative path (use ImageStoreItem/getImageInfo to obtain
binary and filename), or (B) omit emitting the <image> tag and call the existing
warning/logging path to surface the omission; ensure this same gate is applied
to the code block around the saveXml/.mxl handling (the code referenced near
lines ~8960) so behavior is consistent between compressed and uncompressed
exports.
🪄 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: 59f40812-2310-4c5a-9b4c-6909e1b3bca8
📒 Files selected for processing (2)
src/engraving/dom/imageStore.hsrc/importexport/musicxml/internal/export/exportmusicxml.cpp
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/importexport/musicxml/internal/export/exportmusicxml.cpp (1)
5228-5232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEncode plain-XML image omission explicitly.
The plain
.musicxmlbranch currently writes only the hashedsourcetext in<other-direction>, which is not an explicit/machine-readable “image omitted” marker for downstream consumers.💡 Suggested local change
} else { String otherTag = u"other-direction"; otherTag += positioningAttributes(img); - m_xml.tagRaw(otherTag, XmlWriter::xmlString(source)); + const String omittedInfo = String(u"musescore:image-omitted source=\"%1\" type=\"%2\"") + .arg(XmlWriter::xmlString(source), XmlWriter::xmlString(type)); + m_xml.tagRaw(otherTag, omittedInfo); }🤖 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/internal/export/exportmusicxml.cpp` around lines 5228 - 5232, The current else branch writes only the hashed source via m_xml.tagRaw(otherTag, XmlWriter::xmlString(source)), which is not an explicit machine-readable “image omitted” marker; update the code in that branch (around otherTag, positioningAttributes(img), m_xml.tagRaw and XmlWriter::xmlString(source)) to emit a clear, standardized marker (for example an explicit <image-omitted/> element or a named token inside <other-direction>) while preserving the original hash for traceability (include the hash as an attribute or child element) so downstream consumers can detect omitted images reliably.
🤖 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.
Duplicate comments:
In `@src/importexport/musicxml/internal/export/exportmusicxml.cpp`:
- Around line 5228-5232: The current else branch writes only the hashed source
via m_xml.tagRaw(otherTag, XmlWriter::xmlString(source)), which is not an
explicit machine-readable “image omitted” marker; update the code in that branch
(around otherTag, positioningAttributes(img), m_xml.tagRaw and
XmlWriter::xmlString(source)) to emit a clear, standardized marker (for example
an explicit <image-omitted/> element or a named token inside <other-direction>)
while preserving the original hash for traceability (include the hash as an
attribute or child element) so downstream consumers can detect omitted images
reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd863681-b995-4c45-8485-1fbb7e5d76c5
📒 Files selected for processing (1)
src/importexport/musicxml/internal/export/exportmusicxml.cpp
| */ | ||
|
|
||
| if (text->plainText() == "") { | ||
| if (text->plainText() == u"") { |
Contributor
There was a problem hiding this comment.
Why not if (text->plainText().isEmpty()) {?
Contributor
Author
There was a problem hiding this comment.
There were more cases, so I unified them all.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/importexport/musicxml/internal/export/exportmusicxml.cpp (2)
5154-5188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd TIFF magic-byte detection to the fallback.
tif/tiffis only recognized fromImageStoreItem::type(). If the suffix is missing or wrong, this path falls back toapplication/octet-stream, so TIFF images lose their real MIME type even though TIFF is otherwise supported.Suggested fix
if (type.isEmpty()) { const ByteArray& ba = isi->buffer(); if (ba.size() >= 4) { if (ba.at(0) == 0x89 && ba.at(1) == 0x50 && ba.at(2) == 0x4E && ba.at(3) == 0x47) { type = u"image/png"; if (suffix.isEmpty()) { source += u".png"; } } else if (ba.at(0) == 0xFF && ba.at(1) == 0xD8 && ba.at(2) == 0xFF) { type = u"image/jpeg"; if (suffix.isEmpty()) { source += u".jpg"; } } else if (ba.at(0) == 0x47 && ba.at(1) == 0x49 && ba.at(2) == 0x46 && ba.at(3) == 0x38) { type = u"image/gif"; if (suffix.isEmpty()) { source += u".gif"; } } else if (ba.at(0) == 0x42 && ba.at(1) == 0x4D) { type = u"image/bmp"; if (suffix.isEmpty()) { source += u".bmp"; } + } else if ((ba.at(0) == 0x49 && ba.at(1) == 0x49 && ba.at(2) == 0x2A && ba.at(3) == 0x00) + || (ba.at(0) == 0x4D && ba.at(1) == 0x4D && ba.at(2) == 0x00 && ba.at(3) == 0x2A)) { + type = u"image/tiff"; + if (suffix.isEmpty()) { + source += u".tif"; + } } else if (ba.at(0) == 0x3C && (ba.at(1) == 0x3F || ba.at(1) == 0x73)) { type = u"image/svg+xml"; if (suffix.isEmpty()) { source += u".svg"; } } } }🤖 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/internal/export/exportmusicxml.cpp` around lines 5154 - 5188, The fallback MIME sniffing block (using isi->buffer(), type, suffix, source) lacks TIFF detection; add an else-if that checks the first four bytes for TIFF signatures (little-endian: 0x49,0x49,0x2A,0x00 or big-endian: 0x4D,0x4D,0x00,0x2A) and set type = u"image/tiff" and, if suffix.isEmpty(), append an appropriate extension to source (e.g., u".tif" or u".tiff"); keep this branch alongside the other image checks before the final fallback to application/octet-stream.
5228-5235:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExpose plain-XML image omissions so the caller can warn.
This branch drops the binary image and writes only an
other-directionplaceholder, but it never records that an omission happened.saveXml()only returnsbool, so the exporter gives callers no way to surface the warning required by#32688without duplicating a separate score scan.🤖 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/internal/export/exportmusicxml.cpp` around lines 5228 - 5235, The export currently writes an "other-direction" placeholder for omitted binary images in the branch that calls m_xml.tagRaw(u"other-direction"...) but never records that an omission occurred, so callers of saveXml() cannot warn; modify the export path to set a persistent omission indicator (e.g., a member bool like imageOmitted or an out-parameter/status return from saveXml()) when the code in exportmusicxml.cpp that constructs otherTag and calls m_xml.tagRaw runs, and propagate that flag back to the public saveXml() caller (or change saveXml() to return a status/enum instead of plain bool) so callers can detect and surface the image-omission warning; ensure the flag is reset/initialized appropriately and referenced by downstream callers instead of duplicating score scans.
🤖 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.
Outside diff comments:
In `@src/importexport/musicxml/internal/export/exportmusicxml.cpp`:
- Around line 5154-5188: The fallback MIME sniffing block (using isi->buffer(),
type, suffix, source) lacks TIFF detection; add an else-if that checks the first
four bytes for TIFF signatures (little-endian: 0x49,0x49,0x2A,0x00 or
big-endian: 0x4D,0x4D,0x00,0x2A) and set type = u"image/tiff" and, if
suffix.isEmpty(), append an appropriate extension to source (e.g., u".tif" or
u".tiff"); keep this branch alongside the other image checks before the final
fallback to application/octet-stream.
- Around line 5228-5235: The export currently writes an "other-direction"
placeholder for omitted binary images in the branch that calls
m_xml.tagRaw(u"other-direction"...) but never records that an omission occurred,
so callers of saveXml() cannot warn; modify the export path to set a persistent
omission indicator (e.g., a member bool like imageOmitted or an
out-parameter/status return from saveXml()) when the code in exportmusicxml.cpp
that constructs otherTag and calls m_xml.tagRaw runs, and propagate that flag
back to the public saveXml() caller (or change saveXml() to return a status/enum
instead of plain bool) so callers can detect and surface the image-omission
warning; ensure the flag is reset/initialized appropriately and referenced by
downstream callers instead of duplicating score scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd9a9bdd-f6dc-4bba-8e48-68a06f2dfa8e
📒 Files selected for processing (1)
src/importexport/musicxml/internal/export/exportmusicxml.cpp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/importexport/musicxml/internal/export/exportmusicxml.cpp (1)
5234-5238:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the plain
.musicxmlfallback explicit and user-visible.On Lines 5234-5238, the non-
.mxlpath still silently drops the binary and emits only thesourcestring insideother-direction. That misses the uncompressed-export contract from the linked issue: callers have no warning to surface, and consumers cannot reliably tell this is an omitted-image placeholder or recover the original MIME type. Please thread a warning out of the plain-export path and serialize a stable omission marker that includes at leastsourceandtype.🤖 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/internal/export/exportmusicxml.cpp` around lines 5234 - 5238, The non-.mxl branch in exportmusicxml.cpp currently writes only the raw source string into other-direction (see otherTag, positioningAttributes(img), m_xml.tagRaw(XmlWriter::xmlString(source))) and must instead emit an explicit omission marker and a user-visible warning; update that branch to (1) log a warning via the module's logging facility indicating the image was omitted from uncompressed .musicxml export, and (2) serialize a stable omission element inside other-direction that includes at minimum the original source and the image MIME/type (obtainable from img or its metadata) so consumers can detect the omission and recover type information instead of seeing an ambiguous raw source string. Ensure you modify the code that builds otherTag and the m_xml.tagRaw call to write the structured omission marker (source + type) and to call the logger.
🤖 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.
Outside diff comments:
In `@src/importexport/musicxml/internal/export/exportmusicxml.cpp`:
- Around line 5234-5238: The non-.mxl branch in exportmusicxml.cpp currently
writes only the raw source string into other-direction (see otherTag,
positioningAttributes(img), m_xml.tagRaw(XmlWriter::xmlString(source))) and must
instead emit an explicit omission marker and a user-visible warning; update that
branch to (1) log a warning via the module's logging facility indicating the
image was omitted from uncompressed .musicxml export, and (2) serialize a stable
omission element inside other-direction that includes at minimum the original
source and the image MIME/type (obtainable from img or its metadata) so
consumers can detect the omission and recover type information instead of seeing
an ambiguous raw source string. Ensure you modify the code that builds otherTag
and the m_xml.tagRaw call to write the structured omission marker (source +
type) and to call the logger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23ed7484-ab85-4c0c-aaa8-2cf5fbae3e52
📒 Files selected for processing (1)
src/importexport/musicxml/internal/export/exportmusicxml.cpp
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
4 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.
Resolves: #32688
This adds support for images to the MusicXML export. It checks the type of the imageStore for correctly assigning the mime type (a small method has been added there). If the suffix isn't sufficient, it tries to get it from the magic byte.
Images are stored in compressed MusicXML files.