[MusicXML] add support for straight and wavy chord lines#33346
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request adds bidirectional MusicXML support for chord-line line-shape and line-type attributes. The export layer writes 🚥 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: 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/import/importmusicxmlpass2.cpp`:
- Around line 9231-9233: The code currently treats a missing/empty lineType as
an unsupported error; update the conditional around logger->logError so it only
logs when lineType is present and not "solid" (e.g. check for
non-empty/has-value before comparing to u"solid"). In practice change the branch
that now reads else if (lineType != u"solid") to something like else if
(!lineType.empty() && lineType != u"solid") (or the equivalent
isEmpty()/hasValue() check used in this codebase) so omitted line-type is
treated as the default solid/no-op and the error is only emitted for explicit,
unsupported values; keep the xmlreader parameter in the log call as-is.
🪄 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: f2ae2f75-6547-4b89-9f47-7910557c417f
📒 Files selected for processing (2)
src/importexport/musicxml/internal/export/exportmusicxml.cppsrc/importexport/musicxml/internal/import/importmusicxmlpass2.cpp
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 `@muse`:
- Line 1: The PR includes a muse submodule bump whose commit "Cloud: remove QML
dependency on MuseScore.Project" is unrelated to the MusicXML chord-line
import/export work; either drop the muse submodule update from this branch
(revert the submodule pointer change) or update the PR description and changelog
to explicitly include the cloud QML refactor; locate the submodule pointer
change named "muse" in your branch/commit history and either reset it to the
previous commit or add a clear note that the cloud QML changes are intentionally
part of this PR and explain why they are required alongside the MusicXML chord
work.
🪄 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: 37047da9-3a93-48df-a6b1-36450f19237f
📒 Files selected for processing (2)
musesrc/importexport/musicxml/internal/import/importmusicxmlpass2.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp
| @@ -1 +1 @@ | |||
| Subproject commit 8d6707fb9cbc9b5b52f7b7649d0dd4976dd4e0ac | |||
| Subproject commit 37eb6e3396f6ec795c14d5c28a3e52f624d048f3 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
old_sha="8d6707fb9cbc9b5b52f7b7649d0dd4976dd4e0ac"
new_sha="37eb6e3396f6ec795c14d5c28a3e52f624d048f3"
submodule_name=$(
git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
awk '$2=="muse"{print $1}' |
sed 's/^submodule\.//; s/\.path$//'
)
if [ -z "${submodule_name:-}" ]; then
echo "Could not find submodule metadata for path 'muse'." >&2
exit 1
fi
url=$(git config -f .gitmodules --get "submodule.${submodule_name}.url")
python - "$url" "$old_sha" "$new_sha" <<'PY'
import json
import re
import subprocess
import sys
url, old_sha, new_sha = sys.argv[1:4]
m = re.search(r'github\.com[:/](.+?)(?:\.git)?$', url)
if not m:
raise SystemExit(f"Unsupported submodule URL: {url}")
slug = m.group(1)
payload = json.loads(subprocess.check_output(
["gh", "api", f"repos/{slug}/compare/{old_sha}...{new_sha}"],
text=True,
))
print(f"Submodule repo: {slug}")
print(f"Compare status: {payload.get('status')}")
print(f"Total commits: {payload.get('total_commits')}")
print("\nTouched files:")
for f in payload.get("files", []):
print(f" - {f['filename']} ({f['status']}, +{f['additions']}/-{f['deletions']})")
print("\nCommit subjects:")
for c in payload.get("commits", []):
subject = c["commit"]["message"].splitlines()[0]
print(f" - {c['sha'][:12]} {subject}")
PYRepository: musescore/MuseScore
Length of output: 706
Submodule update does not match PR objective.
The muse submodule bump introduces only cloud-related changes ("Cloud: remove QML dependency on MuseScore.Project") that remove QML files from cloud UI components—completely unrelated to the stated MusicXML chord-line import/export work. The submodule contains no MusicXML or chord-related modifications.
Either remove this unrelated submodule update, or clarify whether the cloud QML refactoring should be included in this PR alongside the MusicXML work.
🤖 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 `@muse` at line 1, The PR includes a muse submodule bump whose commit "Cloud:
remove QML dependency on MuseScore.Project" is unrelated to the MusicXML
chord-line import/export work; either drop the muse submodule update from this
branch (revert the submodule pointer change) or update the PR description and
changelog to explicitly include the cloud QML refactor; locate the submodule
pointer change named "muse" in your branch/commit history and either reset it to
the previous commit or add a clear note that the cloud QML changes are
intentionally part of this PR and explain why they are required alongside the
MusicXML chord work.
There was a problem hiding this comment.
The "Check: Submodules" workflow failed and wanted the submodule to be updated to this commit.
There was a problem hiding this comment.
@rettinghaus, thanks for the clarification! That makes sense — if the "Check: Submodules" CI workflow requires the submodule to point to this commit, then the update is mandatory and my concern doesn't apply here. Sorry for the noise!
(◕ᴥ◕)
not (yet) for wavy chordlines though (as they are not yet supported in Mu3) Backport of musescore#33346 Co-Authored-By: Klaus Rettinghaus <rettinghaus@users.noreply.github.com>
|
@rettinghaus please rebase |
5 participants
No description provided.