Skip to content

Fix #33166: Layout as harmony rather than text in editing#33247

Open
silasvorne wants to merge 2 commits intomusescore:masterfrom
silasvorne:33166-fix-harmony-edit-pos
Open

Fix #33166: Layout as harmony rather than text in editing#33247
silasvorne wants to merge 2 commits intomusescore:masterfrom
silasvorne:33166-fix-harmony-edit-pos

Conversation

@silasvorne
Copy link
Copy Markdown
Contributor

After #32770, it seems that laying out harmony as text is no longer appropriate; change it to be laid out as harmony.

Resolves: #33166

  • 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)

After musescore#32770, it seems that laying out harmony as text is no longer appropriate; change it to be laid out as harmony.
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@silasvorne silasvorne mentioned this pull request May 4, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 081c297f-5db3-4bc3-8757-e8dc02c7a123

📥 Commits

Reviewing files that changed from the base of the PR and between 57cfcd2 and 80244f0.

📒 Files selected for processing (1)
  • src/engraving/tests/chordsymbol_tests.cpp

📝 Walkthrough

Walkthrough

Two calls in Harmony (startEdit and edit) were changed from renderer()->layoutText1(this, true) to renderer()->layoutText1(this), altering the layout mode used during editing while keeping triggerLayout() and existing parsing/spellcheck logic. A unit test harmonyEditKeepsDisplayPosition was added to verify a Harmony canvas position remains unchanged through an edit; a file-local POSITION_ERROR tolerance constant and an EditData include were also added to the test file.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% 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 PR title accurately describes the main change: switching harmony layout from text-based to harmony-based during editing, directly addressing issue #33166.
Description check ✅ Passed The PR description includes the resolved issue reference, motivation for the change, and all required checklist items are marked complete.
Linked Issues check ✅ Passed The code changes properly address the positioning issue by removing the 'layout as text' parameter and using 'layout as harmony' instead, which aligns with fixing the vertical positioning regression in #33166.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective: harmony layout modifications in editing code and a corresponding unit test to verify positioning remains unchanged.

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

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

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
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

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

Add a test case to the 4.7 branch to ensure current behavior.
Prepare for fixing musescore#33166 on the master branch.

(cherry picked from commit e62bec1)
@mathesoncalum mathesoncalum requested a review from mike-spa May 5, 2026 07:27
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.

Chord text box appears too high

3 participants