Fix finding external Musical Text Fonts and their metadata.json files#33757
Fix finding external Musical Text Fonts and their metadata.json files#33757Jojo-Schmitz wants to merge 2 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR changes EngravingFontsController::scanDirectory() to alter metadata discovery and font resolution. Metadata is now looked up first as .json; private directories get additional fallbacks (lowercased/no-spaces _metadata.json, metadata.json, then any *.json), while non-private dirs are skipped if .json is missing. For fonts, symbolFontPath must be found via findFontPath(fontName) or the entry is skipped; textFontPath is tried as fontName + " Text", then with spaces removed + "Text", and finally falls back to symbolFontPath if unresolved. 🚥 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)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. 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 |
Jojo-Schmitz
changed the title
|
@cbjeukendrup mind to review? Seems to have been your code. |
cbjeukendrup
left a comment
cbjeukendrup
left a comment
There was a problem hiding this comment.
Seems to make sense to me. Probably indeed a copy paste mistake...
Jojo-Schmitz
force-pushed
the
external-smulf-fonts
branch
from
b700b09 to
0c84ff8
Compare
Jojo-Schmitz
force-pushed
the
external-smulf-fonts
branch
from
0c84ff8 to
05bca93
Compare
|
There another quirk, MuseScore look for the metadata.json files in an (inner) iterator, picking the last one it finds matching "*.json".
It should stop at the 1st find. Example (taken all files from https://github.com/mikkopatama/eugenefont): |
|
I haven't seen enough fonts to be able to judge whether those three variants are enough. If we want to be really thorough, we should perhaps consider all json files in the folder and use |
|
I've so far only come accross those 3 variants, in 18 external and all the builtin fonts |
|
In that case let's go with your suggestion |
if those don't have a space before the "Text" in their filename
|
Jojo-Schmitz
force-pushed
the
external-smulf-fonts
branch
2 times, most recently
from
05241c6 to
075ed44
Compare
![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/notation/internal/engravingfontscontroller.cpp`:
- Around line 117-124: The metadata fallback loop using QDirIterator populates
metadataPath by taking the last iterated JSON and then only checks
metadataPath.empty(), which can register an invalid/nonexistent metadata path
and is unstable when multiple JSONs exist; change the loop in the code that
builds metadataPath (the QDirIterator block) to deterministically select a
single metadata file (e.g., prefer a known filename like "metadata.json" or
immediately take the first matching file) and after selection verify the file
actually exists with QFileInfo::exists() before calling addExternalFont();
ensure addExternalFont() receives a validated, existing metadataPath so
EngravingFont::ensureLoad() won’t fail on missing/incorrect metadata.
🪄 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: 60b3d1d5-95ff-4091-b3b8-8180c3802bcb
📒 Files selected for processing (1)
src/notation/internal/engravingfontscontroller.cpp
Jojo-Schmitz
force-pushed
the
external-smulf-fonts
branch
from
075ed44 to
c7e5fa4
Compare
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/notation/internal/engravingfontscontroller.cpp`:
- Line 123: The single-line "continue;" in
src/notation/internal/engravingfontscontroller.cpp is mis-indented and failing
uncrustify; locate the continue statement inside the affected loop in the
EngravingFontsController implementation (the method containing the loop that
handles font resolution) and adjust its indentation to match the surrounding
block scope (align it with other statements inside that conditional/loop), then
re-run the codestyle check to confirm it passes.
🪄 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: 2c675925-47b0-4a2f-a676-30de7f00037b
📒 Files selected for processing (1)
src/notation/internal/engravingfontscontroller.cpp
Jojo-Schmitz
force-pushed
the
external-smulf-fonts
branch
from
c7e5fa4 to
2f9b456
Compare
|
I don't understand the unit test failure, I don't think my changes are the culprit. @coderabbitai? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the check runs/status for PR `#33757`
gh pr checks 33757 --repo musescore/MuseScore 2>&1 | head -80Length of output: 1596 🏁 Script executed: #!/bin/bash
# Get recent PR comments and reviews to find any CI failure details
gh pr view 33757 --repo musescore/MuseScore --json statusCheckRollup 2>&1 | head -100Length of output: 4242 🏁 Script executed: #!/bin/bash
# Get the failing job log from the utest run
gh api /repos/musescore/MuseScore/actions/jobs/80854436260/logs 2>&1 | head -300Length of output: 22806 🏁 Script executed: #!/bin/bash
# Also check if there are annotations/failure details on the job
gh api /repos/musescore/MuseScore/actions/runs/27362803918/jobs 2>&1 | python3 -c "
import sys, json
data = json.load(sys.stdin)
for job in data.get('jobs', []):
if job.get('conclusion') == 'failure':
print('Job:', job['name'])
print('Steps:')
for step in job.get('steps', []):
if step.get('conclusion') == 'failure':
print(' FAILED step:', step['name'])
print('Annotations:')
" 2>&1
gh api /repos/musescore/MuseScore/actions/runs/27362803918/jobs --jq '.jobs[] | select(.conclusion=="failure") | {name: .name, steps: [.steps[] | select(.conclusion=="failure") | .name]}' 2>&1Length of output: 867 🏁 Script executed: #!/bin/bash
# Get just the failure-relevant portion of the log
gh api /repos/musescore/MuseScore/actions/jobs/80854436260/logs 2>&1 | grep -A 30 -i "FAILED\|error\|assert\|fail\|PASS\|RUN\|test" | tail -200Length of output: 21541 🏁 Script executed: #!/bin/bash
# Check annotations on the run for test failure details
gh api /repos/musescore/MuseScore/check-runs/80854436260/annotations 2>&1 | head -100Length of output: 504 🏁 Script executed: #!/bin/bash
# Get the log and search specifically around muse_global_tests
gh api /repos/musescore/MuseScore/actions/jobs/80854436260/logs 2>&1 | grep -A 80 "muse_global_tests" | head -120Length of output: 10687 🏁 Script executed: #!/bin/bash
# Check if this test also fails on main branch (recent runs)
gh api /repos/musescore/MuseScore/actions/workflows --jq '.workflows[] | select(.name | test("Unit Tests|utest"; "i")) | {id: .id, name: .name}' 2>&1Length of output: 319 🏁 Script executed: #!/bin/bash
# Look for the actual muse_global_tests runtime output (CTest prefixes lines with "1: ")
gh api /repos/musescore/MuseScore/actions/jobs/80854436260/logs 2>&1 | grep -E "^\s*(2026-[^ ]+ 1: |.*1: .*FAILED|.*1: .*PASSED|.*1: \[)" | head -100Length of output: 9900 🏁 Script executed: #!/bin/bash
# Also check recent runs of the utest workflow on main to see if this is a pre-existing failure
gh api "/repos/musescore/MuseScore/actions/workflows/111488818/runs?branch=main&per_page=5" \
--jq '.workflow_runs[] | {id: .id, conclusion: .conclusion, created_at: .created_at}' 2>&1Length of output: 729
More importantly, checking the scheduled
So this is a pre-existing, persistent failure on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the check runs / status for PR `#33757`
gh pr checks 33757 --repo musescore/MuseScore 2>&1 | head -80Length of output: 1598 🏁 Script executed: #!/bin/bash
# Get recent workflow run failures linked to this PR
gh run list --repo musescore/MuseScore --branch external-smulf-fonts --limit 10 2>&1Length of output: 1686 🏁 Script executed: #!/bin/bash
# Get jobs and steps for the failing unit test run
gh run view 27362803918 --repo musescore/MuseScore 2>&1Length of output: 933 🏁 Script executed: #!/bin/bash
# Get the failed job log output (truncated)
gh run view 27362803918 --repo musescore/MuseScore --log-failed 2>&1 | head -200Length of output: 19707 🏁 Script executed: #!/bin/bash
# Get more of the failed log, focusing on the test failure section
gh run view 27362803918 --repo musescore/MuseScore --log-failed 2>&1 | grep -A 5 -i "FAILED\|error\|assert\|test.*fail\|fail.*test\|CRASH\|segfault\|exit code\|Run tests" | head -150Length of output: 19787 🏁 Script executed: #!/bin/bash
# Also check if the same test failure exists on main branch recently
gh run list --repo musescore/MuseScore --branch main --workflow "Check: Unit Tests (utest)" --limit 5 2>&1Length of output: 860 🏁 Script executed: #!/bin/bash
# Check the most recent main branch failure - get the specific test failures
gh run view 27323913973 --repo musescore/MuseScore --log-failed 2>&1 | grep -i "FAILED\|PASS\|ERROR\|\*\*\*\|assertion\| fail\b" | grep -v "warning\|unused param\|Wunused" | head -80Length of output: 13633 🏁 Script executed: #!/bin/bash
# Same for the PR run
gh run view 27362803918 --repo musescore/MuseScore --log-failed 2>&1 | grep -i "FAILED\|PASS\|ERROR\|\*\*\*\|assertion\| fail\b" | grep -v "warning\|unused param\|Wunused" | head -80Length of output: 13822 🏁 Script executed: #!/bin/bash
# Get last portion of the failed log to see actual test failures
gh run view 27362803918 --repo musescore/MuseScore --log-failed 2>&1 | tail -150Length of output: 18772 🏁 Script executed: #!/bin/bash
# Also get the last part of the main branch failure for comparison
gh run view 27323913973 --repo musescore/MuseScore --log-failed 2>&1 | tail -60Length of output: 8984
The failing test: It's a pre-existing failure on
The 🐇 ₍ᐢ. .ᐢ₎ |
|
@coderabbitai: why then are other recent PRs if mine are not faíling the same way? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List recent open/merged PRs targeting main
gh pr list --repo musescore/MuseScore --base main --limit 10 --state all \
--json number,title,headRefName,createdAt,state,statusCheckRollup 2>&1 | \
python3 -c "
import sys, json
prs = json.load(sys.stdin)
for pr in prs:
checks = pr.get('statusCheckRollup') or []
utest = [c for c in checks if 'utest' in c.get('name','').lower() or 'unit' in c.get('name','').lower()]
status = [(c.get('name'), c.get('conclusion') or c.get('state')) for c in utest]
print(f\"PR #{pr['number']} ({pr['state']}): {pr['title'][:60]}\")
print(f\" utest checks: {status}\")
"Length of output: 1441 🏁 Script executed: #!/bin/bash
# Check what triggered the utest run on this PR specifically
gh run list --repo musescore/MuseScore --branch external-smulf-fonts --limit 20 2>&1
echo "---"
# Check what event triggered run 27362803918
gh run view 27362803918 --repo musescore/MuseScore --json event,triggerActor,headBranch,workflowName 2>&1
echo "---"
# Check the utest workflow triggers
gh api /repos/musescore/MuseScore/actions/workflows/111488818 --jq '{name:.name, path:.path}' 2>&1
echo "---"
# List all workflow runs for utest on PR branches (not main/scheduled)
gh api "/repos/musescore/MuseScore/actions/workflows/111488818/runs?per_page=10" \
--jq '.workflow_runs[] | {id:.id, event:.event, branch:.head_branch, conclusion:.conclusion, created_at:.created_at}' 2>&1Length of output: 5408
So the failing Your changes are still not the cause (they're completely unrelated to 🐇 ₍ᐢ. .ᐢ₎ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get recent PRs on the main branch to compare their CI status
gh pr list --repo musescore/MuseScore --base main --limit 10 --json number,title,headRefName,statusCheckRollup 2>&1 | python3 -c "
import sys, json
data = json.load(sys.stdin)
for pr in data:
print(f\"PR #{pr['number']}: {pr['title'][:60]}\")
checks = pr.get('statusCheckRollup', [])
for c in checks:
name = c.get('name', c.get('context', '?'))
status = c.get('conclusion') or c.get('state') or c.get('status', '?')
if 'test' in name.lower() or 'utest' in name.lower() or 'unit' in name.lower():
print(f\" CHECK: {name} -> {status}\")
print()
" 2>&1Length of output: 2272 🏁 Script executed: #!/bin/bash
# Check when PR `#33779` (Framework update) was merged, and its details
gh pr view 33779 --repo musescore/MuseScore --json number,title,mergedAt,mergeCommit,baseRefName 2>&1
echo "---"
# Check current status of PR `#33757` checks in more detail
gh pr checks 33757 --repo musescore/MuseScore 2>&1
echo "---"
# Check if there have been any newer CI runs on this PR branch
gh run list --repo musescore/MuseScore --branch external-smulf-fonts --limit 5 2>&1Length of output: 2787 🏁 Script executed: #!/bin/bash
# Get the run_tests workflow runs for PR `#33780` to compare with `#33757`
gh run list --repo musescore/MuseScore --branch fix-date-macros-format --limit 5 2>&1 || true
# Also check which branch PR `#33780` is on
gh pr view 33780 --repo musescore/MuseScore --json headRefName,statusCheckRollup 2>&1 | python3 -c "
import sys, json
data = json.load(sys.stdin)
print('Branch:', data['headRefName'])
for c in data.get('statusCheckRollup', []):
name = c.get('name', c.get('context', '?'))
status = c.get('conclusion') or c.get('state') or '?'
url = c.get('detailsUrl', c.get('targetUrl', ''))
print(f' {name} -> {status} {url}')
" 2>&1
echo "==="
# Now check the actual job IDs for the failing utest run on `#33757` vs a passing one
# Get created_at timestamps for the runs
gh api /repos/musescore/MuseScore/actions/runs/27362803918 --jq '{id: .id, name: .name, created_at: .created_at, head_sha: .head_sha}' 2>&1
echo "---"
# And check what muse_framework submodule commit the PR is at vs main
gh api /repos/musescore/MuseScore/contents/muse --ref external-smulf-fonts --jq '{sha: .sha}' 2>&1 || true
gh api /repos/musescore/MuseScore/contents/muse --ref main --jq '{sha: .sha}' 2>&1 || trueLength of output: 5710 🏁 Script executed: #!/bin/bash
# Check when `#33780`'s utest run was created
gh api /repos/musescore/MuseScore/actions/runs/27368123783 --jq '{id: .id, name: .name, created_at: .created_at, head_sha: .head_sha}' 2>&1
echo "---"
# Check recent scheduled utest runs on main to see if the failure was fixed
gh api "/repos/musescore/MuseScore/actions/workflows/111488818/runs?per_page=8" \
--jq '.workflow_runs[] | {id: .id, conclusion: .conclusion, created_at: .created_at, event: .event, head_branch: .head_branch}' 2>&1Length of output: 1755
There was no code change on This is also consistent with the scheduled 🐰 ¯_(ツ)_/¯ |
So far all fonts I've encountered have their metadata.json named "FontName.json" (mixed case, possible containing spaces, same as the fontname itself, the only documented option for non-private fonts), "fontname_metadata.json" (all lowercase, no spaces) or "metadata.json" (lowercase), so let's search for these, in that order and stop at the 1st hit. As a last reseort leave the previous iteration method (which stops at the last match for "*.json").
Jojo-Schmitz
force-pushed
the
external-smulf-fonts
branch
from
2f9b456 to
5909a28
Compare
|
OK, let's see whether another run succeeds Edit: it does indeed |
Jojo-Schmitz
changed the title
4 participants
Fix finding external Musical Text Fonts, if those don't have a space before the "Text" in their filename.
Pretty much looks like the original code had a copy/paste error here.
Log before the change (tested with private fonts):
So instead of BettenhovenText.otf it finds (and loads) Bettenhoven.otf.
Log after:
Avoids the need to rename those Text font files
Also make sure their metadata.json files' names follow common naming conventions rather that doing a wildcard search on "*.json" and picking the last match