Stave sharing - Part 4#34036
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:
📝 WalkthroughWalkthroughThis change updates shared voice and unison label formatting, adds new style settings and stave-sharing dialog controls for shared-on-staff numeral behavior, extends property and undo propagation to origin and shared items, and guards shared tuplet connection setup behind an assertion check. Changes: Sequence Diagram(s)sequenceDiagram
participant StaveSharingLayout
participant SystemHeaderLayout
participant EngravingObject
participant TextBase
StaveSharingLayout->>SystemHeaderLayout: formatSharedVoiceLabel(origin instruments, trailing-dot and hyphen style flags)
SystemHeaderLayout-->>StaveSharingLayout: formatted shared label prefix
StaveSharingLayout->>StaveSharingLayout: add restatement parentheses when needed
EngravingObject->>EngravingObject: propagate changeProperty to originItems()
EngravingObject->>EngravingObject: propagate changeProperty to sharedItem() when first origin
TextBase->>TextBase: schedule ChangeTextProperties on originItems()
TextBase->>TextBase: schedule ChangeTextProperties on sharedItem() when first origin
Related PRs: None identified from provided context. Suggested labels: engraving, style, qml Suggested reviewers: None identified from provided context. Poem: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ 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.
🧹 Nitpick comments (1)
src/engraving/rendering/score/systemheaderlayout.cpp (1)
1230-1279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
'\n'check informatSharedVoiceLabel.Line 1239's
result.back() != '\n'guard is unreachable: every call site (the merged HORIZONTAL list, each VOICE-group call, andformatUnisonLabel's call) starts with a fresh emptyString, and this function never inserts'\n'intoresultitself — that join happens in the caller (Line 1206). This appears to be a leftover from collapsing the old two-voiceformatVoice(isFirstVoice)logic into a single helper.♻️ Suggested simplification
- if (!result.empty() && result.back() != '\n') { + if (!result.empty()) { result += '.'; }🤖 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/rendering/score/systemheaderlayout.cpp` around lines 1230 - 1279, The `formatSharedVoiceLabel` helper still contains a dead `result.back() != '\n'` guard that can never trigger because the function always builds a fresh `String` and never appends newlines itself. Remove that newline check and keep the separator logic inside `SystemHeaderLayout::formatSharedVoiceLabel` focused on joining labels with dots/hyphens only, since newline insertion is handled by the caller.
🤖 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.
Nitpick comments:
In `@src/engraving/rendering/score/systemheaderlayout.cpp`:
- Around line 1230-1279: The `formatSharedVoiceLabel` helper still contains a
dead `result.back() != '\n'` guard that can never trigger because the function
always builds a fresh `String` and never appends newlines itself. Remove that
newline check and keep the separator logic inside
`SystemHeaderLayout::formatSharedVoiceLabel` focused on joining labels with
dots/hyphens only, since newline insertion is handled by the caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2e8c465-f84a-493e-a309-19820d1096a2
📒 Files selected for processing (3)
src/engraving/rendering/score/stavesharinglayout.cppsrc/engraving/rendering/score/systemheaderlayout.cppsrc/engraving/rendering/score/systemheaderlayout.h
mike-spa
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dom/textbase.cpp`:
- Around line 3390-3397: The shared-text undo path in TextBase::setProperty is
recording ChangeTextProperties on this->score() even when the cursor belongs to
a different origin/shared item. Update the loop over originItems() and the
sharedItem() propagation so each undo command is submitted through the target
item’s owning score, matching the linkedText->score() behavior and keeping the
undo transaction attached to the edited object.
🪄 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: a56e4de5-ddba-4e95-a4a4-4301e8fc3d54
📒 Files selected for processing (3)
src/engraving/dom/engravingobject.cppsrc/engraving/dom/textbase.cppsrc/engraving/rendering/score/stavesharinglayout.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engraving/rendering/score/stavesharinglayout.cpp
| for (EngravingItem* originItem : originItems()) { | ||
| // This is a shared item: propagate to all origin items | ||
| score()->undo(new ChangeTextProperties(toTextBase(originItem)->cursor(), id, v, ps)); | ||
| } | ||
| if (EngravingItem* sharedEl = sharedItem(); sharedEl && sharedEl->originItems().front() == this) { | ||
| // This is the first origin item of the shared item: propagate to shared item | ||
| score()->undo(new ChangeTextProperties(toTextBase(sharedEl)->cursor(), id, v, ps)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the target item's score for shared text undo commands.
The linked-object path above uses linkedText->score(), but this new shared-item path records undo commands on this->score() while targeting another item's cursor. Route each command through the target item’s score to keep the undo transaction attached to the edited object’s owning score.
Proposed fix
for (EngravingItem* originItem : originItems()) {
// This is a shared item: propagate to all origin items
- score()->undo(new ChangeTextProperties(toTextBase(originItem)->cursor(), id, v, ps));
+ originItem->score()->undo(new ChangeTextProperties(toTextBase(originItem)->cursor(), id, v, ps));
}
if (EngravingItem* sharedEl = sharedItem(); sharedEl && sharedEl->originItems().front() == this) {
// This is the first origin item of the shared item: propagate to shared item
- score()->undo(new ChangeTextProperties(toTextBase(sharedEl)->cursor(), id, v, ps));
+ sharedEl->score()->undo(new ChangeTextProperties(toTextBase(sharedEl)->cursor(), id, v, ps));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (EngravingItem* originItem : originItems()) { | |
| // This is a shared item: propagate to all origin items | |
| score()->undo(new ChangeTextProperties(toTextBase(originItem)->cursor(), id, v, ps)); | |
| } | |
| if (EngravingItem* sharedEl = sharedItem(); sharedEl && sharedEl->originItems().front() == this) { | |
| // This is the first origin item of the shared item: propagate to shared item | |
| score()->undo(new ChangeTextProperties(toTextBase(sharedEl)->cursor(), id, v, ps)); | |
| } | |
| for (EngravingItem* originItem : originItems()) { | |
| // This is a shared item: propagate to all origin items | |
| originItem->score()->undo(new ChangeTextProperties(toTextBase(originItem)->cursor(), id, v, ps)); | |
| } | |
| if (EngravingItem* sharedEl = sharedItem(); sharedEl && sharedEl->originItems().front() == this) { | |
| // This is the first origin item of the shared item: propagate to shared item | |
| sharedEl->score()->undo(new ChangeTextProperties(toTextBase(sharedEl)->cursor(), id, v, ps)); | |
| } |
🤖 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/textbase.cpp` around lines 3390 - 3397, The shared-text
undo path in TextBase::setProperty is recording ChangeTextProperties on
this->score() even when the cursor belongs to a different origin/shared item.
Update the loop over originItems() and the sharedItem() propagation so each undo
command is submitted through the target item’s owning score, matching the
linkedText->score() behavior and keeping the undo transaction attached to the
edited object.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/notationscene/qml/MuseScore/NotationScene/styledialog/StaveSharingPage.qml (1)
72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent checkbox toggle idiom.
Line 75 uses
!staveSharingModel.allowVoiceCrossing.valuewhile line 141 uses!checkedfor the same kind of fully-controlledCheckBox. Both are functionally equivalent here, but using!checkedconsistently (as in line 141) is the established idiom for this component in the codebase.♻️ Optional consistency fix
CheckBox { text: qsTrc("notation/editstyle/stavesharing", "Allow voice crossing") checked: staveSharingModel.allowVoiceCrossing.value === true - onClicked: staveSharingModel.allowVoiceCrossing.value = !staveSharingModel.allowVoiceCrossing.value + onClicked: staveSharingModel.allowVoiceCrossing.value = !checked }Also applies to: 138-142
🤖 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/notationscene/qml/MuseScore/NotationScene/styledialog/StaveSharingPage.qml` around lines 72 - 76, The fully controlled CheckBox in StaveSharingPage.qml uses an inconsistent toggle pattern for “Allow voice crossing”; update the onClicked handler in the CheckBox tied to staveSharingModel.allowVoiceCrossing so it follows the same established idiom as the other CheckBox in this component. Keep the checked binding as-is, but change the toggle logic to use the current checked state rather than directly negating the model value, matching the pattern used by the other control in StaveSharingPage.qml.Source: Learnings
🤖 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.
Nitpick comments:
In
`@src/notationscene/qml/MuseScore/NotationScene/styledialog/StaveSharingPage.qml`:
- Around line 72-76: The fully controlled CheckBox in StaveSharingPage.qml uses
an inconsistent toggle pattern for “Allow voice crossing”; update the onClicked
handler in the CheckBox tied to staveSharingModel.allowVoiceCrossing so it
follows the same established idiom as the other CheckBox in this component. Keep
the checked binding as-is, but change the toggle logic to use the current
checked state rather than directly negating the model value, matching the
pattern used by the other control in StaveSharingPage.qml.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4575c34b-1078-4bab-b447-73d46fb6ef62
📒 Files selected for processing (10)
src/engraving/dom/engravingobject.cppsrc/engraving/dom/textbase.cppsrc/engraving/rendering/score/stavesharinglayout.cppsrc/engraving/rendering/score/systemheaderlayout.cppsrc/engraving/rendering/score/systemheaderlayout.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/notationscene/qml/MuseScore/NotationScene/styledialog/StaveSharingPage.qmlsrc/notationscene/qml/MuseScore/NotationScene/styledialog/stavesharingpagemodel.cppsrc/notationscene/qml/MuseScore/NotationScene/styledialog/stavesharingpagemodel.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/engraving/rendering/score/systemheaderlayout.h
- src/engraving/dom/engravingobject.cpp
- src/engraving/rendering/score/stavesharinglayout.cpp
- src/engraving/dom/textbase.cpp
- src/engraving/rendering/score/systemheaderlayout.cpp
2 participants
Resolves: #34033
Resolves: #34039