Stave sharing - Part 3#33838
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a new 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/engraving/rendering/score/stavesharinglayout.cpp`:
- Around line 411-417: The forward-tie compatibility check is comparing the
wrong endpoint for tie consistency. In the condition that checks
`tieFor1->startElement()` against `tieFor2->startElement()`, you are checking
the start of the ties when you should be checking the end. Replace the
`startElement()` calls with `endElement()` calls (or `endNote()` if that is the
appropriate method) to correctly compare whether forward ties have matching
open-vs-closed endpoint configurations.
- Around line 703-708: There is a null dereference bug in the tieFor cloning
logic. When the condition checks if sharedTieFor is null with if
(!sharedTieFor), the code immediately tries to call sharedTieFor->clone() inside
that block, which dereferences a null pointer and causes a crash. The fix is to
clone tieFor (the original tie object) instead of sharedTieFor when sharedTieFor
is null. Change the clone call from sharedTieFor->clone() to tieFor->clone() to
use the correct source object for cloning.
- Around line 375-395: The annotation matching logic does not enforce one-to-one
correspondence between annotations on the previous and next tracks. Currently,
when iterating through annotationsOnPrevTrack and comparing each against
candidates in annotationsOnNextTrack via equal_range, there is no mechanism to
remove or mark matched annotations as consumed. This means a single annotation
from the next track can be matched multiple times, breaking duplicate handling.
To fix this, modify the matching logic to track which annotations from
annotationsOnNextTrack have already been matched (for example, using a set of
matched keys or erasing matched items), and ensure that each annotation is only
matched once. After finding a matching annotation in the inner loop (the loop
iterating through equal_range results), immediately mark or remove it so
subsequent iterations in the outer loop cannot reuse it. This will enforce
strict one-to-one pairing of annotations between the two tracks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7217ee5e-37fa-4426-a4f4-4c3b1b31a392
📒 Files selected for processing (5)
src/engraving/dom/engravingitem.cppsrc/engraving/dom/engravingobject.cppsrc/engraving/rendering/score/stavesharinglayout.cppsrc/engraving/rendering/score/stavesharinglayout.hsrc/engraving/types/types.h
💤 Files with no reviewable changes (1)
- src/engraving/dom/engravingitem.cpp
miiizen
left a comment
miiizen
left a comment
There was a problem hiding this comment.
Spotted a few things, but this is looking so good! 😍
There was a problem hiding this comment.
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/engravingitem.cpp (1)
1348-1350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing assertion for
originItem->ldata()->m_sharedItem == nullptr.The assertion validates that
sharedItemisn't already a shared item and thatoriginItemisn't acting as a shared item, but it doesn't verify thatoriginItemisn't already connected to a different shared item.If
originItem->m_sharedItempoints tooldSharedItem(notsharedItem), the early return on line 1340 is bypassed, and line 1352 overwritesm_sharedItem. This leavesoldSharedItem->m_originItemswith a stale pointer tooriginItem.🛡️ Suggested fix
- IF_ASSERT_FAILED(sharedItem->ldata()->m_sharedItem == nullptr && originItem->ldata()->m_originItems.empty()) { + IF_ASSERT_FAILED(sharedItem->ldata()->m_sharedItem == nullptr + && originItem->ldata()->m_sharedItem == nullptr + && originItem->ldata()->m_originItems.empty()) { return; }🤖 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/engravingitem.cpp` around lines 1348 - 1350, The assertion in the IF_ASSERT_FAILED condition is incomplete and missing a check to ensure originItem is not already connected to a different shared item. Currently it only validates that sharedItem->ldata()->m_sharedItem is nullptr and originItem->ldata()->m_originItems is empty, but it does not check that originItem->ldata()->m_sharedItem is also nullptr. Add the missing condition originItem->ldata()->m_sharedItem == nullptr to the assertion to prevent the code from overwriting m_sharedItem when originItem is already connected to another shared item, which would leave a stale pointer in the old shared item's m_originItems collection.
🤖 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/engravingitem.cpp`:
- Around line 1348-1350: The assertion in the IF_ASSERT_FAILED condition is
incomplete and missing a check to ensure originItem is not already connected to
a different shared item. Currently it only validates that
sharedItem->ldata()->m_sharedItem is nullptr and
originItem->ldata()->m_originItems is empty, but it does not check that
originItem->ldata()->m_sharedItem is also nullptr. Add the missing condition
originItem->ldata()->m_sharedItem == nullptr to the assertion to prevent the
code from overwriting m_sharedItem when originItem is already connected to
another shared item, which would leave a stale pointer in the old shared item's
m_originItems collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc234357-b50d-4106-9d08-bd1952b9db4a
📒 Files selected for processing (6)
src/engraving/dom/engravingitem.cppsrc/engraving/dom/engravingitem.hsrc/engraving/dom/engravingobject.cppsrc/engraving/rendering/score/stavesharinglayout.cppsrc/engraving/rendering/score/stavesharinglayout.hsrc/engraving/types/types.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/engraving/dom/engravingobject.cpp
- src/engraving/types/types.h
- src/engraving/rendering/score/stavesharinglayout.h
- src/engraving/rendering/score/stavesharinglayout.cpp
Dynamics, text, etc
Because the current m_sharedItem may have been already invalidated, so don't try to use it.
They can never belong to one of the origin staves, e.g. the top ones always belong to track 0, which is the shared staves. So exclude them from disconnect and cleanup
2 participants
No description provided.