Implement page locks#33737
Open
miiizen wants to merge 8 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces page-level layout locking through a comprehensive refactoring. The core change replaces the system-specific ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engraving/rendering/score/pagelayout.cpp (1)
586-617:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHandle overfull-page compaction in dedicated negative-space branches.
These loops still use the “grow the smallest gap” algorithm when
restHeight/spaceRemainingis negative. InlayoutPage(), that makesfillnegative and shrinks the smallest inter-system gaps first, which makes spacing less even instead of squeezing the loosest gaps. IndistributeStaves(),nextSmallestdrops belowsmallest, everystepbecomes negative, and the loop exits aftermaxPasseswithout changing any staff gap. With page locks enabled, overfull pages will therefore either compact the wrong places or not compact at all, depending on vertical-spread mode.Please split the negative-space case and explicitly remove space from the largest expandable gaps instead of feeding negative values through the existing spread logic.
Also applies to: 743-790
🤖 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/pagelayout.cpp` around lines 586 - 617, The current loop that increases system distances (in layoutPage() using restHeight and the pageFilled lambda and the sort over sList) fails when restHeight is negative because it feeds negative fills back into the "grow smallest gap" algorithm; update layoutPage() and the similar logic in distributeStaves() (the code that computes fill, totalFill, iterates k and calls s->setDistance(d)) to handle negative-space as a separate branch: when restHeight (or spaceRemaining) < 0, compute how much space must be removed and identify the largest expandable inter-system gaps (reverse the sort or compute expandable capacity per System via System::distance() - minimumAllowed or maxDist - System::height() as relevant), then subtract space from those largest gaps (clamping at their minimums) until the negative restHeight is consumed, updating s->setDistance(d) accordingly; do not pass negative fill into the existing positive-growth loop, and mirror the same change for the distributeStaves() loop that uses nextSmallest, smallest, step and maxPasses so that steps remove space from largest gaps instead of producing negative steps that no-op.src/engraving/editing/cmd.cpp (1)
4005-4011:⚠️ Potential issue | 🔴 CriticalAdd page-lock guard for NOBREAK
cmdToggleLayoutBreak()blocksLayoutBreakType::NOBREAKonly whenmb->systemLock()exists, butMeasureBasealso providespageLock(). The page-lock editor helper (EditPageLocks::removePageLocksOnAddLayoutBreak) explicitly states “NOBREAK not allowed on locked measures” (mirroring the system-lock helper), andMeasureBase::undoSetBreak/cleanupLayoutBreaksdon’t enforce locks themselves—so page-locked measures should be blocked here too for consistency.Current snippet
if (type == LayoutBreakType::NOBREAK && !allNoBreaks) { for (MeasureBase* mb : mbl) { if (mb->systemLock()) { 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/editing/cmd.cpp` around lines 4005 - 4011, The NOBREAK guard in cmdToggleLayoutBreak only checks MeasureBase::systemLock() but should also block measures with MeasureBase::pageLock(); update the conditional loop that inspects mbl when type == LayoutBreakType::NOBREAK (and !allNoBreaks) to check mb->pageLock() as well (mirror the behavior referenced in EditPageLocks::removePageLocksOnAddLayoutBreak) so page-locked measures are rejected just like system-locked ones; no other changes to MeasureBase::undoSetBreak/cleanupLayoutBreaks are needed.
🧹 Nitpick comments (3)
src/notationscene/internal/notationuiactions.cpp (1)
682-690: ⚡ Quick winAlign page-move labels with system-level behavior.
Line 682 and Line 689 say “Move measure…”, but the notation interaction API is system-oriented for page moves (
moveSystemToPrevPage/moveSystemToNextPage). This wording mismatch can confuse users about what will move.Suggested text-only fix
- TranslatableString("action", "Move measure to previous page"), - TranslatableString("action", "Move measure to previous page"), + TranslatableString("action", "Move system to previous page"), + TranslatableString("action", "Move system to previous page"), @@ - TranslatableString("action", "Move measure to next page"), - TranslatableString("action", "Move measure to next page"), + TranslatableString("action", "Move system to next page"), + TranslatableString("action", "Move system to next page"),🤖 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/internal/notationuiactions.cpp` around lines 682 - 690, The action labels for the page-move UI actions are written as “Move measure…” but the underlying notation interaction API operates on systems (moveSystemToPrevPage / moveSystemToNextPage); update the TranslatableString labels used in the UiAction instances (the ones with ids "move-measure-to-previous-page" and "move-measure-to-next-page") to say “Move system to previous page” and “Move system to next page” (including both occurrences of the label/tooltip) so wording aligns with the system-level behavior.src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresFlowSection.qml (1)
71-71: ⚡ Quick winMake
controllera required contract (or guard click handlers).Line 71 declares
controlleras optional, but Lines 94, 128, 146, and 168 call methods unguarded. A missing/miswired binding will throw runtime errors on click.Suggested hardening
- property var controller + required property var controller @@ - onClicked: root.controller.toggleLock() + onClicked: root.controller?.toggleLock?.() @@ - onClicked: root.controller.movePrevious() + onClicked: root.controller?.movePrevious?.() @@ - onClicked: root.controller.moveNext() + onClicked: root.controller?.moveNext?.() @@ - onClicked: root.controller.makeInto() + onClicked: root.controller?.makeInto?.()Also applies to: 94-95, 128-129, 146-147, 168-169
🤖 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/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresFlowSection.qml` at line 71, The property controller is declared optional but is used unguarded in click handlers (methods called from the onClicked handlers referenced around lines 94, 128, 146, 168), which will throw if it's undefined; either make controller a required binding (e.g., convert to a required alias/bind and document it) or add null guards in each click handler before invoking controller methods (e.g., check if controller is truthy and otherwise no-op or log an error). Update the property declaration for controller and/or wrap every invocation site in the MeasuresFlowSection's click handlers with a guard that references the same unique symbol "controller" so missing/miswired bindings won't cause runtime exceptions.src/engraving/dom/system.cpp (1)
313-316: Avoid leaking replaced PageLockIndicator; current code path already deletes the previous one.
System::setPageLockIndicator()(system.cpp:315) is only called fromSystemLayout::layoutPageLockIndicators(), and that function callssystem->deletePageLockIndicator()immediately beforehand (systemlayout.cpp:530 vs 539).System::deletePageLockIndicator()deletes and nullsm_pageLockIndicator(system.cpp:318-322), andSystem::~System()also frees it.- Optional hardening: assert
m_pageLockIndicator == nullptrinsetPageLockIndicator()(or delete the previous pointer there) to prevent future call-site misuse.🤖 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/system.cpp` around lines 313 - 316, System::setPageLockIndicator currently overwrites m_pageLockIndicator and can leak if callers forget to delete the previous indicator; update it to either assert that m_pageLockIndicator == nullptr or safely delete the existing m_pageLockIndicator before assigning the new pointer. Locate System::setPageLockIndicator and add a defensive check (assert or delete-and-null) referencing m_pageLockIndicator, and ensure this behavior is consistent with System::deletePageLockIndicator and System::~System and the call sites such as SystemLayout::layoutPageLockIndicators.
🤖 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/measurebase.cpp`:
- Around line 955-985: measureBaseByTick currently collapses multiple
MeasureBase entries at the same tick by using m_tickIndex.upper_bound(tick),
losing disambiguation when several MeasureBases share a tick; change it to
perform an exact, disambiguated lookup (not upper_bound on tick alone) by using
the composite boundary key used by RangeLock (e.g., tick + boundary type/offset)
or by using m_tickIndex.equal_range(tick) and then selecting the correct
MeasureBase by checking the boundary identity (start vs end, frame vs measure)
so saved locks round-trip to the same MeasureBase; update
MeasureBaseList::measureBaseByTick to accept or derive the extra boundary
identifier and iterate the equal_range results to return the exact MeasureBase
instead of the current upper_bound-based logic.
In `@src/engraving/dom/page.cpp`:
- Around line 293-297: The Page::pageLock() implementation dereferences
firstMeasureBase() unconditionally; update Page::pageLock() to call
firstMeasureBase(), check if the result (firstMeasure) is nullptr, and if so
return a safe default (e.g., nullptr or the page's own lock) instead of
dereferencing; otherwise return firstMeasure->pageLock() as before — modify the
function around firstMeasureBase() / Page::pageLock() to perform this null check
and return the fallback.
- Around line 47-63: Page::firstMeasureBase and Page::lastMeasureBase currently
only check m_systems.front()/back(), which can return nullptr even if other
systems contain measures; fix by iterating m_systems instead of only checking
the ends: in Page::firstMeasureBase loop from the start over m_systems and
return the first non-null result of System::first(); in Page::lastMeasureBase
loop from the end (reverse) over m_systems and return the first non-null result
of System::last(); keep the empty m_systems check and return nullptr if no
systems or no measures found.
In `@src/engraving/dom/score.h`:
- Around line 1054-1059: The inline clearSystemLocks and clearPageLocks
currently just clear m_systemLocks/m_pageLocks and therefore leak RangeLock
objects and skip the endpoint relayout that addPageLock/removePageLock perform;
change these methods (clearSystemLocks and clearPageLocks) to iterate
m_systemLocks.allLocks()/m_pageLocks.allLocks(), call startMB()->triggerLayout()
and endMB()->triggerLayout() on each RangeLock, call muse::DeleteAll(...) to
delete the RangeLock objects, then finally call clear() on the registry,
ensuring behavior matches destructor cleanup and preserves relayout semantics.
In `@src/engraving/dom/utils.cpp`:
- Around line 129-130: The current check returns m_measures.first() for tick <=
0, which incorrectly maps tick 0 to the first node (including zero-duration
VBox/TBox); change the condition to only short-circuit for negative ticks (e.g.
use tick < Fraction(0,1)) so that tick == 0 follows the normal tick-to-measure
lookup path; update the conditional in the function that currently does "if
(tick <= Fraction(0, 1)) { return m_measures.first(); }" to clamp only negatives
and let the lookup logic (used by Selection/range starts) resolve tick 0 to the
first timed MeasureBase.
In `@src/engraving/editing/editpagelocks.cpp`:
- Around line 211-235: The current code only removes locks fully contained in
the selection (uses score->pageLocks()->locksContainedInRange) which leaves
locks that start before startMeasure or end after endMeasure and causes
overlapping RangeLock entries; before rebuilding interval locks, locate any page
locks that overlap the selection boundaries (locks that start < startMeasure &&
end >= startMeasure, and locks that start <= endMeasure && end > endMeasure) and
split or trim them just like makeIntoPage() does: use the same logic to remove
the original boundary RangeLock(s) via undoRemovePageLock(score, lock) and
create new adjusted RangeLock(s) for the portions outside the selection (via
undoAddPageLock(score, new RangeLock(...))) so no lock spans across the
selection before you add the new interval locks with undoAddPageLock.
In `@src/engraving/editing/editsystemlocks.cpp`:
- Around line 421-429: The repair branch currently uses nextMeasure() /
prevMeasure() when rebuilding a surviving RangeLock (see the m2->nextMeasure()
and m1->prevMeasure() usages) which can skip across multi-measure (MM)
boundaries; change those calls to the MM-aware neighbor accessors (use the
codebase's MM-aware next/prev measure helpers for the same objects) so the
newRange start/end are found respecting mm-rest boundaries, and apply the same
change in the corresponding page-lock helper that mirrors this logic; keep the
undoAddSystemLock calls and RangeLock construction but pass the MM-aware
neighbor MeasureBase pointers instead of nextMeasure()/prevMeasure().
In `@src/notationscene/internal/notationactioncontroller.cpp`:
- Around line 285-286: The registered action codes in registerAction
("move-measure-to-prev-page", "move-measure-to-next-page") do not match the
codes used by shortcutsForActionCode in systemlayoutsettingsmodel.cpp
("move-system-to-prev-page", "move-system-to-next-page"); update the strings
passed to registerAction to "move-system-to-prev-page" and
"move-system-to-next-page" so they match the lookup, keeping the handler
functions Interaction::moveSystemToPrevPage and
Interaction::moveSystemToNextPage unchanged; alternatively, if preferred, change
the lookup keys in systemlayoutsettingsmodel.cpp to the "move-measure-..."
variants but ensure both sides use the identical action code names.
In
`@src/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.cpp`:
- Around line 246-254: SystemLayoutSettingsModel::updateScoreIsInPageView calls
currentNotation()->viewMode() without checking for null; add a guard that
fetches currentNotation(), returns or treats absence as LayoutMode::LINE (i.e.,
not page view) if it is null, compute isInPageView from that safe value, compare
to m_scoreIsInPageView and only update/emit scoreIsInPageViewChanged when it
actually changes; reference currentNotation(), viewMode(), LayoutMode::LINE,
m_scoreIsInPageView and emit scoreIsInPageViewChanged in the fix.
---
Outside diff comments:
In `@src/engraving/editing/cmd.cpp`:
- Around line 4005-4011: The NOBREAK guard in cmdToggleLayoutBreak only checks
MeasureBase::systemLock() but should also block measures with
MeasureBase::pageLock(); update the conditional loop that inspects mbl when type
== LayoutBreakType::NOBREAK (and !allNoBreaks) to check mb->pageLock() as well
(mirror the behavior referenced in
EditPageLocks::removePageLocksOnAddLayoutBreak) so page-locked measures are
rejected just like system-locked ones; no other changes to
MeasureBase::undoSetBreak/cleanupLayoutBreaks are needed.
In `@src/engraving/rendering/score/pagelayout.cpp`:
- Around line 586-617: The current loop that increases system distances (in
layoutPage() using restHeight and the pageFilled lambda and the sort over sList)
fails when restHeight is negative because it feeds negative fills back into the
"grow smallest gap" algorithm; update layoutPage() and the similar logic in
distributeStaves() (the code that computes fill, totalFill, iterates k and calls
s->setDistance(d)) to handle negative-space as a separate branch: when
restHeight (or spaceRemaining) < 0, compute how much space must be removed and
identify the largest expandable inter-system gaps (reverse the sort or compute
expandable capacity per System via System::distance() - minimumAllowed or
maxDist - System::height() as relevant), then subtract space from those largest
gaps (clamping at their minimums) until the negative restHeight is consumed,
updating s->setDistance(d) accordingly; do not pass negative fill into the
existing positive-growth loop, and mirror the same change for the
distributeStaves() loop that uses nextSmallest, smallest, step and maxPasses so
that steps remove space from largest gaps instead of producing negative steps
that no-op.
---
Nitpick comments:
In `@src/engraving/dom/system.cpp`:
- Around line 313-316: System::setPageLockIndicator currently overwrites
m_pageLockIndicator and can leak if callers forget to delete the previous
indicator; update it to either assert that m_pageLockIndicator == nullptr or
safely delete the existing m_pageLockIndicator before assigning the new pointer.
Locate System::setPageLockIndicator and add a defensive check (assert or
delete-and-null) referencing m_pageLockIndicator, and ensure this behavior is
consistent with System::deletePageLockIndicator and System::~System and the call
sites such as SystemLayout::layoutPageLockIndicators.
In `@src/notationscene/internal/notationuiactions.cpp`:
- Around line 682-690: The action labels for the page-move UI actions are
written as “Move measure…” but the underlying notation interaction API operates
on systems (moveSystemToPrevPage / moveSystemToNextPage); update the
TranslatableString labels used in the UiAction instances (the ones with ids
"move-measure-to-previous-page" and "move-measure-to-next-page") to say “Move
system to previous page” and “Move system to next page” (including both
occurrences of the label/tooltip) so wording aligns with the system-level
behavior.
In
`@src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresFlowSection.qml`:
- Line 71: The property controller is declared optional but is used unguarded in
click handlers (methods called from the onClicked handlers referenced around
lines 94, 128, 146, 168), which will throw if it's undefined; either make
controller a required binding (e.g., convert to a required alias/bind and
document it) or add null guards in each click handler before invoking controller
methods (e.g., check if controller is truthy and otherwise no-op or log an
error). Update the property declaration for controller and/or wrap every
invocation site in the MeasuresFlowSection's click handlers with a guard that
references the same unique symbol "controller" so missing/miswired bindings
won't cause runtime exceptions.
🪄 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: 1e34b88a-3946-4482-8f5c-02646fd6f327
📒 Files selected for processing (78)
src/engraving/CMakeLists.txtsrc/engraving/api/v1/apitypes.hsrc/engraving/api/v1/elements.cppsrc/engraving/dom/dom.cmakesrc/engraving/dom/engravingobject.hsrc/engraving/dom/factory.cppsrc/engraving/dom/factory.hsrc/engraving/dom/measurebase.cppsrc/engraving/dom/measurebase.hsrc/engraving/dom/navigate.cppsrc/engraving/dom/page.cppsrc/engraving/dom/page.hsrc/engraving/dom/rangelock.cppsrc/engraving/dom/rangelock.hsrc/engraving/dom/score.cppsrc/engraving/dom/score.hsrc/engraving/dom/select.cppsrc/engraving/dom/select.hsrc/engraving/dom/system.cppsrc/engraving/dom/system.hsrc/engraving/dom/utils.cppsrc/engraving/editing/cmd.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/editpagelocks.cppsrc/engraving/editing/editpagelocks.hsrc/engraving/editing/editstyle.cppsrc/engraving/editing/editsystemlocks.cppsrc/engraving/editing/editsystemlocks.hsrc/engraving/rendering/score/layoutcontext.cppsrc/engraving/rendering/score/layoutcontext.hsrc/engraving/rendering/score/measurelayout.cppsrc/engraving/rendering/score/pagelayout.cppsrc/engraving/rendering/score/pagelayout.hsrc/engraving/rendering/score/scorehorizontalviewlayout.cppsrc/engraving/rendering/score/systemlayout.cppsrc/engraving/rendering/score/systemlayout.hsrc/engraving/rendering/score/tdraw.cppsrc/engraving/rendering/score/tdraw.hsrc/engraving/rendering/score/tlayout.cppsrc/engraving/rendering/score/tlayout.hsrc/engraving/rw/read410/tread.cppsrc/engraving/rw/read460/tread.cppsrc/engraving/rw/read500/read500.cppsrc/engraving/rw/read500/tread.cppsrc/engraving/rw/read500/tread.hsrc/engraving/rw/write/twrite.cppsrc/engraving/rw/write/twrite.hsrc/engraving/rw/write/writer.cppsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/page_locks_data/page_locks-1.mscxsrc/engraving/tests/page_locks_data/page_locks-1.msczsrc/engraving/tests/page_locks_tests.cppsrc/engraving/tests/system_locks_tests.cppsrc/engraving/types/types.hsrc/engraving/types/typesconv.cppsrc/notation/inotationinteraction.hsrc/notation/inotationselection.hsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/internal/notationparts.cppsrc/notation/internal/notationselection.cppsrc/notation/internal/notationselection.hsrc/notation/tests/mocks/notationinteractionmock.hsrc/notation/tests/mocks/notationselectionmock.hsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationuiactions.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/CMakeLists.txtsrc/propertiespanel/qml/MuseScore/PropertiesPanel/PropertiesPanelSectionDelegate.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresFlowSection.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresSection.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.hsrc/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanelabstractmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanelabstractmodel.hsrc/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanellistmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/SystemLayoutSection.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.h
💤 Files with no reviewable changes (2)
- src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.h
- src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.cpp
Comment on lines
+955
to
+985
| MeasureBase* MeasureBaseList::measureBaseByTick(int tick) const | ||
| { | ||
| if (empty() || tick > m_last->endTick().ticks()) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| auto it = m_tickIndex.upper_bound(tick); | ||
|
|
||
| if (it == m_tickIndex.begin()) { | ||
| MeasureBase* mb = it->second; | ||
|
|
||
| return mb; | ||
| } | ||
|
|
||
| --it; | ||
| for (;; --it) { | ||
| if (it == m_tickIndex.begin()) { | ||
| MeasureBase* mb = it->second; | ||
|
|
||
| return mb; | ||
| } | ||
|
|
||
| MeasureBase* mb = it->second; | ||
| if (!mb) { | ||
| break; | ||
| } | ||
|
|
||
| return mb; | ||
| } | ||
|
|
||
| return nullptr; |
Contributor
There was a problem hiding this comment.
Persisting arbitrary lock boundaries by tick is ambiguous here.
RangeLock now starts/ends on any MeasureBase, but this helper still collapses a tick to one floor entry from m_tickIndex. That loses information whenever multiple MeasureBases share the same tick (for example a frame and the following measure at tick 0), so a saved page/system lock can round-trip onto the wrong boundary and change which range is actually locked. This lookup needs an exact, disambiguated boundary key instead of upper_bound() on tick alone.
🤖 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/measurebase.cpp` around lines 955 - 985, measureBaseByTick
currently collapses multiple MeasureBase entries at the same tick by using
m_tickIndex.upper_bound(tick), losing disambiguation when several MeasureBases
share a tick; change it to perform an exact, disambiguated lookup (not
upper_bound on tick alone) by using the composite boundary key used by RangeLock
(e.g., tick + boundary type/offset) or by using m_tickIndex.equal_range(tick)
and then selecting the correct MeasureBase by checking the boundary identity
(start vs end, frame vs measure) so saved locks round-trip to the same
MeasureBase; update MeasureBaseList::measureBaseByTick to accept or derive the
extra boundary identifier and iterate the equal_range results to return the
exact MeasureBase instead of the current upper_bound-based logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/score.h (1)
1057-1059:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
clearPageLocks()helper.
m_pageLocksis introduced as the same non-owning registry pattern asm_systemLocks, but the API only exposes add/remove access. After the undo stack releases the owningRangeLocks, there is no matching way to purge stale page-lock pointers from a liveScore.Suggested change
const RangeLocks* pageLocks() const { return &m_pageLocks; } void addPageLock(const RangeLock* lock); void removePageLock(const RangeLock* lock); + void clearPageLocks() { m_pageLocks.clear(); }Based on learnings, page/system lock registries are non-owning containers that should be clearable once the undo edits have released their
RangeLocks.🤖 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/score.h` around lines 1057 - 1059, Add a new public helper void clearPageLocks() to the Score API that clears the non-owning registry m_pageLocks (similar to how system locks are handled) so stale RangeLock pointers can be purged after undo releases owners; update the header by declaring clearPageLocks() alongside pageLocks(), addPageLock(), removePageLock() and implement it to clear or reset m_pageLocks (the same mechanism used for m_systemLocks) so callers can explicitly purge the registry.Source: Learnings
♻️ Duplicate comments (1)
src/engraving/dom/utils.cpp (1)
141-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTick 0 should not short-circuit to
m_measures.first()Line 141-142 maps
tick == 0to the first node, which can be a zero-duration frame (VBox/TBox) instead of the first timedMeasureBase, causing incorrect start anchoring for range/page-lock flows. Clamp only negative ticks and let tick 0 resolve via tick lookup.Suggested fix
- if (tick <= Fraction(0, 1)) { - return m_measures.first(); + if (tick < Fraction(0, 1)) { + return m_measures.first(); } return m_measures.measureBaseByTick(tick.ticks());🤖 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/utils.cpp` around lines 141 - 142, The current check short-circuits tick == 0 to m_measures.first(), which can point to a zero-duration VBox/TBox instead of the first timed MeasureBase; change the guard to only clamp negative ticks (use tick < Fraction(0,1) instead of tick <= Fraction(0,1)) so tick == 0 falls through to the normal tick lookup/resolution logic that will find the first timed MeasureBase; update the condition in the function referencing tick, Fraction, and m_measures.first() accordingly and ensure downstream tick-resolution logic correctly handles a zero tick.
🤖 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/score.cpp`:
- Around line 2066-2067: The code narrows the clone boundary to Measure* causing
leading VBox/HBox/TBox/FBox frames to be skipped; instead preserve MeasureBase*
when computing boundaries and pass those MeasureBase* (fmb and emb) into
appendMeasuresFromScore() or adjust that function signature to accept
MeasureBase* so all MeasureBase-derived objects (boxes/frames and measures)
between fmb and emb are cloned; specifically remove any cast/conversion from
MeasureBase* to Measure* around fmb/emb and ensure
appendMeasuresFromScore/related callers operate on MeasureBase*.
---
Outside diff comments:
In `@src/engraving/dom/score.h`:
- Around line 1057-1059: Add a new public helper void clearPageLocks() to the
Score API that clears the non-owning registry m_pageLocks (similar to how system
locks are handled) so stale RangeLock pointers can be purged after undo releases
owners; update the header by declaring clearPageLocks() alongside pageLocks(),
addPageLock(), removePageLock() and implement it to clear or reset m_pageLocks
(the same mechanism used for m_systemLocks) so callers can explicitly purge the
registry.
---
Duplicate comments:
In `@src/engraving/dom/utils.cpp`:
- Around line 141-142: The current check short-circuits tick == 0 to
m_measures.first(), which can point to a zero-duration VBox/TBox instead of the
first timed MeasureBase; change the guard to only clamp negative ticks (use tick
< Fraction(0,1) instead of tick <= Fraction(0,1)) so tick == 0 falls through to
the normal tick lookup/resolution logic that will find the first timed
MeasureBase; update the condition in the function referencing tick, Fraction,
and m_measures.first() accordingly and ensure downstream tick-resolution logic
correctly handles a zero tick.
🪄 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: c2491567-a624-4f9e-b3f7-62f31019816f
📒 Files selected for processing (11)
musesrc/engraving/dom/page.cppsrc/engraving/dom/score.cppsrc/engraving/dom/score.hsrc/engraving/dom/utils.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/editpagelocks.cppsrc/engraving/editing/editpagelocks.hsrc/notation/internal/notationinteraction.cppsrc/notationscene/internal/notationactioncontroller.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.cpp
✅ Files skipped from review due to trivial changes (1)
- muse
🚧 Files skipped from review as they are similar to previous changes (6)
- src/engraving/editing/editpagelocks.h
- src/engraving/editing/edit.cpp
- src/notation/internal/notationinteraction.cpp
- src/engraving/dom/page.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.cpp
- src/engraving/editing/editpagelocks.cpp
The old implementation of tick2measureBase worked by returning the first MeasureBase which had a duration. As Boxes don't have durations, this always returned a measure. So, the logic has been simplified and it has been renamed The new implementation of `tick2measureBase` matches the behaviour of `tick2measure` - returns the first measurebase containing the given tick
Contributor
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/rendering/score/pagelayout.cpp`:
- Around line 259-265: The page-break check is using Measure (measure)
boundaries instead of MeasureBase boundaries; update the code that computes
endMB/nextMB and the page-lock booleans to use MeasureBase-aware APIs (e.g.,
replace prevSystem()->lastMeasure() with prevSystem()->lastMeasureBase() and use
the MeasureBase successor accessor instead of nextMM()) so endMB and nextMB are
MeasureBase pointers and endOfPageLock/pageLockStart call
isEndOfPageLock()/isStartOfPageLock() on those MeasureBase objects, then keep
isPageBreak logic unchanged but based on these MeasureBase values.
🪄 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: 6303dd9e-c296-40cc-9eec-9a24868ff10e
📒 Files selected for processing (79)
musesrc/engraving/CMakeLists.txtsrc/engraving/api/v1/apitypes.hsrc/engraving/api/v1/elements.cppsrc/engraving/dom/dom.cmakesrc/engraving/dom/engravingobject.hsrc/engraving/dom/factory.cppsrc/engraving/dom/factory.hsrc/engraving/dom/measurebase.cppsrc/engraving/dom/measurebase.hsrc/engraving/dom/navigate.cppsrc/engraving/dom/page.cppsrc/engraving/dom/page.hsrc/engraving/dom/rangelock.cppsrc/engraving/dom/rangelock.hsrc/engraving/dom/score.cppsrc/engraving/dom/score.hsrc/engraving/dom/select.cppsrc/engraving/dom/select.hsrc/engraving/dom/system.cppsrc/engraving/dom/system.hsrc/engraving/dom/utils.cppsrc/engraving/editing/cmd.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/editpagelocks.cppsrc/engraving/editing/editpagelocks.hsrc/engraving/editing/editstyle.cppsrc/engraving/editing/editsystemlocks.cppsrc/engraving/editing/editsystemlocks.hsrc/engraving/rendering/score/layoutcontext.cppsrc/engraving/rendering/score/layoutcontext.hsrc/engraving/rendering/score/measurelayout.cppsrc/engraving/rendering/score/pagelayout.cppsrc/engraving/rendering/score/pagelayout.hsrc/engraving/rendering/score/scorehorizontalviewlayout.cppsrc/engraving/rendering/score/systemlayout.cppsrc/engraving/rendering/score/systemlayout.hsrc/engraving/rendering/score/tdraw.cppsrc/engraving/rendering/score/tdraw.hsrc/engraving/rendering/score/tlayout.cppsrc/engraving/rendering/score/tlayout.hsrc/engraving/rw/read410/tread.cppsrc/engraving/rw/read460/tread.cppsrc/engraving/rw/read500/read500.cppsrc/engraving/rw/read500/tread.cppsrc/engraving/rw/read500/tread.hsrc/engraving/rw/write/twrite.cppsrc/engraving/rw/write/twrite.hsrc/engraving/rw/write/writer.cppsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/page_locks_data/page_locks-1.mscxsrc/engraving/tests/page_locks_data/page_locks-1.msczsrc/engraving/tests/page_locks_tests.cppsrc/engraving/tests/system_locks_tests.cppsrc/engraving/types/types.hsrc/engraving/types/typesconv.cppsrc/notation/inotationinteraction.hsrc/notation/inotationselection.hsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/internal/notationparts.cppsrc/notation/internal/notationselection.cppsrc/notation/internal/notationselection.hsrc/notation/tests/mocks/notationinteractionmock.hsrc/notation/tests/mocks/notationselectionmock.hsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationuiactions.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/CMakeLists.txtsrc/propertiespanel/qml/MuseScore/PropertiesPanel/PropertiesPanelSectionDelegate.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresFlowSection.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresSection.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.hsrc/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanelabstractmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanelabstractmodel.hsrc/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanellistmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/SystemLayoutSection.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.h
💤 Files with no reviewable changes (2)
- src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/measuressettingsmodel.h
✅ Files skipped from review due to trivial changes (2)
- src/engraving/CMakeLists.txt
- src/engraving/tests/page_locks_data/page_locks-1.mscx
🚧 Files skipped from review as they are similar to previous changes (63)
- src/engraving/rw/read500/read500.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanelabstractmodel.h
- src/engraving/rw/write/writer.cpp
- src/engraving/dom/navigate.cpp
- src/engraving/dom/select.h
- src/notationscene/internal/notationuiactions.cpp
- src/engraving/rendering/score/measurelayout.cpp
- src/notation/tests/mocks/notationselectionmock.h
- src/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanelabstractmodel.cpp
- src/engraving/dom/page.h
- src/engraving/types/typesconv.cpp
- src/engraving/types/types.h
- src/engraving/rw/read460/tread.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/CMakeLists.txt
- src/engraving/editing/editstyle.cpp
- src/engraving/rendering/score/scorehorizontalviewlayout.cpp
- src/engraving/rendering/score/tdraw.h
- src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresFlowSection.qml
- src/engraving/api/v1/apitypes.h
- src/engraving/rendering/score/layoutcontext.h
- src/engraving/editing/editpagelocks.h
- src/notation/internal/notationinteraction.h
- src/engraving/rw/read410/tread.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/PropertiesPanelSectionDelegate.qml
- src/notation/internal/notationparts.cpp
- src/engraving/dom/dom.cmake
- src/engraving/editing/cmd.cpp
- src/engraving/dom/page.cpp
- src/engraving/rw/write/twrite.h
- src/notation/inotationselection.h
- src/engraving/dom/engravingobject.h
- src/engraving/rendering/score/systemlayout.cpp
- src/engraving/tests/page_locks_tests.cpp
- src/engraving/editing/editsystemlocks.h
- src/engraving/tests/CMakeLists.txt
- src/engraving/tests/system_locks_tests.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/SystemLayoutSection.qml
- src/engraving/rw/read500/tread.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.h
- src/propertiespanel/qml/MuseScore/PropertiesPanel/measures/MeasuresSection.qml
- src/engraving/rendering/score/systemlayout.h
- src/notation/tests/mocks/notationinteractionmock.h
- src/propertiespanel/qml/MuseScore/PropertiesPanel/propertiespanellistmodel.cpp
- src/engraving/rw/read500/tread.h
- src/engraving/editing/edit.cpp
- src/engraving/rendering/score/pagelayout.h
- src/engraving/dom/factory.h
- src/engraving/rendering/score/layoutcontext.cpp
- src/engraving/api/v1/elements.cpp
- src/engraving/dom/measurebase.h
- src/engraving/rendering/score/tlayout.cpp
- src/engraving/dom/rangelock.cpp
- src/engraving/dom/system.cpp
- src/engraving/dom/select.cpp
- src/engraving/rendering/score/tdraw.cpp
- src/engraving/dom/score.h
- src/engraving/editing/editpagelocks.cpp
- src/notation/internal/notationinteraction.cpp
- src/engraving/rw/write/twrite.cpp
- src/engraving/dom/score.cpp
- src/engraving/editing/editsystemlocks.cpp
- src/propertiespanel/qml/MuseScore/PropertiesPanel/systemlayout/systemlayoutsettingsmodel.cpp
- src/engraving/dom/rangelock.h
Comment on lines
+259
to
+265
| MeasureBase* endMB = ctx.state().prevSystem()->lastMeasure(); | ||
| bool endOfPageLock = endMB && endMB->isEndOfPageLock(); | ||
| const MeasureBase* nextMB = endMB ? endMB->nextMM() : nullptr; | ||
| bool pageLockStart = nextMB && nextMB->isStartOfPageLock(); | ||
|
|
||
| bool isPageBreak = !ctx.state().curSystem() | ||
| || (breakPages && (ctx.state().prevSystem()->pageBreak() || endOfPageLock || pageLockStart)); |
Contributor
There was a problem hiding this comment.
Use MeasureBase system boundaries here, not Measure boundaries.
RangeLock is now anchored on arbitrary MeasureBases, but this break check still asks the previous system for lastMeasure(). If a system ends or the next system starts with a non-measure boundary item, endOfPageLock / pageLockStart can miss the real lock edge and keep or break the page in the wrong place.
Suggested fix
- MeasureBase* endMB = ctx.state().prevSystem()->lastMeasure();
+ MeasureBase* endMB = ctx.state().prevSystem()->last();
bool endOfPageLock = endMB && endMB->isEndOfPageLock();
- const MeasureBase* nextMB = endMB ? endMB->nextMM() : nullptr;
+ const MeasureBase* nextMB = ctx.state().curSystem() ? ctx.state().curSystem()->first() : nullptr;
bool pageLockStart = nextMB && nextMB->isStartOfPageLock();📝 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.
Suggested change
| MeasureBase* endMB = ctx.state().prevSystem()->lastMeasure(); | |
| bool endOfPageLock = endMB && endMB->isEndOfPageLock(); | |
| const MeasureBase* nextMB = endMB ? endMB->nextMM() : nullptr; | |
| bool pageLockStart = nextMB && nextMB->isStartOfPageLock(); | |
| bool isPageBreak = !ctx.state().curSystem() | |
| || (breakPages && (ctx.state().prevSystem()->pageBreak() || endOfPageLock || pageLockStart)); | |
| MeasureBase* endMB = ctx.state().prevSystem()->last(); | |
| bool endOfPageLock = endMB && endMB->isEndOfPageLock(); | |
| const MeasureBase* nextMB = ctx.state().curSystem() ? ctx.state().curSystem()->first() : nullptr; | |
| bool pageLockStart = nextMB && nextMB->isStartOfPageLock(); | |
| bool isPageBreak = !ctx.state().curSystem() | |
| || (breakPages && (ctx.state().prevSystem()->pageBreak() || endOfPageLock || pageLockStart)); |
🤖 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/pagelayout.cpp` around lines 259 - 265, The
page-break check is using Measure (measure) boundaries instead of MeasureBase
boundaries; update the code that computes endMB/nextMB and the page-lock
booleans to use MeasureBase-aware APIs (e.g., replace
prevSystem()->lastMeasure() with prevSystem()->lastMeasureBase() and use the
MeasureBase successor accessor instead of nextMM()) so endMB and nextMB are
MeasureBase pointers and endOfPageLock/pageLockStart call
isEndOfPageLock()/isStartOfPageLock() on those MeasureBase objects, then keep
isPageBreak logic unchanged but based on these MeasureBase values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #31426