String tuning follows selected chord spelling#33533
Conversation
Use chord symbol spelling for string tunings text, accessible labels, and popup note values so note names follow the selected spelling system (e.g. "H" instead of "B" for German spelling). Added engraving and notation scene tests covering localized string tunings text, popup note formatting, and popup input parsing across supported spelling systems. Closes musescore#32018 Co-authored-by: Rodrigo Bandeira <rodrigobandeira@tecnico.ulisboa.pt>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR centralizes string-tuning pitch formatting and parsing into new mu::notation utilities, adds engraving::stringTuningPitchName, propagates NoteSpellingType through StringTuningsSettingsModel and StringTuningsItem, updates the QML popup to use text input plus adjust controls feeding setStringValue/setStringPitchValue, and adds unit tests and CMake updates verifying localized formatting and parsing. 🚥 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml (1)
178-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccessibility regression: accessible name now exposes the raw MIDI pitch instead of the spelled note.
Value %1is interpolated withvalueControl.currentValue(an integer pitch). Before this refactor the equivalent control surfaced the localized note string. Screen readers will now announce e.g. "Value 64" rather than "Value E4" / "Value Mi4". Bind this to the spelled text so screen readers reflect the same content as the visible field and stay consistent with the chord-symbol spelling that this PR is otherwise threading through.♻️ Proposed fix
- navigation.accessible.name: visibleBox.navigation.accessible.name + " " + qsTrc("notation", "Value %1").arg(valueControl.currentValue) + navigation.accessible.name: visibleBox.navigation.accessible.name + " " + qsTrc("notation", "Value %1").arg(valueControl.currentText)🤖 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/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml` at line 178, The accessible name currently interpolates the raw integer pitch via valueControl.currentValue; change that to use the spelled/localized note string from the control (e.g., valueControl.spelledText or valueControl.spelledNote) so screen readers announce the note name (like "E4") not the MIDI number; update the expression assigned to navigation.accessible.name (which currently builds on visibleBox.navigation.accessible.name + " " + qsTrc("notation", "Value %1").arg(...)) to pass the spelled-note property instead of valueControl.currentValue.
🧹 Nitpick comments (1)
src/engraving/dom/stringtunings.cpp (1)
40-64: ⚖️ Poor tradeoffConsider extracting a shared pitch-name helper to avoid drift with
stringTuningPitchToString.
tuningPitchNamehere andmu::notation::stringTuningPitchToStringinstringtuningsutils.cppimplement effectively the same algorithm (pitch validation,pitch2tpcwith flat/sharp preference,tpc2name, theGERMAN+TPC_B_Baccidental-clear special case, and Unicode conversion). Any future fix (e.g., additional spelling rules, more accidental special cases) has to be applied in two places or the engraving text and the popup formatting will silently disagree.A small engraving-level helper such as
engraving::stringTuningPitchName(int pitch, bool useFlat, NoteSpellingType spelling, bool withOctave)could be the single implementation; the notationscene utility would just forward to it (wrapping the result inQString).🤖 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/stringtunings.cpp` around lines 40 - 64, Extract the duplicate pitch-name logic into a single helper (suggested signature: engraving::stringTuningPitchName(int pitch, bool useFlat, NoteSpellingType spelling, bool withOctave)) that performs pitchIsValid, pitch2tpc using useFlat/Prefer::FLATS vs Prefer::SHARPS, tpc2name, the NoteSpellingType::GERMAN + TPC_B_B accidental.clear() special-case, convertPitchStringFlatsAndSharpsToUnicode, and the octave math using PITCH_DELTA_OCTAVE; then replace tuningPitchName in stringtunings.cpp and mu::notation::stringTuningPitchToString in stringtuningsutils.cpp to call this new helper (wrapping/convert to QString in the notation utility as needed) so all logic lives in one place.
🤖 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/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml`:
- Line 221: The suppressNextTextCommit flag (property suppressNextTextCommit) is
being set unconditionally in onIncreaseButtonClicked/onDecreaseButtonClicked and
can persist if valueInput does not have focus, causing the next user text commit
to be dropped in onTextEditingFinished; fix by gating or resetting the flag:
either only set suppressNextTextCommit when valueInput.focus is true (i.e. check
valueInput.focus before arming the flag in
onIncreaseButtonClicked/onDecreaseButtonClicked) or add a focus handler on
valueInput (onFocusChanged / onActiveFocusChanged) that clears
suppressNextTextCommit when the field gains focus so it cannot leak between
interactions, and ensure currentText/setStringValue behavior in
onTextEditingFinished remains unchanged except for the guarded flag.
---
Outside diff comments:
In
`@src/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml`:
- Line 178: The accessible name currently interpolates the raw integer pitch via
valueControl.currentValue; change that to use the spelled/localized note string
from the control (e.g., valueControl.spelledText or valueControl.spelledNote) so
screen readers announce the note name (like "E4") not the MIDI number; update
the expression assigned to navigation.accessible.name (which currently builds on
visibleBox.navigation.accessible.name + " " + qsTrc("notation", "Value
%1").arg(...)) to pass the spelled-note property instead of
valueControl.currentValue.
---
Nitpick comments:
In `@src/engraving/dom/stringtunings.cpp`:
- Around line 40-64: Extract the duplicate pitch-name logic into a single helper
(suggested signature: engraving::stringTuningPitchName(int pitch, bool useFlat,
NoteSpellingType spelling, bool withOctave)) that performs pitchIsValid,
pitch2tpc using useFlat/Prefer::FLATS vs Prefer::SHARPS, tpc2name, the
NoteSpellingType::GERMAN + TPC_B_B accidental.clear() special-case,
convertPitchStringFlatsAndSharpsToUnicode, and the octave math using
PITCH_DELTA_OCTAVE; then replace tuningPitchName in stringtunings.cpp and
mu::notation::stringTuningPitchToString in stringtuningsutils.cpp to call this
new helper (wrapping/convert to QString in the notation utility as needed) so
all logic lives in one place.
🪄 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: 930684f7-468a-4edb-99ca-ea2ec2ebb5ec
📒 Files selected for processing (11)
src/engraving/dom/stringtunings.cppsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/stringtunings_tests.cppsrc/notationscene/qml/MuseScore/NotationScene/CMakeLists.txtsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qmlsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/stringtuningssettingsmodel.cppsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/stringtuningssettingsmodel.hsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/stringtuningsutils.cppsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/stringtuningsutils.hsrc/notationscene/qml/MuseScore/NotationScene/tests/CMakeLists.txtsrc/notationscene/qml/MuseScore/NotationScene/tests/stringtuningsutils_tests.cpp
Use spelled note names in the string tunings popup accessibility text so screen readers announce note values instead of raw MIDI numbers. Move string tuning pitch-name formatting into a shared engraving helper and reuse it from the popup utility code to keep spelling behavior in one place. Only suppress the next text commit after arrow-button stepping when the text field had focus, preventing the flag from leaking into later manual edits. Closes musescore#32018 Co-authored-by: Rodrigo Bandeira <rodrigobandeira@tecnico.ulisboa.pt>
|
This latest commit addresses the issues raised by @coderabbitai, that should now be fixed. |
🧩 Analysis chain🏁 Script executed: Length of output: 36855
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/engraving/dom/stringtunings.cpp (1)
50-50: ⚡ Quick winNoteCaseType::CAPITAL preserves SOLFEGGIO/FRENCH base casing (it won’t force uppercase).
tpc2namebuilds SOLFEGGIO/FRENCH names as{"Fa","Do","Sol","Re","La","Mi","Si"}(with FrenchRe→Ré) and then applies casing changes only forLOWER/UPPER;NoteCaseType::CAPITAL(andAUTO) leaves the constructed casing unchanged. So thisstringtunings.cppcall won’t break Solfeggio/French casing by uppercasing—useNoteCaseType::LOWERonly if the PR specifically requires lowercase"do"/"ré".🤖 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/stringtunings.cpp` at line 50, The call to tpc2name(tpc, spelling, NoteCaseType::CAPITAL, step, accidental) is leaving Solfeggio/French base casing intact (CAPITAL does not force uppercase), so if the PR intended to produce lowercase names (e.g., "do"/"ré") change the NoteCaseType to NoteCaseType::LOWER in this call; otherwise keep CAPITAL as-is to preserve the original Solfeggio/French capitalization. Ensure you update the call site referencing tpc2name and the NoteCaseType enum value accordingly.src/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml (1)
219-222: 💤 Low valueRedundant
spelledNoteproperty duplicatescurrentText.Both
currentTextandspelledNoteare bound to the same expressiondelegateItem.modelData["valueStr"]. The accessibility label at line 178 could referencevalueControl.currentTextdirectly, andspelledNotecould be removed to reduce surface area. If the intent is to later differentiate (e.g., spelled-without-octave for screen readers vs. visible text with octave), consider documenting that intent; otherwise this is dead duplication.🤖 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/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml` around lines 219 - 222, spelledNote duplicates currentText by binding both to delegateItem.modelData["valueStr"]; remove the redundant property spelledNote and update any references (e.g. the accessibility label that currently could use spelledNote) to use valueControl.currentText or currentText directly; if you intended different semantics later, add a clear comment above currentText explaining the planned differentiation before reintroducing a separate spelledNote property.
🤖 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/stringtunings.cpp`:
- Line 50: The call to tpc2name(tpc, spelling, NoteCaseType::CAPITAL, step,
accidental) is leaving Solfeggio/French base casing intact (CAPITAL does not
force uppercase), so if the PR intended to produce lowercase names (e.g.,
"do"/"ré") change the NoteCaseType to NoteCaseType::LOWER in this call;
otherwise keep CAPITAL as-is to preserve the original Solfeggio/French
capitalization. Ensure you update the call site referencing tpc2name and the
NoteCaseType enum value accordingly.
In
`@src/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qml`:
- Around line 219-222: spelledNote duplicates currentText by binding both to
delegateItem.modelData["valueStr"]; remove the redundant property spelledNote
and update any references (e.g. the accessibility label that currently could use
spelledNote) to use valueControl.currentText or currentText directly; if you
intended different semantics later, add a clear comment above currentText
explaining the planned differentiation before reintroducing a separate
spelledNote property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 028f4062-4019-42e9-8308-cbbc3ca58c74
📒 Files selected for processing (4)
src/engraving/dom/stringtunings.cppsrc/engraving/dom/stringtunings.hsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/StringTuningsPopup.qmlsrc/notationscene/qml/MuseScore/NotationScene/elementpopups/stringtuningsutils.cpp
|
Would this also fix #20673 ? |
|
@Jojo-Schmitz No, the primary focus of this implementation was to display note tunings in the same language as the selected chord spelling for consistency, as mentioned in the corresponding feature (#32018). |
5 participants
Resolves: #32018
Summary
This pull request makes string tunings follow the note spelling system
selected for chord symbols.
Previously, string tunings could display note names differently from the
rest of the score. With this change, the rendered string tuning text,
accessible labels, and popup note values now all use the active
chord-symbol spelling.
Behavior
String tunings now display localized note names consistently across the
score and the editing popup. This includes spelling systems such as
German, Solfeggio, and French, while preserving the expected casing and
special note-name behavior for each system.
The popup was also updated so that note values are both displayed and
parsed according to the selected spelling. Localized note input is
accepted, and the step controls continue to work with the active note
representation.
Implementation
To support this, the popup note parsing and formatting logic was
refactored into reusable helper functions. On the engraving side,
string tuning text generation and accessibility output now derive note
names from the current chord-symbol spelling style.
Testing
The patch adds engraving and notation scene tests covering: