Repository navigation
Fix #35279, #35280: Move tuplets correctly when Insert adds time or time is removed - #35282
davidstephengrant wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description explains both issues, the root causes, the fix, and the test coverage. However, it omits the required checklist and does not mark the AI-assistance checkbox or list the tool used in th… | Complete the template checklist accurately. Mark applicable items, including the unit-test and AI-assistance items, and list Claude Code 2.1.290 in the AI tools section. Confirm the remaining checklist items before submission. |
| Docstring Coverage | |
Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the two resolved issues and the main change: moving tuplets correctly when time is inserted or removed. Linked Issues check The changes address both active linked issues. For #35279,insertChordByInsertingTimeexcludes tuplets with shared origins from independent shifting, so shared and origin copies do not move twice. T…Out of Scope Changes check The source changes are limited to tuplet tick handling during time insertion and time deletion. The fixture and test changes support the linked regression cases. No unrelated product behavior or unrel… Full details: Linked Issues check
-
Explanation
The changes address both active linked issues. For
#35279,insertChordByInsertingTimeexcludes tuplets with shared origins from independent shifting, so shared and origin copies do not move twice. The stave-sharing test covers insertion, undo, redo, removal, and measure validity. For#35280,Score::doTimeDeleteForMeasurecollects affected nested tuplets and shifts their ticks with the later segments. Tests cover direct removal, removal of inserted time, nested tuplets, tuplet deletion, undo/redo, and score sanity.
Full details: Out of Scope Changes check
-
Explanation
The source changes are limited to tuplet tick handling during time insertion and time deletion. The fixture and test changes support the linked regression cases. No unrelated product behavior or unrelated file changes are demonstrated.
-
Full details: Description check
-
Explanation
The description explains both issues, the root causes, the fix, and the test coverage. However, it omits the required checklist and does not mark the AI-assistance checkbox or list the tool used in the required section.
Resolution
Complete the template checklist accurately. Mark applicable items, including the unit-test and AI-assistance items, and list Claude Code 2.1.290 in the AI tools section. Confirm the remaining checklist items before submission.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were 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.
Comment @coderabbitai help to get the list of available commands.
|
|
||
| // shared and origin tuplets are both in the set: don't propagate | ||
| for (Tuplet* t : tuplets) { | ||
| undo(new ChangeProperty(t, Pid::TICK, t->tick() - len)); |
There was a problem hiding this comment.
I would prefer it if we filtered the set so it only contained main and origin items and continued to use undoChangeProperty. Same in noteinput
There was a problem hiding this comment.
Done in both places: shared tuplets are now filtered out and the rest use undoChangeProperty again.
With stave sharing on, Insert collected the tuplets of both the origin staves and the shared staff, and undoChangeProperty(Pid::TICK) also propagated each change to the shared or origin copy, so tuplets moved by twice the inserted length. Deleting one of them later corrupted the measure. Leave shared tuplets out of the set: they follow their origin. Assisted-by: Claude Code 2.1.290 (Claude Opus 5.5)
Score::doTimeDeleteForMeasure() moved the measure's later segments back but not the ticks that tuplets store, so a tuplet after the removed time kept its old tick. Deleting that tuplet then put its rest at the old position, corrupting the measure. Since Insert moves tuplets forward, removing inserted time always left this state. Move those tuplets back with their segments, leaving shared tuplets to follow their origin, as Insert does. Assisted-by: Claude Code 2.1.290 (Claude Opus 5.5)
4457125 to
34a3017
Compare
This branch has not been deployed
2 participants
Resolves: #35279
Resolves: #35280
Follow-up to #35261, which made Insert move the tuplets after the insertion point.
undoChangeProperty, which also moved its shared or origin copy, and both copies were in the list, so tuplets moved by twice the inserted length.Both now change the tuplets' ticks directly, without propagation. Unit tests cover Insert and remove time with stave sharing, remove time before a tuplet, and removing inserted time before a nested tuplet.
Screen.Recording.2026-10-18.at.11.19.41.mov
Assisted by Claude Code 2.1.290 (Claude Opus 5.5)