Skip to content

Fix Instrument change text should show full instrument name#32821

Open
CubikingChill wants to merge 1 commit intomusescore:masterfrom
CubikingChill:my-new-branch
Open

Fix Instrument change text should show full instrument name#32821
CubikingChill wants to merge 1 commit intomusescore:masterfrom
CubikingChill:my-new-branch

Conversation

@CubikingChill
Copy link
Copy Markdown
Contributor

@CubikingChill CubikingChill commented Mar 27, 2026

Resolves: #32588

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

Summary by CodeRabbit

  • Bug Fixes
    • Fixed inconsistent instrument name display to ensure transposition information appears consistently across the application.

@CubikingChill CubikingChill force-pushed the my-new-branch branch 3 times, most recently from ea4b092 to c9d48dd Compare March 27, 2026 17:26

return instrumentName; // Example: "Flute"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also my #32771, which also removes this apparently unused method

@CubikingChill
Copy link
Copy Markdown
Contributor Author

CubikingChill commented Mar 28, 2026

Do you think in future we should only keep 1 format-name method in the whole codebase?

Currently my PR creates a non-QT method while some other parts of the UI use this

@RomanPudashkin RomanPudashkin requested a review from shoogle March 30, 2026 07:55
@CubikingChill
Copy link
Copy Markdown
Contributor Author

2026-03-30.20-29-05.-.Trim.mp4

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

This change refactors instrument name formatting by introducing a new Instrument::trackNameWithTrait() method that conditionally appends transposition trait information to the base track name, and updates instrument-change text generation to use this method instead of the plain track name. The previous formatting logic in notationparts.cpp was removed.

Changes

Cohort / File(s) Summary
Instrument class enhancement
src/engraving/dom/instrument.h, src/engraving/dom/instrument.cpp
Added new public method trackNameWithTrait() const that returns the track name with appended transposition trait when the trait type is Transposition and not hidden on score; includes translation.h dependency.
Instrument change text generation
src/engraving/dom/instrchange.cpp
Updated "To %1" placeholder to call trackNameWithTrait() instead of trackName() when generating instrument-change display text.
Notation parts cleanup
src/notation/internal/notationparts.cpp
Removed local helper function formatInstrumentTitleOnScore() that previously handled conditional trait-based title formatting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: fixing instrument-change text to display the full instrument name with transposition traits.
Description check ✅ Passed The description includes the linked issue (#32588), references the PR checklist with most items checked, and covers the scope of changes despite lacking detailed technical explanation.
Linked Issues check ✅ Passed The code changes directly address issue #32588 by introducing trackNameWithTrait() method and using it in instrument-change text generation, replacing the deleted formatInstrumentTitleOnScore() logic.
Out of Scope Changes check ✅ Passed All changes are directly related to the linked issue objective of showing full instrument names with transposition traits in instrument-change text.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0253b09c-5f16-4927-820a-bde311309f9f

📥 Commits

Reviewing files that changed from the base of the PR and between 41b7bde and 6305669.

📒 Files selected for processing (4)
  • src/engraving/dom/instrchange.cpp
  • src/engraving/dom/instrument.cpp
  • src/engraving/dom/instrument.h
  • src/notation/internal/notationparts.cpp
💤 Files with no reviewable changes (1)
  • src/notation/internal/notationparts.cpp

Comment on lines +1071 to +1077
String Instrument::trackNameWithTrait() const
{
if (m_trait.type == TraitType::Transposition && !m_trait.isHiddenOnScore) {
return muse::mtrc("engraving", "%1 in %2").arg(m_trackName, m_trait.name);
}
return m_trackName;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Use a single shared formatter for instrument display names across contexts.

This method introduces another formatting path for transposition naming. To prevent drift between score text, mixer, and Staff/Part properties, prefer one canonical formatter and route all UI/display callers through it.

Comment on lines +1073 to +1075
if (m_trait.type == TraitType::Transposition && !m_trait.isHiddenOnScore) {
return muse::mtrc("engraving", "%1 in %2").arg(m_trackName, m_trait.name);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard against empty transposition trait names in formatted output.

If m_trait.name is empty, Line 1074 can yield a user-visible string like "Piccolo in ". Add a validity check before formatting.

💡 Proposed fix
 String Instrument::trackNameWithTrait() const
 {
-    if (m_trait.type == TraitType::Transposition && !m_trait.isHiddenOnScore) {
+    if (m_trait.type == TraitType::Transposition && !m_trait.isHiddenOnScore && m_trait.isValid()) {
         return muse::mtrc("engraving", "%1 in %2").arg(m_trackName, m_trait.name);
     }
     return m_trackName;
 }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instrument change text should show full instrument name

2 participants