Use named noteheads without accidentals for double/triple sharps/flats

[Jojo-Schmitz]

if using one of the two new "no accidental" settings

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Extended Note::noteHead fallback handling for pitch-name-related schemes to cover double and triple sharp or flat TPC values. These values now resolve to corresponding notehead groups without accidentals, with German-specific mappings for B variants.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description is far too vague and misses the required issue reference, motivation, and checklist details from the template.	Add a linked issue number, a short change summary, and complete the required checklist items from the template.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code addresses #34229 by remapping unsupported double/triple sharp/flat TPCs to the default no-accidental notehead groups, including German B/H handling.
Out of Scope Changes check	✅ Passed	The changes stay within the notehead fallback logic and do not introduce unrelated code paths or features.
Title check	✅ Passed	The title clearly matches the change: named noteheads without accidentals for double/triple sharps and flats.

✨ 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/engraving/dom/note.cpp (1)

465-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix typographical error in the comment.

There is a minor typo in the comment on line 467. "rather that" should be "rather than".

📝 Proposed fix

         // for TPCs that don't have their own heads (i.e. with 2 or 3 accidentals),
         // fall back to using the corresponding ones without accidentals
-        // rather that using the (smaller!) normal noteheads
+        // rather than using the (smaller!) normal noteheads
         } else if (tpc == Tpc::TPC_A_SS || tpc == Tpc::TPC_A_SSS) {

🤖 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/engraving/dom/note.cpp` around lines 465 - 499, Correct the typo in the
fallback notehead comment near the TPC handling branches, changing “rather that”
to “rather than” without modifying the surrounding logic.

🤖 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/engraving/dom/note.cpp`:
- Around line 465-499: Correct the typo in the fallback notehead comment near
the TPC handling branches, changing “rather that” to “rather than” without
modifying the surrounding logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3bdf151b-4cff-42dd-8201-8f4d6ff21b83

📥 Commits

Reviewing files that changed from the base of the PR and between 4d3e04e and 98a74c1.

📒 Files selected for processing (1)

• src/engraving/dom/note.cpp

[Jojo-Schmitz]

🧹 Nitpick comments (1)

src/engraving/dom/note.cpp (1)> 465-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix typographical error in the comment.
There is a minor typo in the comment on line 467. "rather that" should be "rather than".

@coderabbitai: Of course, fixed

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@Jojo-Schmitz Thanks for fixing it!

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[Jojo-Schmitz]

@ajuncosa Maybe you can cherry-pick this into your #34230, so we have it in 4.7 too?

Edit: too late, merged already... so I'll backport it myself

[Jojo-Schmitz]

In (the botched and meahwhile closed #34244) @coderabbitai suggested to use switch ... case ... rather than if ... else if ..., see #34244 (review)
Opinions?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== PR `#34244` review comments mentioning switch/case ==='
gh api --paginate \
  repos/musescore/MuseScore/pulls/34244/comments \
  --jq '.[] | select((.body // "") | test("switch|case"; "i")) |
    {path, line, original_line, user: .user.login, body, html_url}'

echo
echo '=== Outline: current note.cpp ==='
ast-grep outline src/engraving/dom/note.cpp --items all --type function --match 'noteHead'

echo
echo '=== Current Note::noteHead pitch-name section ==='
rg -n -A110 -B12 'NoteHeadGroup Note::noteHead\(' src/engraving/dom/note.cpp

Length of output: 1288

---

🏁 Script executed:

#!/bin/bash
set -u

echo '=== PR `#34244` submitted reviews ==='
gh api --paginate repos/musescore/MuseScore/pulls/34244/reviews \
  --jq '.[] | {user: .user.login, state, body, html_url}' || true

echo
echo '=== PR `#34244` inline review comments ==='
gh api --paginate repos/musescore/MuseScore/pulls/34244/comments \
  --jq '.[] | {path, line, original_line, user: .user.login, body, html_url}' || true

echo
echo '=== PR `#34244` conversation comments ==='
gh api --paginate repos/musescore/MuseScore/issues/34244/comments \
  --jq '.[] | {user: .user.login, body, html_url}' || true

echo
echo '=== Note::noteHead implementation, Lines 336–540 ==='
sed -n '336,540p' src/engraving/dom/note.cpp

Length of output: 22173

---

@Jojo-Schmitz I’d treat that as an optional, separate refactor—not something this fix needs.

A switch (tpc) is arguably a better fit because the outer decision is exclusively a mapping from one discriminant value to a notehead group; grouped cases would make the double/triple-accidental fallbacks compact. But converting the entire existing chain would be a broad, mechanical diff with regression risk around the German and no_accidentals special cases, while adding no functional value to #34242.

I also withdraw the “slightly improve execution performance” rationale: that is not established here. A compiler may produce equivalent code, and it would need measurement to justify a performance claim.

So my recommendation is:

• keep the current focused if / else if change for this bug fix and its 4.7 backport;
• optionally do a standalone readability refactor to switch (tpc) later, with dedicated coverage for the German B/H behavior.

🔍🐇

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="musescore/MuseScore"

printf '%s\n' '=== Referenced PR `#34244` review comments ==='
gh api "repos/${repo}/pulls/34244/comments?per_page=100" --paginate \
  --jq '.[] | select(.user.login == "coderabbitai" or (.body | test("switch|case|else if"; "i"))) | {path, line, original_line, body, html_url}'

printf '%s\n' '=== Current note.cpp outline ==='
ast-grep outline src/engraving/dom/note.cpp --items all --type function --match 'noteHead'

printf '%s\n' '=== Current pitch-name scheme logic ==='
rg -n -C 12 'NoteHeadGroup Note::noteHead|PITCHNAME|PITCH_NAME|case Tpc::|if \(.*tpc|else if' src/engraving/dom/note.cpp

Length of output: 37324

---

@Jojo-Schmitz I’d keep the current if ... else if ... form for this PR.

A switch (tpc) is certainly valid for a discrete enum-like value, but here it would not be clearly better: most branches group two TPC values, and the B cases still need the german conditional. The existing code at Lines 468–503 is consistent with the larger pitch-name mapping immediately above it and keeps the fallback mapping explicit.

So: no strong maintainability benefit from changing it now. I would only choose switch if this entire TPC-to-notehead mapping were being deliberately refactored as a separate cleanup.

Also, the accessible comment on #34244 was the German B-double/triple-flat correctness finding, rather than a switch-style suggestion; that correctness fix is now present here.

[Jojo-Schmitz]

@coderabbitai: that if (german) (and if no_accidental) stuff could gert much shorter using the ? operator, e.g. group = german ? NoteHead::Group::HEAD_H: NoteHead::Group::HEAD_B;

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@Jojo-Schmitz You’re right—when both branches only assign group, the conditional operator is clearer and more compact:

group = german ? NoteHead::Group::HEAD_H : NoteHead::Group::HEAD_B;

The same applies to the no_accidentals selection. That improves the local logic without requiring a broader switch refactor. 🐇

[Jojo-Schmitz]

Human reviewers' opnions on that? @ajuncosa ?

[davidstephengrant]

Thanks @Jojo-Schmitz, this is nice for the two no accidentals schemes, but for regular Pitch names and German pitch names (with accidentals) we should still fall back to default noteheads.

[Jojo-Schmitz]

Why? IMHO a named notehead without an accidental is better than none (and would fix the isssue as a side effect)

[davidstephengrant]

Because in schemes with accidentals this will show the wrong pitch rather than failing more obviously for an unsupported case.

A few concerns:

1. It is more likely to slip through in editing because the error is less easy to spot while skimming through.
2. Someone may be rehearsing by reading only from the pitch names, in which case they may (repeatedly) play an incorrect pitch, rather than pausing to investigate.
3. E.g. C#-C inside the noteheads in a rising chromatic line makes no sense.

[Jojo-Schmitz]

OK, then a) use them only for the two no accidentals schemes and b) don't (yet) fix #34229

[ajuncosa]

@Jojo-Schmitz Thanks for this! My human opinion:

1. I agree about the ternary operators, and
2. I also agree that these many chained conditions could be re-arranged - possibly under a switch statement, or at least we could extract the entire no_accidentals case -, but I agree with the rabbit that for the current PR I'd leave it as it is, to make sure we don't introduce a new bug by accidentally missing a little condition.

[davidstephengrant]

Tested and approved on Ubuntu 24.04.4 LTS.