Skip to content

Fix implode now works when voice 1 has only rests#32862

Open
CubikingChill wants to merge 1 commit intomusescore:masterfrom
CubikingChill:implode-explode-rework
Open

Fix implode now works when voice 1 has only rests#32862
CubikingChill wants to merge 1 commit intomusescore:masterfrom
CubikingChill:implode-explode-rework

Conversation

@CubikingChill
Copy link
Copy Markdown
Contributor

@CubikingChill CubikingChill commented Mar 30, 2026

This is the complete version of 3rd commit of #4309

In the current implode code, whether a tick/beat is empty/all-rests or not solely depends on whether voice 1 is empty or not. Which leads to the buggy behaviour: When voice 1 is empty, implode would just empty all voices, as if the whole tick/beat is empty.

My suggested fix is that we check all voices from 1 to 4 to make sure that the relevant tick/beat is really empty. Otherwise, we will choose the first/lowest-numbered voice as the implode destination.

It is definitely worth discussion that what should be the destination of the imploded chord. The current implementation follows the suggestion comments in the code.

2026-03-30.19-53-18.-.Trim.mp4

Resolves: #32830

  • 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
    • Improved the implode function to reliably prevent accidental deletion of destination chords during note merging operations. The function now correctly identifies and preserves target chords across all voice positions within the staff, ensuring data integrity when consolidating notes from multiple voice tracks into a single staff.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

Updated Score::cmdImplode() to search for the first chord across all voice tracks in the destination staff/segment instead of assuming voice 1 contains the destination chord. Added a safeguard to prevent deletion of the destination chord when consolidating notes.

Changes

Cohort / File(s) Summary
Implode Destination Chord Selection
src/engraving/editing/cmd.cpp
Modified chord discovery logic in single-staff case to scan all voices for the first chord, updated tie evaluation to use discovered chord from any voice, and added src != dstChord check when deleting source content to prevent accidental removal of the destination chord during merge.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code changes directly address issue #32830 by checking all voices instead of only voice 1 to determine if a tick/beat is empty, and selecting the first non-empty voice as the destination.
Out of Scope Changes check ✅ Passed All changes in cmd.cpp are directly related to fixing the implode behavior when voice 1 has only rests, with no unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title accurately describes the main fix: implode now works when voice 1 contains only rests, which is the core issue addressed by checking all voices instead of just voice 1.
Description check ✅ Passed The PR description adequately covers the problem statement, motivation, and fix. It addresses issue #32830, explains the buggy behavior, proposes the solution, acknowledges design considerations, and completes the required checklist items (except unit tests).

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

@CubikingChill CubikingChill force-pushed the implode-explode-rework branch from c9ee447 to 14a634d Compare March 30, 2026 19:17
@davidstephengrant
Copy link
Copy Markdown
Contributor

@CubikingChill The attached screen recording is not relevant to this PR. Please can you attach the correct recording.

@CubikingChill
Copy link
Copy Markdown
Contributor Author

@CubikingChill The attached screen recording is not relevant to this PR. Please can you attach the correct recording.

Oops. Fixed.

@CubikingChill CubikingChill changed the title Fix #174111: implode now works when voice 1 has only rests Fix implode now works when voice 1 has only rests Mar 30, 2026
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.

Implode doesn't work when voice 1 are rests

2 participants