Skip to content

Fix chord symbols alignment when multiple staves have chord symbols#32811

Open
cbjeukendrup wants to merge 2 commits intomusescore:masterfrom
cbjeukendrup:chord_symbols_alignment
Open

Fix chord symbols alignment when multiple staves have chord symbols#32811
cbjeukendrup wants to merge 2 commits intomusescore:masterfrom
cbjeukendrup:chord_symbols_alignment

Conversation

@cbjeukendrup
Copy link
Copy Markdown
Collaborator

@cbjeukendrup cbjeukendrup commented Mar 27, 2026

Resolves: #31927

Follow-up to #29745

Summary by CodeRabbit

  • Bug Fixes
    • Improved harmony and fret diagram positioning in sheet music by enhancing duplicate detection to consider both note timing and staff position, resulting in better vertical alignment of these musical elements.

@RomanPudashkin RomanPudashkin requested a review from miiizen March 30, 2026 07:00
@miiizen miiizen force-pushed the chord_symbols_alignment branch from cd34692 to 5baef5b Compare March 30, 2026 13:03
Copy link
Copy Markdown
Contributor

@miiizen miiizen left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me, I've just added a vtest.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

Updated duplicate-detection logic in harmony and fret diagram layout methods to track both tick position and staff index as a pair, replacing individual tick-only tracking. This enables correct alignment of chord symbols across multiple staves within a system.

Changes

Cohort / File(s) Summary
Harmony and Fret Diagram Layout
src/engraving/rendering/score/systemlayout.cpp
Replaced std::map<Fraction, staff_idx_t> with std::set<std::pair<Fraction, staff_idx_t>> in layoutHarmonies() and layoutFretDiagrams() to track harmony positions by both tick and staff index. Updated membership checks to use (tick, staffIdx) pairs. Adjusted autoplace call to use the correct harmony object reference.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main change: fixing chord symbol alignment across multiple staves in a system.
Description check ✅ Passed The description includes an issue reference (#31927) and context (follow-up to #29745), but lacks detail about the implementation and test coverage.
Linked Issues check ✅ Passed The code changes directly address the reported issue (#31927) by fixing how chord symbols are tracked and positioned across multiple staves.
Out of Scope Changes check ✅ Passed All changes are focused on the systemlayout.cpp file and directly relate to the harmony and fret diagram positioning logic needed to fix chord symbol alignment.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/engraving/rendering/score/systemlayout.cpp (1)

1123-1127: ⚠️ Potential issue | 🟠 Major

Fix the incomplete erase-remove idiom in two locations.

Lines 1126 and 1144 use an incomplete form of the erase-remove idiom. The std::remove() call moves matching elements to the end and returns an iterator, but erase() must be called with both the iterator and the container's end to remove the entire moved range. Currently, only one element is erased; if the same Harmony* appears twice, one copy survives and can still participate in alignment logic. This creates silent correctness issues in harmony placement.

Suggested fix
         if (muse::contains(fretHarmonyPositions, { fd->tick(), fd->staffIdx() })) {
             fretOrHarmonyItemsNoAlign.push_back(fd);
             if (fd->harmony()) {
-                harmonyItemsAlign.erase(std::remove(harmonyItemsAlign.begin(), harmonyItemsAlign.end(), fd->harmony()));
+                harmonyItemsAlign.erase(
+                    std::remove(harmonyItemsAlign.begin(), harmonyItemsAlign.end(), fd->harmony()),
+                    harmonyItemsAlign.end());
                 fretOrHarmonyItemsNoAlign.push_back(fd->harmony());
             }
             continue;
@@
         if (muse::contains(fretHarmonyPositions, { h->tick(), h->staffIdx() })) {
-            harmonyItemsAlign.erase(std::remove(harmonyItemsAlign.begin(), harmonyItemsAlign.end(), h));
+            harmonyItemsAlign.erase(
+                std::remove(harmonyItemsAlign.begin(), harmonyItemsAlign.end(), h),
+                harmonyItemsAlign.end());
             fretOrHarmonyItemsNoAlign.push_back(h);
             continue;
         }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 43c984a2-3018-4f38-92d0-dcc5d44ca0bf

📥 Commits

Reviewing files that changed from the base of the PR and between c3842f5 and 5baef5b.

📒 Files selected for processing (2)
  • src/engraving/rendering/score/systemlayout.cpp
  • vtest/scores/harmony-align-2.mscz

@miiizen miiizen added the vtests This PR produces approved changes to vtest results label Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

vtests This PR produces approved changes to vtest results

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chord symbols are only aligned on one stave per system

3 participants