Fix #31056 and add tie and slur masking over key and time signatures#33024
Fix #31056 and add tie and slur masking over key and time signatures#33024ajuncosa wants to merge 7 commits intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughThis change extends Z-order management for spanner segments and implements a masking system for slurs and ties. The modifications make 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/rendering/score/tdraw.cpp (1)
3094-3101:⚠️ Potential issue | 🔴 CriticalBalance the painter state on the early-return path.
painter->save()now runs before the cross-measure guard, but the return on Line 3099 exits withoutrestore(). That leaves the painter stack/mask state dirty for subsequent drawing.🐛 Proposed fix
- painter->save(); - - setMask(item, painter); - // hide tie toward the second chord of a cross-measure value if (item->tie()->endNote() && item->tie()->endNote()->chord()->crossMeasure() == CrossMeasure::SECOND) { return; } + painter->save(); + DEFER { + painter->restore(); + }; + + setMask(item, painter); @@ - painter->restore();Also applies to: 3143-3144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/rendering/score/tdraw.cpp` around lines 3094 - 3101, The painter state is saved by painter->save() before the cross-measure guard but not restored on the early return; update the guard that checks item->tie()->endNote() && item->tie()->endNote()->chord()->crossMeasure() == CrossMeasure::SECOND to call painter->restore() (and undo any mask set by setMask(item, painter) if necessary) before returning, and apply the same change to the similar early-return guard later in the file (the other occurrence around the same cross-measure check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engraving/rendering/score/masklayout.cpp`:
- Around line 83-91: The loop is skipping continuation slur/tie segments because
of the isSingleBeginType() check; remove that check so continuation segments are
processed too. In the loop over system->spannerSegments(), keep the visibility
and staff show checks (system->staff(spannerSeg->staffIdx())->show() and
spannerSeg->visible()), but drop the !spannerSeg->isSingleBeginType() condition
and still call computeSlurTieMasks(toSlurTieSegment(spannerSeg),
SegmentType::KeySig | SegmentType::TimeSig) when (maskSlurs &&
spannerSeg->isSlurSegment()) || (maskTies && spannerSeg->isTieSegment()) so
masks are written on continuation SlurTieSegment instances as well.
In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qml`:
- Around line 137-146: The onClicked handler for the CheckBox
maskSlursOverTimeAndKeySignaturesCheckbox writes the inverse of the new state,
causing the model to revert; change the handler to use onToggled (or assign
directly in onClicked without negation) and set
root.pageModel.maskSlursOverTimeAndKeySignatures.value =
maskSlursOverTimeAndKeySignaturesCheckbox.checked (i.e., assign the current
checked state directly) so the page model reflects the new checkbox state.
In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml`:
- Around line 180-189: Replace the indirect negation handler on the CheckBox by
using onToggled and directly assigning the new state to the model: in the
CheckBox with id maskTiesOverTimeAndKeySignaturesCheckbox, replace the onClicked
block that sets root.pageModel.maskTiesOverTimeAndKeySignatures.value =
!maskTiesOverTimeAndKeySignaturesCheckbox.checked with an onToggled handler that
assigns root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked (or the
toggled parameter), removing the unnecessary negation and aligning with Qt Quick
idioms.
---
Outside diff comments:
In `@src/engraving/rendering/score/tdraw.cpp`:
- Around line 3094-3101: The painter state is saved by painter->save() before
the cross-measure guard but not restored on the early return; update the guard
that checks item->tie()->endNote() &&
item->tie()->endNote()->chord()->crossMeasure() == CrossMeasure::SECOND to call
painter->restore() (and undo any mask set by setMask(item, painter) if
necessary) before returning, and apply the same change to the similar
early-return guard later in the file (the other occurrence around the same
cross-measure check).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 34b737f5-57a9-472a-9118-7f8738d93782
📒 Files selected for processing (10)
src/engraving/dom/spanner.cppsrc/engraving/rendering/score/masklayout.cppsrc/engraving/rendering/score/masklayout.hsrc/engraving/rendering/score/tdraw.cppsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qmlsrc/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qmlsrc/notationscene/qml/MuseScore/NotationScene/styledialog/slursandtiespagemodel.cppsrc/notationscene/qml/MuseScore/NotationScene/styledialog/slursandtiespagemodel.h
| CheckBox { | ||
| id: maskSlursOverTimeAndKeySignaturesCheckbox | ||
| text: qsTrc("notation/editstyle/slursandties", "Mask slurs over time and key signatures") | ||
| checked: root.pageModel.maskSlursOverTimeAndKeySignatures.value | ||
| width: parent.width | ||
| Layout.columnSpan: 2 | ||
|
|
||
| onClicked: { | ||
| root.pageModel.maskSlursOverTimeAndKeySignatures.value = !maskSlursOverTimeAndKeySignaturesCheckbox.checked | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt Quick Controls 2, for a CheckBox/AbstractButton, is the clickedsignal emitted before or after thecheckedstate changes? IsonToggled: modelProperty = checked the recommended pattern for syncing a bound boolean setting?
💡 Result:
In Qt Quick Controls 2, for CheckBox and AbstractButton, the clicked signal is emitted when the button is interactively clicked (after press and release). The toggled signal (from QtQuick.Controls 2.2+) is emitted when a checkable button is interactively toggled. The checked state changes before both signals are emitted: on user interaction, the control first toggles the checked property (via toggle or equivalent), then emits toggled(checked) and clicked. Thus, clicked and toggled are emitted after the checked state changes. onToggled: modelProperty = checked is a recommended pattern for syncing a bound boolean model property. Direct binding checked: modelProperty breaks on user click (assignment overrides binding), so use onToggled (or onCheckedChanged) to update the model with the new checked value. Keep an initial binding or Component.onCompleted sync for model→UI, and use Connections for model change notifications if needed. This ensures two-way sync without binding loss.
Citations:
- 1: https://doc.qt.io/qt-6/qml-qtquick-controls-abstractbutton.html
- 2: https://doc.qt.io/qt-6/qml-qtquick-controls-checkbox.html
- 3: https://stackoverflow.com/questions/23860270/binding-checkbox-checked-property-with-a-c-object-q-property
- 4: https://stackoverflow.com/questions/38798450/qt-5-7-qml-why-are-my-checkbox-property-bindings-disappearing
- 5: https://runebook.dev/en/docs/qt/qml-qtquick-controls-buttongroup/checkState
- 6: https://codebrowser.dev/qt5/qtquickcontrols2/src/quicktemplates2/qquickabstractbutton.cpp.html
🏁 Script executed:
fd -t f "SlursSection.qml"Repository: musescore/MuseScore
Length of output: 137
🏁 Script executed:
sed -n '137,146p' src/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qmlRepository: musescore/MuseScore
Length of output: 582
Write the checkbox state back directly, not its inverse.
With checked bound to the page model, negating maskSlursOverTimeAndKeySignaturesCheckbox.checked here writes the previous value back after the control toggles, causing the setting to snap back instead of changing. In Qt Quick Controls, the checked state changes before the clicked signal fires, so the negation reverses the new toggled state. Use onToggled and assign checked directly instead.
♻️ Proposed fix
CheckBox {
id: maskSlursOverTimeAndKeySignaturesCheckbox
text: qsTrc("notation/editstyle/slursandties", "Mask slurs over time and key signatures")
checked: root.pageModel.maskSlursOverTimeAndKeySignatures.value
width: parent.width
Layout.columnSpan: 2
- onClicked: {
- root.pageModel.maskSlursOverTimeAndKeySignatures.value = !maskSlursOverTimeAndKeySignaturesCheckbox.checked
- }
+ onToggled: root.pageModel.maskSlursOverTimeAndKeySignatures.value = checked
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qml`
around lines 137 - 146, The onClicked handler for the CheckBox
maskSlursOverTimeAndKeySignaturesCheckbox writes the inverse of the new state,
causing the model to revert; change the handler to use onToggled (or assign
directly in onClicked without negation) and set
root.pageModel.maskSlursOverTimeAndKeySignatures.value =
maskSlursOverTimeAndKeySignaturesCheckbox.checked (i.e., assign the current
checked state directly) so the page model reflects the new checkbox state.
| CheckBox { | ||
| id: maskTiesOverTimeAndKeySignaturesCheckbox | ||
| text: qsTrc("notation/editstyle/slursandties", "Mask ties over time and key signatures") | ||
| checked: root.pageModel.maskTiesOverTimeAndKeySignatures.value | ||
| width: parent.width | ||
| Layout.columnSpan: 2 | ||
|
|
||
| onClicked: { | ||
| root.pageModel.maskTiesOverTimeAndKeySignatures.value = !maskTiesOverTimeAndKeySignaturesCheckbox.checked | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists
git ls-files | grep -i tiessection.qmlRepository: musescore/MuseScore
Length of output: 136
🏁 Script executed:
# Once we find the file, let's examine the exact lines mentioned
cat -n "src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml" | sed -n '175,195p'Repository: musescore/MuseScore
Length of output: 978
🏁 Script executed:
# Also check the broader context to understand the pattern
cat -n "src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml" | sed -n '170,210p'Repository: musescore/MuseScore
Length of output: 1856
🏁 Script executed:
# Search for similar checkbox patterns in the codebase to see how two-way binding is typically done
rg -A 5 "CheckBox.*{" --type qml src/notationscene/qml/ | head -60Repository: musescore/MuseScore
Length of output: 148
🏁 Script executed:
# Search for CheckBox patterns in qml files with better regex
rg "onClicked|onToggled" --type qml src/notationscene/qml/ -B 3 -A 2 | head -80Repository: musescore/MuseScore
Length of output: 8633
🏁 Script executed:
# Also look for other checkbox assignments to understand the pattern
rg "\.value = " --type qml src/notationscene/qml/ -B 2 -A 0 | grep -A 2 "CheckBox\|checked"Repository: musescore/MuseScore
Length of output: 36131
🏁 Script executed:
# Search more broadly for checkbox state binding patterns
rg "checked:" --type qml src/notationscene/qml/ -A 3 | head -60Repository: musescore/MuseScore
Length of output: 6525
Use onToggled with direct state assignment instead of negating checked.
Line 188 negates the checkbox state when writing to the model, creating an indirect toggle pattern. The idiomatic Qt Quick approach is to use onToggled (which fires with the new state) and assign directly to the model. This eliminates a redundant binding cycle and aligns with standard checkbox binding practices.
Proposed fix
CheckBox {
id: maskTiesOverTimeAndKeySignaturesCheckbox
text: qsTrc("notation/editstyle/slursandties", "Mask ties over time and key signatures")
checked: root.pageModel.maskTiesOverTimeAndKeySignatures.value
width: parent.width
Layout.columnSpan: 2
- onClicked: {
- root.pageModel.maskTiesOverTimeAndKeySignatures.value = !maskTiesOverTimeAndKeySignaturesCheckbox.checked
- }
+ onToggled: root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked
}📝 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.
| CheckBox { | |
| id: maskTiesOverTimeAndKeySignaturesCheckbox | |
| text: qsTrc("notation/editstyle/slursandties", "Mask ties over time and key signatures") | |
| checked: root.pageModel.maskTiesOverTimeAndKeySignatures.value | |
| width: parent.width | |
| Layout.columnSpan: 2 | |
| onClicked: { | |
| root.pageModel.maskTiesOverTimeAndKeySignatures.value = !maskTiesOverTimeAndKeySignaturesCheckbox.checked | |
| } | |
| CheckBox { | |
| id: maskTiesOverTimeAndKeySignaturesCheckbox | |
| text: qsTrc("notation/editstyle/slursandties", "Mask ties over time and key signatures") | |
| checked: root.pageModel.maskTiesOverTimeAndKeySignatures.value | |
| width: parent.width | |
| Layout.columnSpan: 2 | |
| onToggled: root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml`
around lines 180 - 189, Replace the indirect negation handler on the CheckBox by
using onToggled and directly assigning the new state to the model: in the
CheckBox with id maskTiesOverTimeAndKeySignaturesCheckbox, replace the onClicked
block that sets root.pageModel.maskTiesOverTimeAndKeySignatures.value =
!maskTiesOverTimeAndKeySignaturesCheckbox.checked with an onToggled handler that
assigns root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked (or the
toggled parameter), removing the unnecessary negation and aligning with Qt Quick
idioms.
…er time and key signatures
…painter if the function returns in the crossMeasure check
…igned, so the spanners' Z will be saved to XML file (instead of per-segment) and it will be used when reading the XML file and rendering the score
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/spanner.cpp (1)
394-401:⚠️ Potential issue | 🔴 CriticalKeep the base-class modified check for Z-order persistence.
Returning only
modifieddropsEngravingItem::isUserModified(), so a spanner segment whose only explicit edit isPid::Zcan still look unmodified afterPid::Zwas delegated above. That leaves the XML persistence bug unfixed for the exact mask-ordering case this PR is targeting.Proposed fix
- return modified; // ? modified : EngravingItem::isUserModified(); + return modified || EngravingItem::isUserModified();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/spanner.cpp` around lines 394 - 401, SpannerSegment::isUserModified currently returns only the local `modified` boolean, which omits the base-class check and thus can drop edits like Pid::Z; change the function to combine the local check with the base-class result by returning `modified || EngravingItem::isUserModified()` so that edits detected by EngravingItem (e.g., Z-order) are preserved; update the return expression in SpannerSegment::isUserModified accordingly.
♻️ Duplicate comments (3)
src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml (1)
180-189:⚠️ Potential issue | 🟠 MajorWrite the toggled state back directly.
This has the same issue as the slur checkbox:
checkedis already updated whenonClickedruns, so negating it writes the old value back and prevents the style from changing.In Qt Quick Controls 2, when a CheckBox is clicked, does the `checked` property change before the `clicked`/`onClicked` handler runs, and is `onToggled: modelProperty = checked` the recommended way to sync a boolean model value?Proposed fix
CheckBox { id: maskTiesOverTimeAndKeySignaturesCheckbox text: qsTrc("notation/editstyle/slursandties", "Mask ties over time and key signatures") checked: root.pageModel.maskTiesOverTimeAndKeySignatures.value width: parent.width Layout.columnSpan: 2 - onClicked: { - root.pageModel.maskTiesOverTimeAndKeySignatures.value = !maskTiesOverTimeAndKeySignaturesCheckbox.checked - } + onToggled: root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml` around lines 180 - 189, The CheckBox handler is negating the already-updated checked state and thus writes the old value back; in the CheckBox with id maskTiesOverTimeAndKeySignaturesCheckbox, stop inverting checked in the onClicked handler and instead assign the model directly from the current checked value (or use onToggled) so root.pageModel.maskTiesOverTimeAndKeySignatures.value = maskTiesOverTimeAndKeySignaturesCheckbox.checked (or use onToggled: root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked).src/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qml (1)
137-146:⚠️ Potential issue | 🟠 MajorWrite the toggled state back directly.
checkedhas already changed by the timeonClickedruns, so negating it here restores the old value and the setting never sticks. UseonToggledand assigncheckeddirectly instead.In Qt Quick Controls 2, when a CheckBox is clicked, does the `checked` property change before the `clicked`/`onClicked` handler runs, and is `onToggled: modelProperty = checked` the recommended way to sync a boolean model value?Proposed fix
CheckBox { id: maskSlursOverTimeAndKeySignaturesCheckbox text: qsTrc("notation/editstyle/slursandties", "Mask slurs over time and key signatures") checked: root.pageModel.maskSlursOverTimeAndKeySignatures.value width: parent.width Layout.columnSpan: 2 - onClicked: { - root.pageModel.maskSlursOverTimeAndKeySignatures.value = !maskSlursOverTimeAndKeySignaturesCheckbox.checked - } + onToggled: root.pageModel.maskSlursOverTimeAndKeySignatures.value = checked }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qml` around lines 137 - 146, The checkbox handler is reverting the new state because onClicked runs after checked already toggled; update the handler to use onToggled and write the checked value directly to the model (replace the onClicked block on maskSlursOverTimeAndKeySignaturesCheckbox with an onToggled handler that sets root.pageModel.maskSlursOverTimeAndKeySignatures.value = checked), ensuring the UI state and model stay in sync.src/engraving/rendering/score/masklayout.cpp (1)
83-91:⚠️ Potential issue | 🟠 MajorContinuation slur/tie segments are still skipped.
The
isSingleBeginType()guard means only the first system segment ever gets a mask. A slur or tie that continues onto a later system still won't be masked over that system's header key/time signature.Proposed fix
if (maskSlurs || maskTies) { for (SpannerSegment* spannerSeg : system->spannerSegments()) { - if (!spannerSeg->isSlurTieSegment() || !spannerSeg->isSingleBeginType() + if (!spannerSeg->isSlurTieSegment() || !system->staff(spannerSeg->staffIdx())->show() || !spannerSeg->visible()) { continue; } if ((maskSlurs && spannerSeg->isSlurSegment()) || (maskTies && spannerSeg->isTieSegment())) { computeSlurTieMasks(toSlurTieSegment(spannerSeg), SegmentType::KeySig | SegmentType::TimeSig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/rendering/score/masklayout.cpp` around lines 83 - 91, The loop currently skips continuation slur/tie segments due to the isSingleBeginType() guard; remove that check so continuation segments are considered as well—i.e., keep the filters isSlurTieSegment(), staff(...)->show(), and visible() but drop the isSingleBeginType() requirement so computeSlurTieMasks(toSlurTieSegment(spannerSeg), SegmentType::KeySig | SegmentType::TimeSig) runs for slur/tie segments on later systems (when maskSlurs or maskTies apply).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/engraving/dom/spanner.cpp`:
- Around line 394-401: SpannerSegment::isUserModified currently returns only the
local `modified` boolean, which omits the base-class check and thus can drop
edits like Pid::Z; change the function to combine the local check with the
base-class result by returning `modified || EngravingItem::isUserModified()` so
that edits detected by EngravingItem (e.g., Z-order) are preserved; update the
return expression in SpannerSegment::isUserModified accordingly.
---
Duplicate comments:
In `@src/engraving/rendering/score/masklayout.cpp`:
- Around line 83-91: The loop currently skips continuation slur/tie segments due
to the isSingleBeginType() guard; remove that check so continuation segments are
considered as well—i.e., keep the filters isSlurTieSegment(),
staff(...)->show(), and visible() but drop the isSingleBeginType() requirement
so computeSlurTieMasks(toSlurTieSegment(spannerSeg), SegmentType::KeySig |
SegmentType::TimeSig) runs for slur/tie segments on later systems (when
maskSlurs or maskTies apply).
In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qml`:
- Around line 137-146: The checkbox handler is reverting the new state because
onClicked runs after checked already toggled; update the handler to use
onToggled and write the checked value directly to the model (replace the
onClicked block on maskSlursOverTimeAndKeySignaturesCheckbox with an onToggled
handler that sets root.pageModel.maskSlursOverTimeAndKeySignatures.value =
checked), ensuring the UI state and model stay in sync.
In `@src/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qml`:
- Around line 180-189: The CheckBox handler is negating the already-updated
checked state and thus writes the old value back; in the CheckBox with id
maskTiesOverTimeAndKeySignaturesCheckbox, stop inverting checked in the
onClicked handler and instead assign the model directly from the current checked
value (or use onToggled) so
root.pageModel.maskTiesOverTimeAndKeySignatures.value =
maskTiesOverTimeAndKeySignaturesCheckbox.checked (or use onToggled:
root.pageModel.maskTiesOverTimeAndKeySignatures.value = checked).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9ab3254-3b26-4a84-abb2-4d259c926bf9
📒 Files selected for processing (14)
src/engraving/dom/engravingitem.hsrc/engraving/dom/slurtie.cppsrc/engraving/dom/spanner.cppsrc/engraving/dom/spanner.hsrc/engraving/rendering/score/masklayout.cppsrc/engraving/rendering/score/masklayout.hsrc/engraving/rendering/score/tdraw.cppsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/tests/readwriteundoreset_data/slurs.mscxsrc/notationscene/qml/MuseScore/NotationScene/styledialog/SlursSection.qmlsrc/notationscene/qml/MuseScore/NotationScene/styledialog/TiesSection.qmlsrc/notationscene/qml/MuseScore/NotationScene/styledialog/slursandtiespagemodel.cppsrc/notationscene/qml/MuseScore/NotationScene/styledialog/slursandtiespagemodel.h
💤 Files with no reviewable changes (2)
- src/engraving/dom/slurtie.cpp
- src/engraving/tests/readwriteundoreset_data/slurs.mscx
| continue; | ||
| } | ||
| if ((maskSlurs && spannerSeg->isSlurSegment()) || (maskTies && spannerSeg->isTieSegment())) { | ||
| computeSlurTieMasks(toSlurTieSegment(spannerSeg), SegmentType::KeySig | SegmentType::TimeSig); |
There was a problem hiding this comment.
No reason to pass the segment type from here, it can be written within computeSlurTieMask
There was a problem hiding this comment.
You're right, I kept it in case we wanted to separately personalise the types to mask in the future, but it's unnecessary for now. I'll update.
| TRACEFUNC; | ||
|
|
||
| Spanner* spanner = slurTieSegment->spanner(); | ||
| staff_idx_t spannerStaffTrackIdx = spanner->staffIdx() * VOICES; |
There was a problem hiding this comment.
track_idx_t spannerTrackIdx = spanner->track();
There was a problem hiding this comment.
Wouldn't this be a bit too specific? If the time and key signatures are always on voice 0, the ties/slurs corresponding to other voices on the same staff wouldn't be masked, so you could end up with something like this:
PS: this makes me wonder about cross-staff notation, maybe we shouldn't filter by staff either?
There was a problem hiding this comment.
Ha! You're right, good point
There was a problem hiding this comment.
I think you can use trackZeroVoice for that. And for cross-staff notation (very good observation, I would have brought it up later) you can either use Spanner::track2 which is precisely meant for that, or you can get start- and end chord and use their vStaffIdx(), and collect the key- and timeSigs from both staves
There was a problem hiding this comment.
Good to know, I'll have a look!
| continue; | ||
| } | ||
|
|
||
| Shape itemShape = item->shape().translated(itemPos); |
There was a problem hiding this comment.
You could move this above the if and then do if (!slurTieShape.bbox.intersects(itemShape).padded(...))
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/engraving/rendering/score/masklayout.cpp (1)
376-380:⚠️ Potential issue | 🟠 MajorFilter key/time-sig items by the spanner's staff/track.
seg->elist()returns items across all tracks of that segment. For a slur/tie on one staff this ends up masking key/time signatures that belong to other staves' tracks, which can produce spurious or out-of-staff masks — and for cross-staff spanners the correct staves may be missed entirely.Use the spanner's own track (and, for slurs,
track2()and/or thevStaffIdx()of start/end CRs for cross-staff) to restrict the mask targets, e.g. iterate only the relevantstaff2track(staffIdx)ranges on the segment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/rendering/score/masklayout.cpp` around lines 376 - 380, seg->elist() currently collects items from all tracks causing key/time signatures from other tracks to be masked; restrict itemsToMaskOver to only the spanner's relevant track(s): use the spanner's track (and for slurs/ties check track2() and the vStaffIdx() of the start/end CRs for cross-staff cases) to compute the staff2track(staffIdx) range(s) for this spanner and iterate only those tracks' items on the segment instead of seg->elist(); update the loop that fills itemsToMaskOver to query per-track item lists (or filter seg->elist() by item->track() matching the computed track range) so only same-staff (or intended cross-staff) key/time-sign items are considered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engraving/dom/spanner.cpp`:
- Line 469: The call ls->setZ(z()) triggers the virtual SpannerSegment::setZ
which forwards to m_spanner->setZ and re-writes Z on every segment; instead call
the non-virtual base implementation directly to only initialize the new segment.
Replace the virtual dispatch (ls->setZ(z())) with an explicit base-class call
(e.g., ls->Segment::setZ(z())) so only the new segment's Z is set; adjust the
call site where ls is created/initialized to use Segment::setZ rather than the
SpannerSegment override.
In `@src/engraving/rendering/score/masklayout.cpp`:
- Around line 421-431: Rename the ambiguous parameters in
MaskLayout::createFilteredItemShape to clarify filtering direction: change
overlyingItemShape -> filterShape and maskedItemShape -> targetShape (or
sourceShape -> maskedShape if you prefer), and update all call sites
accordingly; keep the internal logic and uses of filteredItemShape,
collisionPadding, ShapeElement, elements(), intersects, padded and add unchanged
so behavior is preserved while improving readability at invocation sites.
- Line 371: Compute spanner->endSegment() once before the loop and reuse it
instead of calling it on every iteration: call Spanner::endSegment() into a
local const Segment* (e.g., endSeg) prior to the for loop, then change the loop
condition from seg && seg != spanner->endSegment() to seg && seg != endSeg so
the expensive tick2segment/lookups inside Spanner::endSegment() are not executed
on each iteration.
---
Duplicate comments:
In `@src/engraving/rendering/score/masklayout.cpp`:
- Around line 376-380: seg->elist() currently collects items from all tracks
causing key/time signatures from other tracks to be masked; restrict
itemsToMaskOver to only the spanner's relevant track(s): use the spanner's track
(and for slurs/ties check track2() and the vStaffIdx() of the start/end CRs for
cross-staff cases) to compute the staff2track(staffIdx) range(s) for this
spanner and iterate only those tracks' items on the segment instead of
seg->elist(); update the loop that fills itemsToMaskOver to query per-track item
lists (or filter seg->elist() by item->track() matching the computed track
range) so only same-staff (or intended cross-staff) key/time-sign items are
considered.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bed90766-d2f5-4794-839b-f5def53a5360
📒 Files selected for processing (3)
src/engraving/dom/spanner.cppsrc/engraving/rendering/score/masklayout.cppsrc/engraving/rendering/score/masklayout.h
| ls->setSelected(selected()); | ||
| ls->setTrack(track()); | ||
| // ls->setAutoplace(autoplace()); | ||
| ls->setZ(z()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: unnecessary propagation to sibling segments.
ls->setZ(z()) invokes SpannerSegment::setZ, which in turn calls m_spanner->setZ(val) and re-writes Z on every already-existing segment (and the spanner) to the same value. Since only the new segment needs initialization, calling the non-virtual base directly avoids this redundant fan-out:
♻️ Proposed tweak
- ls->setZ(z());
+ ls->EngravingItem::setZ(z());📝 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.
| ls->setZ(z()); | |
| ls->EngravingItem::setZ(z()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/dom/spanner.cpp` at line 469, The call ls->setZ(z()) triggers
the virtual SpannerSegment::setZ which forwards to m_spanner->setZ and re-writes
Z on every segment; instead call the non-virtual base implementation directly to
only initialize the new segment. Replace the virtual dispatch (ls->setZ(z()))
with an explicit base-class call (e.g., ls->Segment::setZ(z())) so only the new
segment's Z is set; adjust the call site where ls is created/initialized to use
Segment::setZ rather than the SpannerSegment override.
|
|
||
| Spanner* spanner = slurTieSegment->spanner(); | ||
| std::vector<const EngravingItem*> itemsToMaskOver; | ||
| for (const Segment* seg = spanner->startSegment(); seg && seg != spanner->endSegment(); seg = seg->next1()) { |
There was a problem hiding this comment.
Hoist spanner->endSegment() out of the loop condition.
Spanner::endSegment() is not trivial — it performs tick2segment lookups and may fall back to further segment searches. Evaluating it on every iteration is wasteful; compute it once before the loop.
♻️ Proposed fix
- for (const Segment* seg = spanner->startSegment(); seg && seg != spanner->endSegment(); seg = seg->next1()) {
+ const Segment* const endSeg = spanner->endSegment();
+ for (const Segment* seg = spanner->startSegment(); seg && seg != endSeg; seg = seg->next1()) {📝 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 (const Segment* seg = spanner->startSegment(); seg && seg != spanner->endSegment(); seg = seg->next1()) { | |
| const Segment* const endSeg = spanner->endSegment(); | |
| for (const Segment* seg = spanner->startSegment(); seg && seg != endSeg; seg = seg->next1()) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/rendering/score/masklayout.cpp` at line 371, Compute
spanner->endSegment() once before the loop and reuse it instead of calling it on
every iteration: call Spanner::endSegment() into a local const Segment* (e.g.,
endSeg) prior to the for loop, then change the loop condition from seg && seg !=
spanner->endSegment() to seg && seg != endSeg so the expensive
tick2segment/lookups inside Spanner::endSegment() are not executed on each
iteration.
| Shape MaskLayout::createFilteredItemShape(const Shape& overlyingItemShape, const Shape& maskedItemShape, const double collisionPadding) | ||
| { | ||
| Shape filteredItemShape; | ||
| filteredItemShape.elements().reserve(overlyingItemShape.elements().size()); | ||
| for (const ShapeElement& el : overlyingItemShape.elements()) { | ||
| if (maskedItemShape.intersects(el.padded(collisionPadding))) { | ||
| filteredItemShape.add(el); | ||
| } | ||
| } | ||
| return filteredItemShape; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nice extraction.
Good deduplication of the filtering logic shared between barline-for-text and slur/tie masking. The parameter names overlyingItemShape / maskedItemShape read a bit ambiguously (both are "items"); renaming to something like filterShape / targetShape (or sourceShape / maskedShape) could make the filtering direction clearer at call sites, but this is cosmetic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/rendering/score/masklayout.cpp` around lines 421 - 431, Rename
the ambiguous parameters in MaskLayout::createFilteredItemShape to clarify
filtering direction: change overlyingItemShape -> filterShape and
maskedItemShape -> targetShape (or sourceShape -> maskedShape if you prefer),
and update all call sites accordingly; keep the internal logic and uses of
filteredItemShape, collisionPadding, ShapeElement, elements(), intersects,
padded and add unchanged so behavior is preserved while improving readability at
invocation sites.
2 participants
Resolves: #31056