[4.7] Allow deleting and dragging parentheses

[miiizen]

Resolves: #33812

[coderabbitai]

📝 Walkthrough

Walkthrough

Two small changes are made to parenthesis element handling in the engraving domain. In parenthesis.cpp, the Parenthesis constructor now passes ElementFlag::MOVABLE to the EngravingItem base class. In chord.cpp, the debug log message for the articulation-not-found case is corrected from ChordRest::remove() to Chord::remove(), and a new ElementType::PARENTHESIS branch is added to Chord::remove that looks up the associated NoteParenthesisInfo, aborts with an assertion failure if the parenthesis does not belong to the chord, and otherwise delegates removal to EditChord::removeChordParentheses.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description provides only an issue reference without following the template structure with required sections like summary, motivation, or checklists.	Complete the PR description template by adding a brief summary of changes, motivation, and filling in the required checklist items.
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main changes: restoring the ability to delete and drag parentheses in MuseScore 4.7.
Linked Issues check	✅ Passed	The changes directly address both requirements from #33812: adding deletion handling for parentheses via IF_ASSERT_FAILED and making parentheses MOVABLE.
Out of Scope Changes check	✅ Passed	All changes in chord.cpp and parenthesis.cpp are directly related to implementing parenthesis deletion and dragging functionality as specified in issue #33812.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Linked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. 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]

Caution

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

⚠️ Outside diff range comments (1)

src/engraving/dom/chord.cpp (1)

783-800: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Add a break after articulation/tapping removal to prevent unintended fallthrough.

Line 792 currently executes for articulation/ornament/tapping removals too, because the case at Lines 783-791 has no terminating break. That routes non-parenthesis elements into parenthesis-removal logic and can trigger assertion/early-return paths while skipping expected removal lifecycle handling.

Proposed fix

     case ElementType::ARTICULATION:
     case ElementType::ORNAMENT:
     case ElementType::TAPPING:
     {
         Articulation* a = toArticulation(e);
         if (!muse::remove(m_articulations, a)) {
             LOGD("Chord::remove(): articulation not found");
         }
     }
+    break;
     case ElementType::PARENTHESIS: {
         NoteParenthesisInfo* parenInfo = findNoteParenthesisInfo(toParenthesis(e));
         IF_ASSERT_FAILED(parenInfo) {
             LOGD() << "Chord::remove(): This parenthesis does not belong to this chord";
             return;
         }
         EditChord::removeChordParentheses(this, parenInfo->notes());
         break;
     }

🤖 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/chord.cpp` around lines 783 - 800, The switch statement for
element type removal is missing a break statement after the block handling
ElementType::ARTICULATION, ElementType::ORNAMENT, and ElementType::TAPPING
cases. Add a break statement immediately after the closing brace of that case
block (after the muse::remove and logging code) to prevent unintended
fallthrough into the ElementType::PARENTHESIS case, which would cause
non-parenthesis elements to be processed by parenthesis removal logic and skip
expected removal lifecycle handling.

🤖 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.

Outside diff comments:
In `@src/engraving/dom/chord.cpp`:
- Around line 783-800: The switch statement for element type removal is missing
a break statement after the block handling ElementType::ARTICULATION,
ElementType::ORNAMENT, and ElementType::TAPPING cases. Add a break statement
immediately after the closing brace of that case block (after the muse::remove
and logging code) to prevent unintended fallthrough into the
ElementType::PARENTHESIS case, which would cause non-parenthesis elements to be
processed by parenthesis removal logic and skip expected removal lifecycle
handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c60b8f57-f6b6-4fe4-97e2-8854a28bdb4d

📥 Commits

Reviewing files that changed from the base of the PR and between b98651a and f936918.

📒 Files selected for processing (2)

• src/engraving/dom/chord.cpp
• src/engraving/dom/parenthesis.cpp